This is an archive of the discontinued LLVM Phabricator instance.

[IR] make stack-protector-guard-* flags into module attrs
ClosedPublic

Authored by nickdesaulniers on May 18 2021, 5:51 PM.

Details

Summary

D88631 added initial support for:

  • -mstack-protector-guard=
  • -mstack-protector-guard-reg=
  • -mstack-protector-guard-offset=

flags, and D100919 extended these to AArch64. Unfortunately, these flags
aren't retained for LTO. Make them module attributes rather than
TargetOptions.

Link: https://github.com/ClangBuiltLinux/linux/issues/1378

Diff Detail

Unit TestsFailed

Event Timeline

nickdesaulniers requested review of this revision.May 18 2021, 5:51 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 18 2021, 5:51 PM
nickdesaulniers planned changes to this revision.May 18 2021, 5:53 PM

Obviously needs work/cleanup, changes to x86, and tests, but posting for early feedback about module level attributes vs function level attributes, or possibly something else. I tested this quickly with thin LTO of the Linux kernel and it worked.

  • remove TargetOption members, update tests
nickdesaulniers planned changes to this revision.May 19 2021, 2:37 PM
nickdesaulniers added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
776–783

I should add tests for these; that clang emits these module attributes as expected.

llvm/include/llvm/IR/Module.h
898

TODO: delete this comment

  • add cc1 test
  • remove spurious comment

Obviously needs work/cleanup, changes to x86, and tests, but posting for early feedback about module level attributes vs function level attributes, or possibly something else. I tested this quickly with thin LTO of the Linux kernel and it worked.

I haven't looked through in too much detail, but I see you have the module flag type set as Warning on conflict. I guess the answer to module level vs function level depends on whether it is valid to link together files compiled with different values of these flags and if so what the expected behavior should be. How does this work on non-LTO? If it works just fine, i.e. the functions from the modules with one value are compiled ok with that value and linked with functions compiled effectively with another value, then that would point to a function attribute so you can mimic the mix-and-match behavior. If it is unexpected, then perhaps better to keep as a module flag but Error? With Warning, the value from the first module being linked is used - would that be surprising?

Obviously needs work/cleanup, changes to x86, and tests, but posting for early feedback about module level attributes vs function level attributes, or possibly something else. I tested this quickly with thin LTO of the Linux kernel and it worked.

I haven't looked through in too much detail, but I see you have the module flag type set as Warning on conflict. I guess the answer to module level vs function level depends on whether it is valid to link together files compiled with different values of these flags and if so what the expected behavior should be. How does this work on non-LTO? If it works just fine, i.e. the functions from the modules with one value are compiled ok with that value and linked with functions compiled effectively with another value, then that would point to a function attribute so you can mimic the mix-and-match behavior. If it is unexpected, then perhaps better to keep as a module flag but Error? With Warning, the value from the first module being linked is used - would that be surprising?

Good point; it won't work as intended on mismatch. Let me upgrade that to Error.

  • upgrade module merge strategy to Error
  • upgrade module merge strategy to Error

Probably want a test using llvm-link or llvm-lto to check this behavior (that alike flags are getting propagated as expected and that conflicting ones error)

clang/include/clang/Driver/Options.td
3416

What's the effect of or reason for this change?

clang/test/CodeGen/stack-protector-guard.c
5

Perhaps add a check that the module flags not added without these options

llvm/include/llvm/IR/Module.h
898

add a doxygen comment like the ones for the other interfaces here

nickdesaulniers marked 2 inline comments as done.
  • doxygen comments, test that module flags aren't emitted without flags
  • upgrade module merge strategy to Error

Probably want a test using llvm-link or llvm-lto to check this behavior (that alike flags are getting propagated as expected and that conflicting ones error)

Not many tests under llvm/test/LTO or llvm/test/ThinLTO use llvm-link; is there a more appropriate subdir for such a test for conflicting module attributes?

clang/include/clang/Driver/Options.td
3416

Of the 3 options added in D88631 (mstack_protector_guard_EQ, mstack_protector_guard_offset_EQ, mstack_protector_guard_reg_EQ) 2 are strings (mstack_protector_guard_EQ and mstack_protector_guard_reg_EQ). It was inconsistent that one could be unspecified, where as the other could be unspecified or "none" (but those 2 values were equivalent).

