Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create RFC for MIR linker #1600

Merged
merged 13 commits into from
Sep 7, 2022
244 changes: 244 additions & 0 deletions rfcs/src/rfcs/0001-mir-linker.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
- **Feature Name:** `MIR` Linker (mir_linker)
celinval marked this conversation as resolved.
Show resolved Hide resolved
- **RFC Tracking Issue**: <https://github.com/model-checking/kani/issues/1588>
- **RFC PR:** <https://github.com/model-checking/kani/pull/1600>
- **Status:** Under Review
- **Version:** 0

-------------------

## Summary

Fix linking issues with the rust standard library in a scalable manner by only generating `goto-program` for
celinval marked this conversation as resolved.
Show resolved Hide resolved
code that is reachable from the user harnesses.

## User Impact

The main goal of this RFC is to enable Kani users to link against all supported constructs from the `std` library.
Currently, Kani will only link to items that are either generic or have an inline annotation.

The approach introduced in this RFC will have the following secondary benefits.
- Reduce spurious warnings about unsupported features for cases where the feature is not reachable from any harness.
- In the verification mode, we will likely see a reduction on the compilation time and memory consumption
by pruning the inputs of symtab2gb and goto-instrument.
celinval marked this conversation as resolved.
Show resolved Hide resolved
- Linking against the standard library goto-models takes more than 5 GB which is not feasible with the current
GitHub workers.
celinval marked this conversation as resolved.
Show resolved Hide resolved
- In a potential assessment mode, only analyze code that is reachable from all public items in the target crate.

One downside is that our release bundle will be bigger.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much larger?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approx 26MB bigger for linux 64bits. Kani 0.9 bundle size was 22MB. So this will more than double the size. That said, we also download the entire rustup toolchain as part of kani setup and I believe it has more than 50MB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the text a bit. Let me know if it's clearer now.

We are going to include compilation artifacts for the `std` and `kani` libraries.
This will negatively impact the time taken to set up Kani
(triggered by either the first time a user invokes `kani | cargo-kani` , or explicit invoke the subcommand `setup`).

## User Experience

Once this RFC has been stabilized users shall use Kani in the same manner as they have been today.
Until then, we wil add an unstable option `--mir-linker` to enable the cross-crate reachability analysis
and the generation of the `goto-program` model only when compiling the target crate.

Kani setup will likely take longer and more disk space as mentioned in the section above.
This change will not be guarded by `--mir-linker` option above.

## Detailed Design

In a nutshell, we will no longer generate a `goto-program` model for every crate we compile.
Instead, we will generate the `MIR` for every crate, and we will generate only one `goto-program` model.
This model will only include items reachable from the target crate's harnesses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider one per harness to get even more slicing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally. I put that as a future work for now. Let me know if you think this should be in the scope of the initial work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielsn is it OK if I leave it like this for the first version of the MIR Linker?


The current system flow for a crate verification is the following (Kani here represents either `kani | cargo-kani`
executable):

1. Kani compiles the user crate as well as all its dependencies.
For every crate compiled, `kani-compiler` will generate a `goto-program` model.
This model includes everything reachable from the crate's public functions.
2. After that, Kani links all models together by invoking `goto-cc`.
This step will also link against Kani's `C` library.
3. For every harness, Kani invokes `goto-instrument` to prune the linked model to only include items reachable from the given harness.
4. Finally, Kani instruments and verify each harness model via `goto-program` and `cbmc` calls.
celinval marked this conversation as resolved.
Show resolved Hide resolved

After this RFC, the system flow would be slightly different:

1. Kani compiles the user crate dependencies up to the `MIR` translation.
I.e., for every crate compiled, `kani-compiler` will generate an artifact that includes the `MIR` representation
celinval marked this conversation as resolved.
Show resolved Hide resolved
of all items in the crate.
2. Kani will generate the `goto-program` only while compiling the target user crate.
It will generate one `goto-program` model that includes all items reachable from any harness in the target crate.
3. `goto-cc` will still be invoked to link the generated model against Kani's `C` library.
4. Steps #3 and #4 above will be performed without any change.

This feature will require three main changes to Kani which are detailed in the sub-sections below.

### Kani's Sysroot

