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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to