Without this change, in clang/lib/CodeGen/CodeGenModule.cpp I'd need to check that StackProtectorGuardReg != "none" rather than !StackProtectorGuardReg.empty() below.

I can change it back, but I think the asymmetry between mstack_protector_guard_EQ and mstack_protector_guard_reg_EQ in D88631, and I missed that in code review.

I don't think there would be any other observers of such a change.

  • upgrade module merge strategy to Error

Probably want a test using llvm-link or llvm-lto to check this behavior (that alike flags are getting propagated as expected and that conflicting ones error)

Not many tests under llvm/test/LTO or llvm/test/ThinLTO use llvm-link; is there a more appropriate subdir for such a test for conflicting module attributes?

Hmm, maybe in llvm/test/Linker? Or you could presumably use llvm-lto.

clang/include/clang/Driver/Options.td
3416

I see. Does unspecified mean something like just "-mstack-protector-guard-reg=" with nothing after the =? I didn't realize that was supported.

nickdesaulniers planned changes to this revision.May 21 2021, 2:02 PM

Will work on explicit LTO tests.

clang/include/clang/Driver/Options.td
3416

It looks like we validate for that case in the front end already. Specifically, RenderAnalyzerOptions in clang/lib/Driver/ToolChains/Clang.cpp.

$ clang -mstack-protector-guard-reg= ...
clang-13: error: invalid value '' in 'mstack-protector-guard-reg='
tejohnson added inline comments.May 21 2021, 2:17 PM
clang/include/clang/Driver/Options.td
3416

Does that mean that without the "none" handling there is no way to disable? I.e. overriding an earlier value. Not sure how important this is.

  • add llvm/tests/LTO/AArch64/ and test case for module attr mismatch.
llvm/test/LTO/AArch64/stack-protector-guard-module-attrs.ll
2–5 ↗(On Diff #347124)

I used llvm-link here, but please let me know if it would be preferable to convert these to use llvm-lto2 instead?

Also, is there any issue with this new dir, llvm/test/LTO/AArch64/? Do I need to modify any lit cfg files so that the tests don't run for non-aarch64?

Also, I guess this test isn't really specific to aarch64; I don't do any codegen and don't specify the target triple (though, these module attributes only will be emitted from the front end for x86 and aarch64 at the moment; perhaps riscv and ppc64le in the future).

clang/include/clang/Driver/Options.td
3416

Oh, that's a great point. I guess I'm not really sure of the intention of "none" then, @xiangzhangllvm can you comment whether that was the intention?

A quick test in GCC shows that GCC does not accept the value "none" for either -mstack-protector-guard= or -mstack-protector-guard-reg=.

We could strive to support disabling the command line flag once respecified, but I'd rather do it for both of the above two flags, not just -mstack-protector-guard-reg=.

llvm/test/LTO/AArch64/stack-protector-guard-module-attrs.ll
2–5 ↗(On Diff #347124)

Perhaps I should move this test under llvm/test/tools/llvm-link/ or llvm/test/Linker/ ? llvm/test/Linker/ has lots of tests that invoke llvm-link.

  • moved module attr mismatch test to llvm/test/Linker/
tejohnson added inline comments.May 21 2021, 2:52 PM
clang/include/clang/Driver/Options.td
3416

Yeah and it doesn't look like gcc supports anything like -mno-stack-...
So this seems fine the way you have changed it, unless @xiangzhangllvm has a different opinion, but perhaps that could be resolved later and consistently between all the options as you note.

llvm/test/LTO/AArch64/stack-protector-guard-module-attrs.ll
2–5 ↗(On Diff #347124)

Yeah I think you will want to add the aarch64 equivalent of llvm/test/LTO/X86/lit.local.cfg, even if not needed for this initial test.

2–5 ↗(On Diff #347124)

That would be fine too, I don't have a strong opinion either way. I suppose since you don't need any code gen putting it under Linker would be fine (I think I'd prefer that to it going under tools/llvm-link since you aren't testing that tool specifically but rather the module linker behavior.

This revision is now accepted and ready to land.May 21 2021, 2:54 PM
clang/include/clang/Driver/Options.td
3416

Sure, I'm happy to follow up on this if I got it wrong here. In the meantime, this will help our CI go back green, so I've merged it. Thank you much for the review!