[PATCH] D140655: [LowerTypeTests] Add ENDBR to .cfi.jumptable for x86 Indirect Branch Tracking

Wed Jan 4 08:16:44 PST 2023

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm with comment or change suggested below.

================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1416
+  if (JumpTableArch == Triple::x86 || JumpTableArch == Triple::x86_64)
+    F->addFnAttr(Attribute::NoCfCheck);
   // Make sure we don't emit .eh_frame for this function.
----------------
MaskRay wrote:
> tejohnson wrote:
> > Add comment about why this attribute is needed. I see that the attribute is only checked for the x86 backends. But can be queried on the function for any arch - is there any possibility this feature will be implemented for non-x86 in the future? If so, I suggest adding the attribute unconditionally here.
> > 
> > Also, is this needed in the non-endbr case for x86? I.e. it is being added here for all x86 which presumably also changes the pre-existing non-endbr x86 handling.
> This is similar to ` F->addFnAttr("branch-target-enforcement", "false");` for AArch64 BTI (D81251). Without this attribute, the function will have 2 consecutive endbr64 at the function start (one from the BTI instrumentation, the other from the first instruction of the inline asm instruction).
Please add a comment about why it is needed and that it does not have any effect in the case where CF branch protection is not enabled (alternatively, just guard this with whether the cf-protection-branch module flag is set).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140655/new/

https://reviews.llvm.org/D140655