Implement building GenMC C++ code #4453
Conversation
|
@rustbot author |
rustbot
added
the
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Here's my first round of comments. :) @rustbot author |
rustbot
added
S-waiting-on-author
|
Should the build script not download anything if cargo is invoked with |
genmc-sys/build.rs
Outdated
| // Adding that path here would also trigger an unnecessary rebuild after the repo is cloned (since cargo detects that as a file modification). | ||
| println!("cargo::rerun-if-changed={RUST_CXX_BRIDGE_FILE_PATH}"); | ||
| println!("cargo::rerun-if-changed=./src_cpp"); | ||
| println!("cargo::rerun-if-changed={GENMC_LOCAL_PATH}"); |
There was a problem hiding this comment.
For some reason, adding this path to the rerun-if-changed if the path doesn't exist yet causes full rebuilds on every build. I think the non-existing directory is seen as a change by cargo.
Here is the output from running CARGO_LOG=cargo::core::compiler::fingerprint=info ./miri build --features=genmc after a complete build without any file modifications (as described here):
cargo::core::compiler::fingerprint: stale: missing "/path/to/miri/genmc-sys/./genmc/"
cargo::core::compiler::fingerprint: fingerprint dirty for miri v0.1.0 (/path/to/miri)
Is this intended behavior for cargo?
There was a problem hiding this comment.
Can you just add GENMC_LOCAL_PATH only if you actually use the local path, not the download?
There was a problem hiding this comment.
Yes, but I think then we wouldn't trigger an automatic rebuild if a user adds the ./genmc directory. That's probably still better than constantly rebuilding, since this case should be rare.
There was a problem hiding this comment.
Oh, you are using the existence of the directory as the signal for using it... yeah that doesn't work with cargo's model, it seems. I'll ask the cargo devs about it, but for now, let's use an env var to signal explicitly that you want a local build instead of a download, and then add that env var to rerun-if-changed.
Yeah I don't think we have a way to check for that. Please ask on some RUst forum (URLO on Zulip) what the typical approach here is, I don't know. |
genmc-sys/build.rs
Outdated
| // Adding that path here would also trigger an unnecessary rebuild after the repo is cloned (since cargo detects that as a file modification). | ||
| println!("cargo::rerun-if-changed={RUST_CXX_BRIDGE_FILE_PATH}"); | ||
| println!("cargo::rerun-if-changed=./src_cpp"); | ||
| println!("cargo::rerun-if-changed={GENMC_LOCAL_PATH}"); |
There was a problem hiding this comment.
Can you just add GENMC_LOCAL_PATH only if you actually use the local path, not the download?
|
which one, adding the warning?
Ah, sorry, stopped reading too early...
Add the flag to disable SB to the tests.
|
…matting exception for downloaded GenMC.
|
As mentioned on Zulip, the license changes are still missing, so this probably shouldn't be merged yet, but otherwise:
@rustbot ready |
rustbot
added
S-waiting-on-review
Co-authored-by: Ralf Jung <[email protected]>
…ng issues) Co-authored-by: Ralf Jung <[email protected]>
|
Thanks! It's probably easiest for me to just do the remaining changes directly instead of you trying to read my mind. Usually I would just push that to your branch, but unfortunately you have disabled the setting that would let me do that. So I had to make a new PR instead: #4498. Please go through the 2nd commit there to see what I changed. |
There was a problem hiding this comment.
I have drastically shortened these docs since they should explain the interface o how to build and work with Miri-GenMC, not all the implementation details.
| @@ -677,8 +677,6 @@ trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> { | |||
| fn run_on_stack_empty(&mut self) -> InterpResult<'tcx, Poll<()>> { | |||
| let this = self.eval_context_mut(); | |||
| // Inform GenMC that a thread has finished all user code. GenMC needs to know this for scheduling. | |||
| // FIXME(GenMC): Thread-local destructors *are* user code, so this is odd. Also now that we | |||
| // support pre-main constructors, it can get called there as well. | |||
There was a problem hiding this comment.
Why did you remove this comment? It is still correct, AFAIK.
This PR is in preparation for adding GenMC support to Miri.
It adds the
genmc-syscrate, which can automatically load GenMC from a git repo (temporarily pointing to a private repo, but will point to the GenMC Github repo after everything is implemented)This PR doesn't add any functionality for GenMC mode, only a test for calling C++ functions from Rust code through the
cxxcrate.