Page MenuHomePhabricator

[AArch64] Fix BTI instruction emission.
ClosedPublic

Authored by danielkiss on Jun 12 2020, 9:22 AM.

Details

Summary

SCTLR_EL1.BT[01] controls the PACI[AB]SP compatibility with PBYTE 11
(see [1])
This bit will be set to zero so PACI[AB]SP are equal to BTI C
instruction only.

[1] https://developer.arm.com/docs/ddi0595/b/aarch64-system-registers/sctlr_el1

Diff Detail

Event Timeline

danielkiss created this revision.Jun 12 2020, 9:22 AM
This revision is now accepted and ready to land.Jun 15 2020, 4:11 AM

Patch rebased, now should apply clearly.

This revision was automatically updated to reflect the committed changes.
chill added inline comments.Jun 15 2020, 7:16 AM
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
125

What if they are set to a different value?

chill added inline comments.Jun 15 2020, 7:21 AM
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
125

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() ?

nickdesaulniers added inline comments.
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
125

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

danielkiss marked an inline comment as done.Jun 18 2020, 8:39 AM
danielkiss added inline comments.
llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
125

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.