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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Codegen looks good, probably just need an additional conditional on `ARMSubtarget::isReadTPHard()`. With that and some more tests for cases we don't intend to support (thumb[1], soft tp) this LGTM. Great job @ardb !



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:4900
+        .addImm(15)
+        .addImm(0)
+        .addImm(13)
----------------
ardb wrote:
> 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.
ah, yep, sorry, LGTM.


================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.cpp:102
+  if (M.getStackProtectorGuard() == "tls") {
+    expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+    return;
----------------
ardb wrote:
> 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.
I agree. Let's add a test that this only works with the `-mattr=+read-tp-hard` llc flag or `"target-features"="+read-tp-hard"` function attribute (one or the other, I don't think we need to exhaustively test both). But it would be good to verify what happens when `+read-tp-hard` is not set, in case someone else cares to add soft tp support here in the future.  (They may never, it's more so to highlight whether such a change would be intentional).

`ARMSubtarget::isReadTPHard()` should be able to help us differentiate this case.


================
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:250
 
 void Thumb2InstrInfo::expandLoadStackGuard(
     MachineBasicBlock::iterator MI) const {
----------------
ardb wrote:
> 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.
Let's add a test for such case.


================
Comment at: llvm/test/CodeGen/ARM/stack-guard-tls.ll:5-11
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-SMALL %s
+; RUN: llc %t/a2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-SMALL %s
+; RUN: llc %t/b2.ll -mtriple=armv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-LARGE %s
+; RUN: llc %t/b2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefix=CHECK --check-prefix=CHECK-LARGE %s
----------------
You can convert `--check-prefix=CHECK --check-prefix=CHECK-SMALL` to `--check-prefixes=CHECK,CHECK-SMALL`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112768