Page MenuHomePhabricator

[AArch64] Remove implicit landing pads.
Changes PlannedPublic

Authored by danielkiss on Apr 6 2020, 9:15 AM.

Details

Summary

PACIASP and PACIA LR, SP are equivalent from PAC point of view while PACIA is not a BTI landing pad.
Direct branch does not need a landing pad. If PACIA is available from v8.3, the unnecessary landing pads could be eliminated.
Another patch will improve the detection of the direct only calls.

Diff Detail

Event Timeline

danielkiss created this revision.Apr 6 2020, 9:15 AM
chill added inline comments.Apr 6 2020, 11:21 AM
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
216

Suggest using an explicit condition, to spare the reader back-inferencing what conditions could set HintNum to 32.

221

Why not create the PACI[AB] from the very start, with a comment why are we doing it?

danielkiss updated this revision to Diff 256025.Apr 8 2020, 7:54 AM
danielkiss marked 2 inline comments as done.
chill added inline comments.Apr 9 2020, 1:04 AM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
940 ↗(On Diff #256025)

Why not create the PACI[AB] from the very start, with a comment why are we doing it?

Sorry, I meant do say, create in the prologue:

  • PACIA if we do know the function should/can not be called indirectly and "branch-target-enforcement" attribute is present (if it's absent, it doesn't matter which insn we emit, right?)
  • PACIASP otherwise

Then this BTI placrement pass could just check

if (MBBI->getOpcode() == AArch64::PACIASP ||
  MBBI->getOpcode() == AArch64::PACIBSP ||
  MBBI->getOpcode() == AArch64::PACIA ||
   MBBI->getOpcode() == AArch64::PACIB) 
  return false;

knowing that what needs to be done has already been done.

941 ↗(On Diff #256025)

Could this be refactored in a separate function and shared with the similar code that signs outlined functions?

danielkiss planned changes to this revision.Jun 18 2020, 6:45 AM

I'm going to refactor this and rebase on my other changes.

Simplify the patch, rebased.

danielkiss marked 3 inline comments as done.Jun 22 2020, 7:47 AM
danielkiss added inline comments.
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
221

Just replacing them here is a why more simpler code.
I won't change the generated code if BTI is not on.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
941 ↗(On Diff #256025)

I'm going to create a new patch that removes the code duplication.

danielkiss marked an inline comment as done.
chill added inline comments.Aug 4 2020, 3:54 AM
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
221

I don't find that simpler or logical at all. We have a function called addBTI and it *removes" a BTI?
The only two things that make sense are:

  • create the right instruction from the very start, we have al the necessary information to do so, it makes sense doing the right thing on the spot, instead of patching it later, which in addition to being harder to understand (you have to have knowledge and be aware of multiple parts of the code in order to figure out what's at the beginning of a function and also has time/space overheads, or
  • create the non-BTI instruction and the add the BTI property in the pass that adds the BTIs
danielkiss planned changes to this revision.Sep 11 2020, 8:53 AM

I'll update this after other PAC/BTI patches got merged.