-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Migrate x86_64-fortanix-unknown-sgx-lvi
run-make
test to rmake
#129055
Conversation
This PR modifies cc @jieyouxu |
I'm going to be real with you, I have absolutely no clue about sgx. So I'm going to request help from our sgx target maintainers after I take a look and try to guess what the test is trying to test. |
This is about verifying that LLVM cross-compiles SGX code with load value injection defenses. It should be possible to make it work after building the cross-compile target. |
This target is tier 2 so we should be building the cross-compilation artifacts every time anyways. |
1f9cc33
to
dc650f6
Compare
So: per policy, tests don't have to pass on SGX, a tier 2 target, in order to merge this. As this is an SGX-only test, we can just merge this through. I do know, however, that the SGX maintainers do in fact run the CI regularly and add ignore-sgx, etc. and are reasonably responsive, usually. @jethrogb @raoulstrackx @mzohreva Can we get your review? |
Thanks for tagging @workingjubilee ! I have a day off today, but I'll try to look at it on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the port, I have several questions (that we will need help from the sgx maintainers).
@raoulstrackx Thank you very much for helping out! ❤️ |
dc650f6
to
aa15961
Compare
The run-make-support library was changed cc @jieyouxu |
aa15961
to
df2ec7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment this PR breaks CI. See comments. Can you please check it? You shouldn't have to have an SGX machine as the enclaves don't need to be executed.
9e0ff38
to
ffdde64
Compare
ffdde64
to
b5a6a38
Compare
There are other issues as well. Please apply:
|
b5a6a38
to
6a9e9fc
Compare
Oh dear, I should have watched where my editor's multiple cursors were going. Thank you so much once again! Changes applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the tests all pass. :)
6a9e9fc
to
e276d22
Compare
I deleted both Thank you so much for all your help!! This would have been much harder without a fortanix-rust maintainer. |
@raoulstrackx thank you so much for helping to review and test 💚 |
They're still in the change history, and now the rmake.rs version reimplements them, so we shouldn't keep them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM as well. Feel free to r=me after PR CI is green.
@bors delegate+ |
✌️ @Oneirical, you can now approve this pull request! If @jieyouxu told you to " |
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128876 (Ship MinGW-w64 runtime DLLs along with `rust-lld.exe` for `-pc-windows-gnu` targets) - rust-lang#129055 (Migrate `x86_64-fortanix-unknown-sgx-lvi` `run-make` test to rmake) - rust-lang#129386 (Use a LocalDefId in ResolvedArg.) - rust-lang#129400 (Update `compiler_builtins` to `0.1.120`) - rust-lang#129414 (Fix extern crates not being hidden with `doc(hidden)`) - rust-lang#129417 (Don't trigger refinement lint if predicates reference errors) - rust-lang#129433 (Fix a missing import in a doc in run-make-support) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128876 (Ship MinGW-w64 runtime DLLs along with `rust-lld.exe` for `-pc-windows-gnu` targets) - rust-lang#129055 (Migrate `x86_64-fortanix-unknown-sgx-lvi` `run-make` test to rmake) - rust-lang#129386 (Use a LocalDefId in ResolvedArg.) - rust-lang#129400 (Update `compiler_builtins` to `0.1.120`) - rust-lang#129414 (Fix extern crates not being hidden with `doc(hidden)`) - rust-lang#129417 (Don't trigger refinement lint if predicates reference errors) - rust-lang#129433 (Fix a missing import in a doc in run-make-support) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#129055 - Oneirical:fortanix-fortification, r=jieyouxu Migrate `x86_64-fortanix-unknown-sgx-lvi` `run-make` test to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). The final Makefile! Every Makefile test is now claimed. This is difficult to test due to the uncommon architecture it is specific to. I don't think it is in the CI (I didn't find it in `jobs.yml`, but if there is a way to test it, please do. Locally, on Linux, it compiles and panics at the `llvm_filecheck` part (if I replace the `x86_64-fortanix-unknown-sgx` with `x86_64-unknown-linux-gnu`, of course), which is expected. For this reason, the Makefile and associated script have been kept, but with a leading underscore.
Part of #121876 and the associated Google Summer of Code project.
The final Makefile! Every Makefile test is now claimed.
This is difficult to test due to the uncommon architecture it is specific to. I don't think it is in the CI (I didn't find it in
jobs.yml
, but if there is a way to test it, please do.Locally, on Linux, it compiles and panics at the
llvm_filecheck
part (if I replace thex86_64-fortanix-unknown-sgx
withx86_64-unknown-linux-gnu
, of course), which is expected.For this reason, the Makefile and associated script have been kept, but with a leading underscore.