[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

joaomoreira added a comment. Given https://reviews.llvm.org/D140363, this patch is being abandoned. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140035/new/ https://reviews.llvm.org/D140035 ___

[PATCH] D140363: Remove incorrectly implemented -mibt-seal

joaomoreira added a comment. FWIIW, agreed on removing this until we figure out how to make it work properly. Thanks for the patch @MaskRay. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140363/new/ https://reviews.llvm.org/D140363

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

joaomoreira added a comment. In D140035#3998954 , @samitolvanen wrote: > In D140035#3996066 , @joaomoreira > wrote: > >> Regarding not being able to reproduce this in kernel -- never mind... I was >> misled by

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

joaomoreira added a comment. >> Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; >> the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it >> had ~500 less ENDBRs throughout the binary > > Did you confirm the issue with the reproducer in the CBL

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

joaomoreira added a comment. Regarding https://github.com/ClangBuiltLinux/linux/issues/1737: Weirdly enough, I double-tested the behavior for -flto=thin + -mibt-seal; the kernel did boot fine on my setup, but when dumped/grep'ed for ENDBRs, it had ~500 less ENDBRs throughout the binary.

[PATCH] D140035: [X86] Prevent -mibt-seal to work together with -flto=thin

joaomoreira created this revision. joaomoreira added reviewers: samitolvanen, nickdesaulniers, xiangzhangllvm, pengfei, aaron.ballman, pcc, gftg85. joaomoreira added a project: LLVM. Herald added a subscriber: inglorion. Herald added a project: All. joaomoreira requested review of this revision.

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121 +if (N == Value) + return ~Value; + } samitolvanen wrote: > joaomoreira wrote: > > Can we use another constant blinding scheme, such as a Value++ or anything > >

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121 +if (N == Value) + return ~Value; + } Can we use another constant blinding scheme, such as a Value++ or anything else? This way, we would prevent endbrs from

[PATCH] D119296: KCFI sanitizer

joaomoreira accepted this revision. joaomoreira added a comment. I really like this revision. It removes the redundancy of having KCFI passes both in CodeGen and in the backend; it detangles CALL instructions from KCFI by creating a new MIR instruction; it fixes alignment while still supporting

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + samitolvanen wrote: > joaomoreira wrote: > > I did not revise the entire patch yet. With this said, IMHO, this looks > > like an

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + I did not revise the entire patch yet. With this said, IMHO, this looks like an overcomplication of a simple problem. Is there a

[PATCH] D118355: Add -mmanual-endbr switch to allow manual selection of control-flow protection

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86IndirectBranchTracking.cpp:156-161 if (needsPrologueENDBR(MF, M)) { -auto MBB = MF.begin(); -Changed |= addENDBR(*MBB, MBB->begin()); +if (!ManualENDBR || MF.getFunction().doesCfCheck()) { +

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. In D119296#3483176 , @nickdesaulniers wrote: > In D119296#3481573 , @joaomoreira > wrote: > >> I'm not an expert on LLVM's pipeline, but it just feels a little awkward and >>

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. > I looked at your code quickly and I wonder if using operand bundles would be > better than adding an attribute. Thoughts? Perhaps @pcc can provide a better feedback on the hassle (or joys) behind using operand bundles. My insights on the attribute is that it is

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. > I agree that a separate pass wasn't ideal, but InstCombine seems to be full > of code to "fix what other passes messed up". :) I'm not sure if messed up > is the correct term though, these are checks that were necessary before > optimizations, but are no longer

[PATCH] D119296: KCFI sanitizer

joaomoreira added a subscriber: craig.topper. joaomoreira added a comment. > This seems like a reasonable approach, and was also the approach taken for > the PAuth ABI. The PAuth ABI attaches an operand bundle to the call > instruction and arranges for the code for the check to be generated

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

joaomoreira added a comment. 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! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118052/new/ https://reviews.llvm.org/D118052

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. Oh, one other tiny detail I forgot to mention. I noticed that the tag is pushing the functions 6 bytes forward, regardless of any prepending padding nops that were added to ensure 16b alignment. It would be cool to care about the proper function alignment and also

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. I played a little bit with kcfi and here are some thoughts: - under -Os I saw functions being inlined, regardless of the source code calling them indirectly. In these scenarios, the KCFI check was still in place, even though there was not a pointer involved in the

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

joaomoreira updated this revision to Diff 423775. This revision is now accepted and ready to land. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118052/new/ https://reviews.llvm.org/D118052 Files: clang/lib/Frontend/CompilerInvocation.cpp clang/test/CodeGen/X86/x86-cf-protection.c

[PATCH] D122673: Add kcfi_unchecked attribute

joaomoreira added a comment. > In the previous discussion, @joaomoreira pointed out that this is very > similar to `nocf_check` and proposed reusing that attribute. In an offline > discussion, @pcc was concerned that an attribute may not be the right > approach here and suggested a

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

joaomoreira added inline comments. Comment at: clang/test/CodeGen/X86/x86-cf-protection.c:4 // 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

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

joaomoreira updated this revision to Diff 417539. joaomoreira edited the summary of this revision. joaomoreira added a reviewer: nickdesaulniers. This revision is now accepted and ready to land. Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

joaomoreira added a comment. 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

[PATCH] D118052: [X86] Fix CodeGen Module Flag for -mibt-seal

joaomoreira planned changes to this revision. joaomoreira added inline comments. Herald added a project: All. Comment at: clang/test/CodeGen/X86/x86-cf-protection.c:4 // RUN: %clang -target i386-unknown-unknown -x c -E -dM -o - -fcf-protection=full %s | FileCheck %s

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: clang/include/clang/Basic/Attr.td:696 +def KCFIUnchecked : Attr { + let Spellings = [Clang<"kcfi_unchecked">]; + let Subjects = SubjectList<[Var, TypedefName]>; Are you considering that perhaps one could use KCFI

[PATCH] D87822: [FPEnv] Evaluate constant expressions under non-default rounding modes

joaomoreira added a comment. I noticed that this commit breaks MUSL 1.2.0. Here is an isolated test-case that illustrates the issue: https://godbolt.org/z/6bdnEh Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87822/new/