[PATCH] D81746: [AArch64] Fix BTI instruction emission.

Thu Jun 18 08:40:54 PDT 2020

danielkiss marked an inline comment as done.
danielkiss added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64BranchTargets.cpp:125

-  // PACI[AB]SP are implicitly BTI JC, so no BTI instruction needed there.
-  if (MBBI != MBB.end() && (MBBI->getOpcode() == AArch64::PACIASP ||
-                            MBBI->getOpcode() == AArch64::PACIBSP))
+  // SCTLR_EL1.BT[01] is set to 0 by default which means
+  // PACI[AB]SP are implicitly BTI C so no BTI C instruction is needed there.
----------------
nickdesaulniers wrote:
> chill wrote:
> > chill wrote:
> > > What if they are set to a different value?
> > Sorry for commenting at this late stage, I didn't know that behavior was configurable.
> > Shouldn't there be a test `isTargetAndroid()` and/or `isTargetLinux()` ?
> At least for fixing the reported bug in Linux kernels, we shouldn't guard on `isTargetAndroid`; Linux kernels used in Android target aarch64-linux-gnu triples. (Apologies if I misunderstood your question, or if you knew that already).
> 
> Re: https://groups.google.com/g/clang-built-linux/c/t2cgw_2fA2w
If the SCTLR_EL1.BT[01] is set it won't break the generated code, it will allow to jump to PACIASP/PACIBSP too with BR with not X16 or X17. This is not an issue here because we anyway adding a landing pad. It is issue only where PACIASP present but we shouldn't jump there, therefore this setting is not expected to be used widely.
Kernel and Userspace could have different setting.
If there will be a platfrom that enforces the SCTLR_EL1.BT to be set then we need a flag here to save a few additional landing pads. I'm not aware of such an ABI.
This bug found in kernel and user space too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81746