[PATCH] D112768: [ARM] implement support for TLS register based stack protector

[Ard Biesheuvel via Phabricator via cfe-commits]

ardb added a comment.

I have split off the LOAD_STACK_GUARD changes into

  [ARM] implement LOAD_STACK_GUARD for remaining targets



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:4900
+        .addImm(15)
+        .addImm(0)
+        .addImm(13)
----------------
nickdesaulniers wrote:
> I think we want to use `Reg` in this instruction, in order to load into the specified destination?
We use Reg, no? MRC does not take reg operands, but I pass it as the target register.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:4904
+        .addImm(3)
+        .add(predOps(ARMCC::AL));
 
----------------
nickdesaulniers wrote:
> do we need to add `al`?
It appears so.


================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.cpp:102
+  if (M.getStackProtectorGuard() == "tls") {
+    expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+    return;
----------------
nickdesaulniers wrote:
> do we have to worry about soft tp, ie. `__aeabi_read_tp` vs `mrc`?
I think we should only allow this hard TP is supported in the first place. I'll add a check somewhere.
Calling a helper in both the prologue and the epilogue for emulated TLS seems a bit too heavyweight to me, and the Linux kernel will not use it in that way anyway.


================
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:250
 
 void Thumb2InstrInfo::expandLoadStackGuard(
     MachineBasicBlock::iterator MI) const {
----------------
nickdesaulniers wrote:
> what about `Thumb1InstrInfo::expandLoadStackGuard`? Do we have `mrc` available in Thumb[1] as well?
No Thumb1 does not have an encoding for MRC so we can ignore it here. It does mean we should not allow this feature to be turned out for such targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112768