tests: Run more pauth tests in CI and make them pass - #162228
Conversation
|
r? @Enselic rustbot has assigned @Enselic. Use Why was this reviewer chosen?The reviewer was selected based on:
|
3dd50a5 to
f9688a6
Compare
|
How come we don't run these tests in CI? Have that been discussed somewhere before? |
No, not discussed yet. This looks like a bug on my part. Early on when we worked on Let me have a once over for all the tests and see what the CI has to say. In the meantime I'll turn it to a WIP PR. |
This comment has been minimized.
This comment has been minimized.
3ce0220 to
4e177fe
Compare
This comment has been minimized.
This comment has been minimized.
4e177fe to
cd25ff9
Compare
@Enselic this is now ready for review. I've gone over the uses of I can see the test executing (and passing) in the CI:
|
|
Reminder, once the PR becomes ready for a review, use |
cd25ff9 to
63a674d
Compare
There was a problem hiding this comment.
I think the changes themselves look reasonable now, but I'd like us to adjust the commit title and PR title.
A commit title shall make sense on its own, so "the test" is a bit too vague. What do you think of having a commit and PR title like the following?:
tests: Run more pauth tests in CI and make them pass
And as PR and commit description we can have something like:
Some pauth tests have ended up failing since they were not run in CI.
Make them run and make them pass.
I'd say it is also overly separated to have two commits. Are you OK with putting it all in one commit?
Yes to both, thank you for the suggestion. Squashed commits to just one. |
63a674d to
03f9117
Compare
-Zpointer-authentication|
@rustbot ready |
There was a problem hiding this comment.
This looks good to me now, thanks.
Since the tests will now run in a broader set of circumstances it is reasonable to expect some failures when the full CI test suite is run. Therefore I am marking this as rollup=iffy.
@bors r+ rollup=iffy
…r=Enselic tests: Run more pauth tests in CI and make them pass Some pauth tests have ended up failing since they were not run in CI. Make them run and make them pass. This is a follow up to: rust-lang#161183
| type Output = i32; | ||
|
|
||
| fn add(self, other: i32) -> i32 { | ||
| 7 |
There was a problem hiding this comment.
Looks like my local LLVM (top of the tree) was able to perform tail call optimization for this test, while CI's (21?) was not.
That's because the particular CI job that fails runs tests without optimizations:
I suggest we do the same thing as many other tests in ./tests/assembly-llvm and set an explicit and fixed -Copt-level to keep emitted assembly fixed. That way we don't have to make strange (on first glance) changes to minicore.
There was a problem hiding this comment.
Right, thank you for the clarification.
I've set the optimisation level and reverted the minicore changes, in favour of using, already defined, Neg.
Kept the uses of function calls as the generated code shows more pointer authentication instructions (pacibsp, retab).
ab9f9a2 to
d91db79
Compare
This comment has been minimized.
This comment has been minimized.
d91db79 to
9528bf9
Compare
| // CHECK: mov x0, [[FN_REG]] | ||
| // CHECK: b call_through | ||
| // CHECK: pacibsp | ||
| // CHECK: adrp [[GOT_REG:x[0-9]+]], :got:c_func |
There was a problem hiding this comment.
I don't feel qualified to review emitted aarch64 assembly (or at least it is a significant effort for me), so may I please ask you to change the emitted assembly in a separate PR and only add the opt-level line to my previously approved commit (in the same, single commit)?
It would be nice to get this CI improvement merged before iterating a change on one specific test.
There was a problem hiding this comment.
That's fair. Reverted to just the opt level change.
9528bf9 to
8a32403
Compare
Some pauth tests have ended up failing since they were not run in CI. Make them run and make them pass.
8a32403 to
96b181c
Compare
…r=Enselic tests: Run more pauth tests in CI and make them pass Some pauth tests have ended up failing since they were not run in CI. Make them run and make them pass. This is a follow up to: rust-lang#161183
Rollup of 9 pull requests Successful merges: - #162228 (tests: Run more pauth tests in CI and make them pass) - #162493 (Add support for -Zsanitizer-cfi-minimal-runtime) - #163271 (add a leak check test) - #163282 (cleanup: clean up more dependencies that are unused) - #157562 (Avoid computing layout of enums with non-int discriminants) - #162275 (Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits) - #163037 (fix and test `va_arg` on `f128` on `x86`) - #163114 (Add regression test for hang on mutually recursive trait impls) - #163255 (Check the entire library and cg_clif workspaces for permitted deps in tidy)
Rollup of 8 pull requests Successful merges: - #162228 (tests: Run more pauth tests in CI and make them pass) - #163271 (add a leak check test) - #163282 (cleanup: clean up more dependencies that are unused) - #157562 (Avoid computing layout of enums with non-int discriminants) - #162275 (Do not increase recursion depth for coroutine witness and rigid opaques when proving auto traits) - #163037 (fix and test `va_arg` on `f128` on `x86`) - #163114 (Add regression test for hang on mutually recursive trait impls) - #163255 (Check the entire library and cg_clif workspaces for permitted deps in tidy)
View all comments
Some pauth tests have ended up failing since they were not run in CI.
Make them run and make them pass.
This is a follow up to: #161183