[X86] Fix CodeGen Module Flag for -mibt-seal

[joaomoreira]

Summary

When assertions are enabled, clang will perform RoundTrip for CompilerInvocation argument generation. ibt-seal flags are currently missing in this argument generation, and because of that, the feature doesn't get enabled for these cases. Performing RoundTrip is the default for assert builds, rendering the feature broken in these scenarios.

This patch fixes this and adds a test to properly verify that modules are being generated with the flag when -mibt-seal is used.

Please, add any known relevant reviewer which I may have missed.

[1] - https://reviews.llvm.org/D116070

[gftg]

Hi Joao,

I understand what the test does and it looks correct. However, I wasn't able to reproduce the problem that this patch is trying to fix (maybe I simply haven't understood the problem, so please bear with me).

Taking just the new test as reference and a compiler built from the main branch at c415ff186dbb (committed today), if I build the simple test program, I get the following:

$ cat a.c
void foo() {}

$ bin/clang a.c -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal -flto
<snip>
!llvm.module.flags = !{!0, !1, !2, !3, !4, !5}
<snip>
!3 = !{i32 4, !"ibt-seal", i32 1}

which I suppose means that -mibt-seal is being forwarded to the backend.

Also, if I actually link the test program, the ENDBR in foo is gone:

$ bin/clang a.c -shared -o a.so -mcmodel=kernel -fcf-protection=branch -mibt-seal -flto -fuse-ld=lld
$ objdump -d a.so | grep "<foo>:" -A 5
0000000000001630 <foo>:
    1630:       55                      push   %rbp
    1631:       48 89 e5                mov    %rsp,%rbp
    1634:       5d                      pop    %rbp
    1635:       c3                      ret

What have I got wrong?

PS: With this patch applied, the result is the same.

[joaomoreira]

Hm. First, thanks a lot for the detailed review. I double check on my end and I still don't get the flag as expected. Here are some of my outputs:

$ bin/clang b.c -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal -flto
!llvm.module.flags = !{!0, !1, !2, !3, !4}
!llvm.ident = !{!5}

!0 = !{i32 1, !"NumRegisterParameters", i32 0}
!1 = !{i32 1, !"wchar_size", i32 4}
!2 = !{i32 4, !"cf-protection-branch", i32 1}
!3 = !{i32 7, !"uwtable", i32 1}
!4 = !{i32 7, !"frame-pointer", i32 2}
!5 = !{!"clang version 14.0.0 ([e-mail]:llvm/llvm-project.git 71953e082f449d5357f6ff2548b6c3d94cd6b291)"}

I tested this on top of revision b1856009fbc1fd594e283460a781f2b34fbd7d55, which is the one I used for writing the patch on top of. Would you mind double checking it? Perhaps the problem was fixed in between revisions?

[gftg]

First of all, let me say that I think that your patch is correct. Both the fix itself and the test case. I haven't checked that all options that are parsed in CompilerInvocation::ParseCodeGenArgs have some sort of initialization in CompilerInvocation::GenerateCodeGenArgs. Anyhow, it seems logical that they should, so your patch looks good to me.

