r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

Some changes occurred in tests/codegen/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

Thanks Ramon for the very quick PR!

I think the matching LLVM PR is llvm/llvm-project#121070, not the other one, right?

Also, would it make sense to add a quick check for the instructions generated, so that we are sure LLVM is actually doing something? e.g. one of the ones from https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/kcfi-arity.ll

I think the matching LLVM PR is llvm/llvm-project#121070, not the other one, right?

Thanks for pointing that out! Sorry, I confused them. Fixed.

Also, would it make sense to add a quick check for the instructions generated, so that we are sure LLVM is actually doing something? e.g. one of the ones from https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/kcfi-arity.ll

For module flags, we usually add tests up to to verifying that flags are added only because the side effects of adding them are not implemented in the Rust compiler (so we don't need to keep track of changes and update tests when changes are made in LLVM). Would that be okay for you?

Do we want to add a check when this flag is set that the LLVM is new enough? I think LLVM will silently ignore unknown module tags, so as is, this could result in someone setting -Zsanitizer-kcfi-arity with a rustc that can't actually produce the new-style tags, and being confused by tags that are incompatible with a clang that has that features enabled.

For module flags, we usually add tests up to to verifying that flags are added only because the side effects of adding them are not implemented in the Rust compiler (so we don't need to keep track of changes and update tests when changes are made in LLVM). Would that be okay for you?

Up to the Rust compiler team, of course -- I am not sure what their policy is.

In any case, to be clear, I only meant it as a smoke test, rather than testing everything that LLVM does. In other words, just to be sure that LLVM actually did something with the module flag, rather than replicating every test in LLVM.

From a quick test, it seems LLVM (well, at least llc in Compiler Explorer) does not say anything if there is a typo in the module flag in the IR.

So what I thought is to check e.g. that we get, say, a mov edx, ... with the flag instead of a mov eax, ... without the flag.

r? @davidtwco

Do we want to add a check when this flag is set that the LLVM is new enough? I think LLVM will silently ignore unknown module tags, so as is, this could result in someone setting -Zsanitizer-kcfi-arity with a rustc that can't actually produce the new-style tags, and being confused by tags that are incompatible with a clang that has that features enabled.

Yes, I think it's a good idea. Done. It seems it's the first time such a check is added for an LLVM-related option so I hope I did it right.

P.S. the implementation chosen for the KCFI arity indicator was to use different registers in the mov instruction as the arity indicator instead of augmenting the KCFI tag with this information. This is entirely implemented in LLVM and visible only in the target architecture assembly (and that is why it's tricky and probably not the right place to test it in the Rust compiler tests).

From a quick test, it seems LLVM (well, at least llc in Compiler Explorer) does not say anything if there is a typo in the module flag in the IR.

So what I thought is to check e.g. that we get, say, a mov edx, ... with the flag instead of a mov eax, ... without the flag.

For non module flags, we test option's side effects (which are actually implemented in the Rust compiler, such as CFI/KCFI checks) in the LLVM assembly, not the target architecture assembly, and since the implementation for the KCFI arity indicator uses different registers in a mov operation in the target architecture assembly as the arity indicator, we'd have to go even further out from what we usually do module flags (which is just test that the module flag was emitted and emitted correctly, see #129373 as an example), and test the target architecture assembly after the KCFI pass is applied and the target architecture assembly is emitted. I'll see if there is a good way to do a smoke test for this, but I still think this a contract and that it's LLVM's responsibility to test the actual side effects of the module flag-- we'd still caught with out test if we didn't pass or if there was a typo in the module flag.

This is entirely implemented in LLVM and visible only in the target architecture assembly (and that is why it's tricky and probably not the right place to test it in the Rust compiler tests).

For non module flags, we test option's side effects (which are actually implemented in the Rust compiler, such as CFI/KCFI checks) in the LLVM assembly, not the target architecture assembly,

Hmm... I am not sure what you mean. Perhaps there is a different policy for LLVM module flags vs. other things like LLVM attributes, but the Rust compiler has assembly tests in tests/assembly/.

For instance, when I added -Zno-jump-tables and -Zfunction-return, I added both an LLVM IR level test as well as an asm level test: 2d47622 ("Add -Zfunction-return={keep,thunk-extern} option"), a65ec44 ("Add -Zno-jump-tables").

Not doing anything is not the end of the world, but it means it is fairly easy for LLVM to e.g. rename the module flags and we wouldn't notice until someone builds the kernel with that compiler and KCFI etc. enabled, as far as I understand, and then it is a mess on the kernel side to add Kconfigs to workaround it.

If adding the test is complex, then sure, we can avoid it. But I don't understand what the issue is adding it. I may be missing a policy thing here, but if it is just the technical side, what is the issue?

In fact, for similar reasons, I would suggest adding one to the feature the PR you linked adds.

Hmm... I am not sure what you mean. Perhaps there is a different policy for LLVM module flags vs. other things like LLVM attributes, but the Rust compiler has assembly tests in tests/assembly/.

At least that is my understanding (i.e., to not duplicate LLVM tests and perform tests in LLVM assembly as much as possible). (Notice there are no sanitizer tests in there.)

For instance, when I added -Zno-jump-tables and -Zfunction-return, I added both an LLVM IR level test as well as an asm level test: 2d47622 ("Add -Zfunction-return={keep,thunk-extern} option"), a65ec44 ("Add -Zno-jump-tables").

Not doing anything is not the end of the world, but it means it is fairly easy for LLVM to e.g. rename the module flags and we wouldn't notice until someone builds the kernel with that compiler and KCFI etc. enabled, as far as I understand, and then it is a mess on the kernel side to add Kconfigs to workaround it.

Adding it is not the end the world either--I'll just add it :) (But if LLVM starts to arbitrarily changing module flags and like this, I'd argue we'd have much more serious problems.)

In fact, for similar reasons, I would suggest adding one to the feature the PR you linked adds.

Since I'll create the sanitizer/ directory structure there and add a test for this module flag, I'll revisit the other module flags and add similar tests for them accordingly.

@bors r=davidtwco

📌 Commit a98546b has been approved by davidtwco

It is now in the queue for this repository.