Kani currently uses `rustup` sysroot to gather information from the standard library constructs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unclear on this section. What standard library constructs are looked up in sysroot? Does sysroot have MIR lying around?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The rustup sysroot does have MIR lying around. They are inside the *rlib files shipped with the toolchain (see more details here: https://rustc-dev-guide.rust-lang.org/backend/libs-and-metadata.html#metadata).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhassan-aws what kind of information would you like me to add here to make this more clear?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine as is.

The artifacts from this `sysroot` include the `MIR` for generic items as well as for items that may be included in
a crate compilation (e.g.: functions marked with `#[inline]` annotation).
The artifacts do not include the `MIR` for items that have already been compiled to the `std` shared library.
This leaves a gap that cannot be filled by the `kani-compiler`;
thus, we are unable to translate these items into `goto-program`.
celinval marked this conversation as resolved.
Show resolved Hide resolved

In order to fulfill this gap, we must compile the standard library from scratch.
This RFC proposes a similar method to what [`MIRI`](https://github.com/rust-lang/miri) implements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can... we just use what MIRI ships?

We install a rust toolchain on install. I'd been thinking about whether we could reduce this to a minimal profile as an optimization, but we should also be able to go the other way and add the miri component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point. I'm assuming the overhead will be the same, but it might save us from maintaining yet another build script.

Let me investigate that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be awkward to use MIRI to grab the sysroot. MIRI only build the sysroot when you first execute cargo miri [run | test]. It also requires xargo in order to do that.

That said, it might not be that bad to build the sysroot instead of downloading it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedinski please let me know your thoughts on this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shucks, I was hoping MIRI would ship it. I'm ok with adding it as part of setup or pre-building it. Though I'm guessing MIRI doesn't pre-build it for a good reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk what's the motivation to build MIRI sysroot as part of the first execution of MIRI opposed to pre-building it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

miri can cross interpret, so we'd need to deploy all targets somewhere once (instead of once per host). Also sometimes you'll want to change how the sysroot is built I guess.

Also we need the ability to build the sysroot from scratch anyway for when we modify miri out of tree.

We will generate our own sysroot using the `-Z always-encode-mir` compilation flag.
This sysroot will be pre-compiled and included in our release bundle.

We will compile `kani`'s libraries (`kani` and `std`) also with `-Z always-encode-mir`
and with the new sysroot.


### Cross-Crate Reachability Analysis

`kani-compiler` will include a new `reachability` module to traverse over the local and external `MIR` items.
This module will `monomorphize` all generic code as it's performing the traversal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to not monomorphize everything, what would be the implications? Does it make the process impossible or the only side-effect would be to potentially add include too many items?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a great question! Conceptually, I don't see any reason why the reachability code couldn't work with polymorphic definitions. If we decide to contribute this code to upstream or put in separate library, it would be a great thing to do.

For our current goal, it does make sense to monomorphize here since Kani's compiler just wants a list of items to generate code for.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes totally agree

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustc already has logic for this in https://github.com/rust-lang/rust/blob/master/compiler/rustc_monomorphize/src/collector.rs

Maybe it could be generalized to allow you to plug into it and make it work cross crate (rustc only needs cross crate for generic functions and will assume nongeneric functions got codegened upstream)

We have for now given up on mir-only rlibs, but if we want to pick up that work, we'd need exactly what you are describing anyway, so it doesn't seem unreasonable to change the collector to support it optionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great! Most of the code I've written so far was based on the collector logic.


The traversal logic will be customizable allowing different starting points to be used.
The two options to be included in this RFC is starting from all local harnesses
(tagged with `#[kani::proof]`) and all public functions in the local crate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that under --test tests are marked public and that keep them here?

I also wonder if we should just... include both by default and not have options here? This avoids the complication of setting/flags/configuration, doesn't seem like a major downside with slightly more code compiled, and makes everything more flexible.

E.g. a few minutes of thought comes up with the case of Rust calling C code and that C calling Rust code that's "extern C" and which we can't inspect the reachability of, but including both harnesses and public items both would handle seamlessly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is two parts to this:
1- I believe the reachability code should be kept generic to take whatever entry point we want.
2- For now, we will only provide two possible choices, harnesses and public functions.

I do see other future usages for the starting point. C + Rust verification will likely be a combination of harness + extern C functions.

For --tests, I believe we still want to use the harness mode unless if the user chooses --function. For the later, I might just keep the legacy mode for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedinski does that make sense? Let me know if you want me to change anything or make the text more clear about the customization points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with whichever course you choose, but my preference is that we should error on the side of simplicity here. IMO it should just use "public + harnesses" as roots and then we don't need to worry about other options. It seems like the worse case scenario is just codegen'ing a bit more than we need, while the advantage is that everything just works and nobody has to think about this again.


The `kani-compiler` behavior will be customizable via a new flag:

```
--reachability=[ harnesses | pub_fns | none | legacy* ]
```

where:

- `harnesses`: Use the local harnesses as the starting points for the reachability analysis.
- `pub_fns`: Use the public local functions as the starting points for the reachability analysis.
celinval marked this conversation as resolved.
Show resolved Hide resolved
- `none`: This will be the default value if `--reachability` flag is not provided. It will basically skip
reachability analysis (and `goto-program` generation).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, goto-programs will still be generated, no?

Copy link
Contributor Author

@celinval celinval Sep 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. With none we won't generate any goto-program. This will be used while compiling the dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedinski and @adpaco-aws I added a bit more information here. Let me know if that makes sense.

`kani-compiler` will still generate artifacts with the crate's `MIR`.
celinval marked this conversation as resolved.
Show resolved Hide resolved
- `legacy`: Keep `kani-compiler` current behavior by using
`rustc_monomorphizer::collect_and_partition_mono_items()` which respects the crate boundary.
This will generate a `goto-program` for each crate compiled by `kani-compiler`, and it will still have the same
`std` linking issues.

*This option will be removed as part of the `rfc` stabilization.

These flags will not be exposed to the final user.
They will only be used for the communication between `kani-driver` and `kani-compiler`.

### Dependencies vs Target Crate Compilation

The flags described in the section above will be used by `kani-driver` to implement the new system flow.
For that, we propose the following mechanism:

- For standalone `kani`, we will pass the option `--reachability=harnesses` to `kani-compiler`.
- For `cargo-kani`, we will replace
```
cargo build <FLAGS>
```

with:

```
cargo rustc <FLAGS> -- --reachability=harnesses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when we run cargo kani --tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question. At some point I thought about adding this to the "Open Questions", but I totally forgot. I'll do that now.

I don't have a great answer yet. The two possibilities I thought so far are on the hacky side, but I believe they would work:

  1. Add a new value to --reachability, maybe "on-tests" or something like that. This would behave as either "harness" or "none" depending whether RUSTFLAGS has --tests.
  2. Change how we handle not having --reachability argument to also take into account the existence of --tests.

Both would ensure that we only run the reachability and codegen at the last compilation step. Any other suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedinski any suggestions? I added this to the open questions too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was less on the reachability side here, and more on the side of the actual pragmatics of doing the build.

cargo build becomes cargo rustc but what does cargo test --no-run become?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will likely stay as is and we might add a --reachability=on-tests or something like that.

Copy link
Contributor Author

@celinval celinval Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedinski I have great news, we can replace

cargo test --no-run

by

cargo rustc --tests -- --reachability=harnesses

I'll add some details here and remove that from the open questions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!

```

to build everything.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, we've been primarily using the KANIFLAGS environment variable to pass things to the kani-compiler? If this is correct, any reason to switch to using cargo rustc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason is that we want to pass an extra flag to the target crate which we wouldn't pass to the dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this will still keep KANIFLAGS working as is today.

This command will compile all dependencies without the `--reachability` argument, and it will only pass `harnesses`
value to the compiler when compiling the target crate.

## Rational and Alternatives

Not doing anything is not an alternative, since this fixes a major gap in Kani's usability.

### Benefits

- The MIR linker will allow us to fix the linking issues with Rust's standard library.
- Once stabilized, the MIR linker will be transparent to the user.
- It will enable more powerful and precise static analysis to `kani-compiler`.
- It won't require any changes to our dependencies.
- This will fix the harnesses' dependency on the`#[no_mangle]` annotation
([Issue-661](https://github.com/model-checking/kani/issues/661)).

### Risks

Failures in the linking stage would not impact the tool soundness. I anticipate the following failure scenarios:
- ICE (Internal compiler error): Some logic is incorrectly implemented and the linking stage crashes.
Although this is a bad experience for the user, this will not impact the verification result.
- Missing items: This would either result in ICE during code generation or a verification failure if the missing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if its used as a potential function pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point. CBMC should catch that once the following PR gets merged: diffblue/cbmc#6987

Another possibility is to change how we invoke --goto-instrument as pointed out here: diffblue/cbmc#6983 (comment)

I created #1621 to address that issue and added to https://github.com/model-checking/kani/milestone/12

item is reachable.
- Extra items: This shouldn't impact the verification results, and they should be pruned by CBMC's reachability
analysis.
celinval marked this conversation as resolved.
Show resolved Hide resolved
This is already the case today. In extreme cases, this could include a symbol that we cannot compile and cause an ICE.

The new reachability code would be highly dependent on the `rustc` unstable APIs, which could increase
the cost of the upstream synchronization.
That said, the APIs that would be required are already used today.
celinval marked this conversation as resolved.
Show resolved Hide resolved
celinval marked this conversation as resolved.
Show resolved Hide resolved

Finally, this implementation relies on a few unstable options from `cargo` and `rustc`.
These APIs are used by other tools such as MIRI, so we don't see a high risk that they would be removed.

### Alternatives

The other options explored were:
1. Pre-compile the standard library and ship the generated `*symtab.json` files.
2. Pre-compile the standard library, convert them to `goto-programs` (via `symtab2gb`) and link them into
one single `goto-program` model. Ship the generated model.
celinval marked this conversation as resolved.
Show resolved Hide resolved

Both alternatives are very similar. They only differ on the artifact that would be shipped.
They require generating and shipping a custom `sysroot`;
however, there is no need to implement the reachability algorithm.

We implemented a prototype for the MIR linker and one for the alternatives.
Both prototypes generate the sysroot as part of the `cargo kani` flow.

We performed a small experiment (on a `c5.4xlarge` ec2 instance running Ubuntu 20.04) to assess the options.

For this experiment, we used the following harness:
```rust
#[kani::proof]
#[kani::unwind(4)]
pub fn check_format() {
assert!("2".parse::<u32>().unwrap() == 2);
}
```
The experiment showed that the MIR linker approach is much more efficient.

See the table bellow for the breakdown of time (in seconds) taken for each major step of
the harness verification:


| Stage | MIR Linker | Alternative |
----------------------------|------------|-------------|
| compilation | 22.2s | 64.7s |
| `goto-program` generation | 2.4s | 90.7s |
| `goto-program` linking | 0.8s | 33.2s |
| code instrumentation | 0.8s | 33.1 |
| verification | 0.5s | 8.5s |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results are amazing! I'd expect memory usage to be significantly lower with the MIR linker as well.

If you still have the log files, can you compare the size of the SAT instance (clauses/literals) between the two alternatives? I'm curious to know how much CBMC's slicer is able to reduce the problem size, and how close it can get to the one generated by the MIR linker.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I totally forgot to include the memory usage here.

Both prototypes run the test described in CI. So you can see the log files for the CI run for MIR linker and the alternative.

The specific data I believe you are asking for is:

14089 variables, 180 clauses

and

21610 variables, 7514 clauses

for MIR Linker and the alternative, respectively.

It just occurred to me that I should try to check the difference under visualize. It might make it more clear to understand the difference.


It is possible that `goto-cc` time can be improved, but this would also require further experimentation and
expertise that we don't have today.

Every option would require a custom sysroot to either be built or shipped with Kani.
The table below shows the size of the sysroot files for the alternative #2
(`goto-program` files) vs compiler artifacts (`*.rmeta` files)
files with `-Z always-encode-mir` for `x86_64-unknown-linux-gnu` (on Ubuntu 18.04).

| File Type | Raw size | Compressed size |
|----------------|----------|-----------------|
| `symtab.json` | 950M | 26M |
| `symtab.out` | 84M | 24M |
| `*.rmeta` | 92M | 25M |

These results were obtained by looking at the artifacts generated during the same experiment.

## Open questions

- Should we build or download the sysroot during `kani setup`?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we handle cases during development when we modify something the sysroot depends on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During development, we shall build the sysroot together with all our binaries. I haven't figured out the best mechanism yet though. Building the sysroot as part of the kani-compiler build isn't quite working for me. The sysroot will require different cargo configuration, so I am considering splitting our workspace and creating a new workspace for the library folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielsn Would you like me to add this information here? I tried to focus on the end user point of view, but it might make sense to clarify this.

- What's the best way to enable support to run Kani in the entire `workspace`?
- One possibility is to run `cargo rustc` per package.
- Should we codegen all static items no matter what? Static objects can only be initialized via constant function.
celinval marked this conversation as resolved.
Show resolved Hide resolved
Thus, it shouldn't have any side effect.
That relies on all constant initializers being evaluated during compilation.

## Future possibilities

- Split `goto-program` models into two or more items to optimize compilation result caching.
celinval marked this conversation as resolved.
Show resolved Hide resolved
- Dependencies: One model will include items from all the crate dependencies.
This model will likely be more stable and require fewer updates.
- Target crate: The model for all items in the target crate.
- Do the analysis per-harness. This might be adequate once we have a mechanism to cache translations.
- Add an option to include external functions to the analysis starting point in order to enable verification when
calls are made from `C` to `rust`.
- Contribute the reachability analysis code back to upstream.