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

[Ard Biesheuvel via Phabricator via cfe-commits]

ardb added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.cpp:102
+  if (M.getStackProtectorGuard() == "tls") {
+    expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+    return;
----------------
nickdesaulniers wrote:
> 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.
> 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.

I could not figure out how to check this at command line parsing time. If we check it later, we basically crash the compiler, right? Not very elegant ...




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

Same problem: I could not figure out how to decide early enough whether we are targetting Thumb1 or Thumb2/ARM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112768