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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Nice job! This looks like it hits every place that I was looking at for this.

In terms of tests, https://reviews.llvm.org/D100919 and https://reviews.llvm.org/D102742 are probably interesting.

In particular, we should test that clang no longer rejects `-mstack-protector-guard=tls -mstack-protector-guard-offset=0` for `--target=arm-linux-gnueabihf` and `--target=arm-linux-gnueabihf -mthumb`. (clang/test/Driver/stack-protector-guard.c
)

Then we should test

  target triple = "arm-unknown-linux-gnueabihf"
  
  declare void @baz(i32*)
  
  define void @foo(i64 %t) sspstrong {
    %vla = alloca i32, i64 %t, align 4
    call void @baz(i32* nonnull %vla)
    ret void
  }
  !llvm.module.flags = !{!1, !2}
  !1 = !{i32 2, !"stack-protector-guard", !"tls"}
  !2 = !{i32 2, !"stack-protector-guard-offset", !"0"}

generates `mrc` (and test again with "thumbv7-linux-gnu"). We can use `-mtriple=` flags to `llc` rather than hard coding the triple in IR.  (create llvm/test/CodeGen/ARM/stack-guard-sysreg.ll)

> This also involves implementing LOAD_STACK_GUARD for other ARM targets than
> Mach-O.

It might be worthwhile to break this up into 2 commits; one that does just that, so that we can isolate any test changes or possible build breakages to that, then have this patch implementing support for tls stack guards on top.



================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:4900
+        .addImm(15)
+        .addImm(0)
+        .addImm(13)
----------------
I think we want to use `Reg` in this instruction, in order to load into the specified destination?


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


================
Comment at: llvm/lib/Target/ARM/ARMInstrInfo.cpp:102
+  if (M.getStackProtectorGuard() == "tls") {
+    expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+    return;
----------------
do we have to worry about soft tp, ie. `__aeabi_read_tp` vs `mrc`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112768