Nonetheless, I'm still unable to reproduce the problem, so I'd like to ask you to share the configuration options that you are passing to cmake. Maybe this is causing the difference and I'd like to understand what's going on (even if it's only for my education).

Here's how I configured llvm, just in case:

$ cmake \
      -G Ninja \
      $LLVM_ROOT/llvm \
      -DCMAKE_BUILD_TYPE=Release \
      -DLLVM_ENABLE_PROJECTS="clang;lld" \
      -DLLVM_TARGETS_TO_BUILD="X86" \
      -DCMAKE_INSTALL_PREFIX=$PWD/install \
      -DLLVM_ENABLE_LTO=OFF \
      -DCMAKE_VERBOSE_MAKEFILE=ON \
      -DLLVM_PARALLEL_LINK_JOBS=1

[joaomoreira]

Hi Gabriel, thanks for the updated review. Here are the cmake flags as you asked:

cmake ../llvm-project/llvm -D LLVM_ENABLE_PROJECTS="lld;clang;llvm" -G Ninja -D CMAKE_BUILD_TYPE=Release -D LLVM_ENABLE_ASSERTIONS=ON -D LLVM_ENABLE_LLD=ON -DLLVM_PARALLEL_LINK_JOBS=2 -DLLVM_PARALLEL_COMPILE_JOBS=30 -DCMAKE_INSTALL_PREFIX=../install/

[gftg]

Thanks, João, that's exactly what I needed. For the record, the use of -DLLVM_ENABLE_ASSERTIONS=ON makes it reproducible. I don't understand why, yet, but there you go.

It goes without saying that you still have my LGTM.

Cheers!

[gftg]

I was just reading through https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted and I realized that I might have given the wrong impression that I'm entitled to give "final approval". I'm not, so I'm sending this message to make it clear (I guess you all already know that). Nevertheless, the patch does look good to me (in the English meaning of the sentence, not in the formal patch acceptance process meaning).

[joaomoreira]

ping.

[pengfei]

something changed in the compiler front-end, changing how some flags are processed throughout the pipeline

I'm not familiar enough with front-end. Maybe better to involve people who worked with the change to have a review?

clang/test/CodeGen/X86/x86-cf-protection.c

4

Is -flto is required?

11–12

Can we add another RUN without -mibt-seal amd check no such flags?

[aaron.ballman]

In D118052#3273241, @gftg wrote:

Thanks, João, that's exactly what I needed. For the record, the use of -DLLVM_ENABLE_ASSERTIONS=ON makes it reproducible. I don't understand why, yet, but there you go.

This would be good to track down -- if it is related to enabling assertions, that suggests we have an assert somewhere with an expression in it where we need the side effects, and when assertions are enabled, we're losing those side effects. That makes me worried we may be silencing the real bug with this workaround.

FWIW, I can reproduce the issue in the test case with and without assertions enabled without the changes in CompilerInvocations.cpp, and applying the rest of the patch does fix the issue for me.

The questions from @pengfei are good ones we should address.

[joaomoreira]

clang/test/CodeGen/X86/x86-cf-protection.c

4

Yes, we can only suppress ENDBR if we are sure the given function is not address taken in all other translation units.

11–12

Sure, I'll work on this try to track the possible bug mentioned by @aaron.ballman, then I'll update the diff.

[joaomoreira]

I did track down the problem to clang/lib/Frontend/CompilerInvocation.cpp -- RoundTrip method. There, we can se the following statement:

#ifndef NDEBUG
  bool DoRoundTripDefault = true;
#else
  bool DoRoundTripDefault = false;
#endif

Comment in the file says: "By default, the round-trip is enabled in assert builds... During round-trip, the command line arguments are parsed into a dummy instance of CompilerInvocation which is used to generate the command line arguments again. The real CompilerInvocation instance is then created by parsing the generated arguments, not the original ones.". Then, because the arguments set through this latest patch were not being generated, they would be missing in the dummy instance of CompilerInvocation and then not repassed into the real instance.

Given the above, my understanding is that there was never an actual bug that made ibt-seal stop working. The patch was just not tested enough and a confusion in different build setups blinded me against what was really happening.

Thank you for pushing me into looking into this @aaron.ballman. Also, with all this said, I'm no expert in clang/front-end, so it would be great if you could give an additional look into it and confirm the above.

If all is right, the patch remains correct and there are no hidden bugs. I'll update the patch shortly to add the test requested by @pengfei.

[pengfei]

LGTM.

clang/test/CodeGen/X86/x86-cf-protection.c

6

I think we can use NOIBTSEAL here too.

[aaron.ballman]

In D118052#3401861, @joaomoreira wrote:

I did track down the problem to clang/lib/Frontend/CompilerInvocation.cpp -- RoundTrip method. There, we can se the following statement:

#ifndef NDEBUG
  bool DoRoundTripDefault = true;
#else
  bool DoRoundTripDefault = false;
#endif

Comment in the file says: "By default, the round-trip is enabled in assert builds... During round-trip, the command line arguments are parsed into a dummy instance of CompilerInvocation which is used to generate the command line arguments again. The real CompilerInvocation instance is then created by parsing the generated arguments, not the original ones.". Then, because the arguments set through this latest patch were not being generated, they would be missing in the dummy instance of CompilerInvocation and then not repassed into the real instance.

Given the above, my understanding is that there was never an actual bug that made ibt-seal stop working. The patch was just not tested enough and a confusion in different build setups blinded me against what was really happening.

Thank you for pushing me into looking into this @aaron.ballman. Also, with all this said, I'm no expert in clang/front-end, so it would be great if you could give an additional look into it and confirm the above.

If all is right, the patch remains correct and there are no hidden bugs. I'll update the patch shortly to add the test requested by @pengfei.

That sounds like the correct analysis to me! Thanks for tracking that down.

[nickdesaulniers]

clang/test/CodeGen/X86/x86-cf-protection.c

5–16

Rather than have two different check prefixes with the same value, you could change the above two --check-prefix= lines into --check-prefixes=<unique val>,NOSEAL then use NOSEAL-NOT: "ibt-seal", i32 1.

[xiangzhangllvm]

clang/test/CodeGen/X86/x86-cf-protection.c

4

Sorry, let me make sure here. what is the "translation units" here mean? Does it means another binary file (e.g. *.so , *.a)?
Using -flto seems here want more compile units (source files) to be one "translation units"?

[joaomoreira]

clang/test/CodeGen/X86/x86-cf-protection.c

4

Translation unit means a source file translated into an object file. When compiling the kernel, we have different .c files that are translated into different .o files. Each .c translated into .o is a translation unit. Because a function might not be address taken in the translation unit where it is defined but could be address taken in a different one, we need to emit ENDBRs to all non-local (static) functions. With LTO this changes, because we can look at all to-be-generated objects and be sure that a given function is not address taken in any of the translation units.

This optimization is kernel-specific because in user-space code non-local functions can be reached through the PLT of a different dynamically linked library (.so) or through dlsym, and this is impossible to predict in compilation time.

In kernel, exported symbols are implicitly address taken. This way, if a module tries to take the address of an exported function, this would be ok. The optimization will mostly rule-out non-static functions that are not exported from receiving an ENDBR. The numeric benefits of the optimization are shown in https://reviews.llvm.org/D116070

[joaomoreira]

I think there are no more untied knots... @pengfei, do you think this is ready to merge? If yes, can you please merge it? tks!

[pengfei]

In D118052#3480564, @joaomoreira wrote:

I think there are no more untied knots... @pengfei, do you think this is ready to merge? If yes, can you please merge it? tks!

Sure.

clang/lib/Frontend/CompilerInvocation.cpp

#undef CODEGEN_OPTION_WITH_MARSHALLING
 
   if (Opts.CFProtectionReturn && Opts.CFProtectionBranch)
     GenerateArg(Args, OPT_fcf_protection_EQ, "full", SA);
   else if (Opts.CFProtectionReturn)
     GenerateArg(Args, OPT_fcf_protection_EQ, "return", SA);
   else if (Opts.CFProtectionBranch)
     GenerateArg(Args, OPT_fcf_protection_EQ, "branch", SA);
 
+  if (Opts.IBTSeal)
+    GenerateArg(Args, OPT_mibt_seal, SA);
+
   for (const auto &F : Opts.LinkBitcodeFiles) {
     bool Builtint = F.LinkFlags == llvm::Linker::Flags::LinkOnlyNeeded &&
                     F.PropagateAttrs && F.Internalize;
     GenerateArg(Args,
                 Builtint ? OPT_mlink_builtin_bitcode : OPT_mlink_bitcode_file,
                 F.Filename, SA);
   }
 

clang/test/CodeGen/X86/x86-cf-protection.c

 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=return %s | FileCheck %s --check-prefix=RETURN
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=branch %s | FileCheck %s --check-prefix=BRANCH
 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=full %s   | FileCheck %s --check-prefix=FULL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal -flto %s | FileCheck %s --check-prefixes=CFPROT,IBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -flto %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
+// RUN: %clang -target i386-unknown-unknown -o - -emit-llvm -S -fcf-protection=branch -mibt-seal %s | FileCheck %s --check-prefixes=CFPROT,NOIBTSEAL
 
 // RETURN: #define __CET__ 2
 // BRANCH: #define __CET__ 1
 // FULL: #define __CET__ 3
+// CFPROT: "cf-protection-branch", i32 1
+// IBTSEAL: "ibt-seal", i32 1
+// NOIBTSEAL-NOT: "ibt-seal", i32 1
 void foo() {}