[PATCH] D112768: [ARM] implement support for TLS register based stack protector
This revision was automatically updated to reflect the committed changes.
ardb marked an inline comment as done.
Closed by commit rGa19da876ab93: [ARM] implement support for TLS register based
stack protector (authored by ardb).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112768/new/
https://reviews.llvm.org/D112768
Files:
clang/include/clang/Basic/DiagnosticCommonKinds.td
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/stack-protector-guard.c
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMInstrInfo.cpp
llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
llvm/test/CodeGen/ARM/stack-guard-tls.ll
Index: llvm/test/CodeGen/ARM/stack-guard-tls.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/stack-guard-tls.ll
@@ -0,0 +1,38 @@
+; RUN: split-file %s %t
+; RUN: cat %t/main.ll %t/a.ll > %t/a2.ll
+; RUN: cat %t/main.ll %t/b.ll > %t/b2.ll
+; RUN: llc %t/a2.ll -mtriple=armv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/a2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/b2.ll -mtriple=armv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+; RUN: llc %t/b2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+
+;--- main.ll
+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"}
+
+;--- a.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 1296}
+
+;--- b.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 4296}
+
+; CHECK: mrc p15, #0, [[REG1:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG1]], [[REG1]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #200]
+; CHECK: bl baz
+; CHECK: mrc p15, #0, [[REG2:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG2]], [[REG2]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #200]
Index: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -250,6 +250,13 @@
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+return;
+ }
+
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
@@ -135,6 +135,11 @@
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ assert(M.getStackProtectorGuard() != "tls" &&
+ "TLS stack protector not supported for Thumb1 targets");
+
if (TM.isPositionIndependent())
expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::tLDRi);
else
Index: llvm/lib/Target/ARM/ARMInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMInstrInfo.cpp
@@ -95,6 +95,12 @@
MachineFunction = *MI->getParent()->getParent();
const ARMSubtarget = MF.getSubtarget();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+return;
+ }
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -4890,40 +4890,70 @@
MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
- const GlobalValue *GV =
- cast((*MI->memoperands_begin())->getValue());
- bool IsIndirect = Subtarget.isGVIndirectSymbol(GV);
MachineInstrBuilder MIB;
+ unsigned int Offset = 0;
-
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land. Looks great! Nice job! 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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb marked 2 inline comments as done.
ardb added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3190-3191
+ }
+ CmdArgs.push_back("-target-feature");
+ CmdArgs.push_back("+read-tp-hard");
+}
nickdesaulniers wrote:
> ardb wrote:
> > nickdesaulniers wrote:
> > > Isn't this redundant/set elsewhere?
> > No, it is not. The alternative is requiring the user to pass -mtp=cp15 when
> > using the TLS stack protector, which is silly because it is implied.
> Right. Perhaps we could emit `diag::err_drv_argument_not_allowed_with` if the
> user specified `-mtp=soft` with `-mstack-protector-guard=tls`.
>
> It looks like a call to clang::driver::tools::arm::getReadTPMode() should
> being able to tell us whether someone explicitly tried to use soft.
> Right. Perhaps we could emit `diag::err_drv_argument_not_allowed_with` if the
> user specified `-mtp=soft` with `-mstack-protector-guard=tls`.
>
> It looks like a call to clang::driver::tools::arm::getReadTPMode() should
> being able to tell us whether someone explicitly tried to use soft.
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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb updated this revision to Diff 384564.
ardb added a comment.
- disallow -mtp=soft when TLS based stack protector is enabled
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112768/new/
https://reviews.llvm.org/D112768
Files:
clang/include/clang/Basic/DiagnosticCommonKinds.td
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/stack-protector-guard.c
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMInstrInfo.cpp
llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
llvm/test/CodeGen/ARM/stack-guard-tls.ll
Index: llvm/test/CodeGen/ARM/stack-guard-tls.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/stack-guard-tls.ll
@@ -0,0 +1,38 @@
+; RUN: split-file %s %t
+; RUN: cat %t/main.ll %t/a.ll > %t/a2.ll
+; RUN: cat %t/main.ll %t/b.ll > %t/b2.ll
+; RUN: llc %t/a2.ll -mtriple=armv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/a2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/b2.ll -mtriple=armv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+; RUN: llc %t/b2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+
+;--- main.ll
+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"}
+
+;--- a.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 1296}
+
+;--- b.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 4296}
+
+; CHECK: mrc p15, #0, [[REG1:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG1]], [[REG1]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #200]
+; CHECK: bl baz
+; CHECK: mrc p15, #0, [[REG2:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG2]], [[REG2]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #200]
Index: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -250,6 +250,13 @@
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+return;
+ }
+
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
@@ -135,6 +135,11 @@
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ assert(M.getStackProtectorGuard() != "tls" &&
+ "TLS stack protector not supported for Thumb1 targets");
+
if (TM.isPositionIndependent())
expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::tLDRi);
else
Index: llvm/lib/Target/ARM/ARMInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMInstrInfo.cpp
@@ -95,6 +95,12 @@
MachineFunction = *MI->getParent()->getParent();
const ARMSubtarget = MF.getSubtarget();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+return;
+ }
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -4891,40 +4891,70 @@
MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
- const GlobalValue *GV =
- cast((*MI->memoperands_begin())->getValue());
- bool IsIndirect = Subtarget.isGVIndirectSymbol(GV);
MachineInstrBuilder MIB;
+ unsigned int Offset = 0;
- unsigned TargetFlags = ARMII::MO_NO_FLAG;
- if (Subtarget.isTargetMachO()) {
-TargetFlags |=
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
nickdesaulniers added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3190-3191
+ }
+ CmdArgs.push_back("-target-feature");
+ CmdArgs.push_back("+read-tp-hard");
+}
ardb wrote:
> nickdesaulniers wrote:
> > Isn't this redundant/set elsewhere?
> No, it is not. The alternative is requiring the user to pass -mtp=cp15 when
> using the TLS stack protector, which is silly because it is implied.
Right. Perhaps we could emit `diag::err_drv_argument_not_allowed_with` if the
user specified `-mtp=soft` with `-mstack-protector-guard=tls`.
It looks like a call to clang::driver::tools::arm::getReadTPMode() should being
able to tell us whether someone explicitly tried to use soft.
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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3177-3179
+ if (!Args.hasArg(options::OPT_mstack_protector_guard_offset_EQ)) {
+D.Diag(diag::err_drv_ssp_missing_offset_argument)
+<< A->getOption().getName() << Value;
nickdesaulniers wrote:
> Does GCC require these flags to be paired (`-mstack-protector-guard=` and
> `-mstack-protector-guard-offset`) ? Only for ARM and THUMB? Doesn't the
> offset only make sense for sysreg and tls, or global, too?
>
> Seems to me like `0` would be a good default offset, if unspecified.
>
> Perhaps this would good to check then warn on for //all// supported
> architectures, and perhaps as another child patch? (or just default to `0`
> and not require `-mstack-protector-offset=`).
> Does GCC require these flags to be paired (`-mstack-protector-guard=` and
> `-mstack-protector-guard-offset`) ? Only for ARM and THUMB? Doesn't the
> offset only make sense for sysreg and tls, or global, too?
>
GCC does not implement this for ARM yet. For AArch64, it requires all three
options: guard, guard-reg and guard-offset.
> Seems to me like `0` would be a good default offset, if unspecified.
>
The default is INT_MAX at the moment, and I wasn't sure if we can simply change
that without breaking users.
> Perhaps this would good to check then warn on for //all// supported
> architectures, and perhaps as another child patch? (or just default to `0`
> and not require `-mstack-protector-offset=`).
Yes, I think it makes sense to check this for AAch64 as well.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3190-3191
+ }
+ CmdArgs.push_back("-target-feature");
+ CmdArgs.push_back("+read-tp-hard");
+}
nickdesaulniers wrote:
> Isn't this redundant/set elsewhere?
No, it is not. The alternative is requiring the user to pass -mtp=cp15 when
using the TLS stack protector, which is silly because it is implied.
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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
nickdesaulniers added inline comments.
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3177-3179
+ if (!Args.hasArg(options::OPT_mstack_protector_guard_offset_EQ)) {
+D.Diag(diag::err_drv_ssp_missing_offset_argument)
+<< A->getOption().getName() << Value;
Does GCC require these flags to be paired (`-mstack-protector-guard=` and
`-mstack-protector-guard-offset`) ? Only for ARM and THUMB? Doesn't the offset
only make sense for sysreg and tls, or global, too?
Seems to me like `0` would be a good default offset, if unspecified.
Perhaps this would good to check then warn on for //all// supported
architectures, and perhaps as another child patch? (or just default to `0` and
not require `-mstack-protector-offset=`).
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3190-3191
+ }
+ CmdArgs.push_back("-target-feature");
+ CmdArgs.push_back("+read-tp-hard");
+}
Isn't this redundant/set elsewhere?
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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb updated this revision to Diff 384154.
ardb added a comment.
- fix failure in newly added LLVM test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112768/new/
https://reviews.llvm.org/D112768
Files:
clang/include/clang/Basic/DiagnosticCommonKinds.td
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/stack-protector-guard.c
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMInstrInfo.cpp
llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
llvm/test/CodeGen/ARM/stack-guard-tls.ll
Index: llvm/test/CodeGen/ARM/stack-guard-tls.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/stack-guard-tls.ll
@@ -0,0 +1,38 @@
+; RUN: split-file %s %t
+; RUN: cat %t/main.ll %t/a.ll > %t/a2.ll
+; RUN: cat %t/main.ll %t/b.ll > %t/b2.ll
+; RUN: llc %t/a2.ll -mtriple=armv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/a2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/b2.ll -mtriple=armv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+; RUN: llc %t/b2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -mattr=+read-tp-hard -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+
+;--- main.ll
+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"}
+
+;--- a.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 1296}
+
+;--- b.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 4296}
+
+; CHECK: mrc p15, #0, [[REG1:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG1]], [[REG1]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #200]
+; CHECK: bl baz
+; CHECK: mrc p15, #0, [[REG2:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG2]], [[REG2]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #200]
Index: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -250,6 +250,13 @@
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+return;
+ }
+
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
@@ -135,6 +135,11 @@
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ assert(M.getStackProtectorGuard() != "tls" &&
+ "TLS stack protector not supported for Thumb1 targets");
+
if (TM.isPositionIndependent())
expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::tLDRi);
else
Index: llvm/lib/Target/ARM/ARMInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMInstrInfo.cpp
@@ -95,6 +95,12 @@
MachineFunction = *MI->getParent()->getParent();
const ARMSubtarget = MF.getSubtarget();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+return;
+ }
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -4891,40 +4891,70 @@
MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
- const GlobalValue *GV =
- cast((*MI->memoperands_begin())->getValue());
- bool IsIndirect = Subtarget.isGVIndirectSymbol(GV);
MachineInstrBuilder MIB;
+ unsigned int Offset = 0;
- unsigned TargetFlags = ARMII::MO_NO_FLAG;
- if (Subtarget.isTargetMachO()) {
-TargetFlags |= ARMII::MO_NONLAZY;
- } else
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb updated this revision to Diff 384116.
ardb added a comment.
- add diagnostics to the frontend and asserts to the backend to ensure that the
TLS stack protector is only used on target subarchs that implement the hardware
TLS register to begin with
- ensure that the offset parameter is not omitted, as the default is INT_MAX
which is out of bounds
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112768/new/
https://reviews.llvm.org/D112768
Files:
clang/include/clang/Basic/DiagnosticCommonKinds.td
clang/include/clang/Basic/DiagnosticDriverKinds.td
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/stack-protector-guard.c
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMInstrInfo.cpp
llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
llvm/test/CodeGen/ARM/stack-guard-tls.ll
Index: llvm/test/CodeGen/ARM/stack-guard-tls.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/stack-guard-tls.ll
@@ -0,0 +1,38 @@
+; RUN: split-file %s %t
+; RUN: cat %t/main.ll %t/a.ll > %t/a2.ll
+; RUN: cat %t/main.ll %t/b.ll > %t/b2.ll
+; RUN: llc %t/a2.ll -mtriple=armv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/a2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-SMALL %s
+; RUN: llc %t/b2.ll -mtriple=armv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+; RUN: llc %t/b2.ll -mtriple=thumbv7-unknown-linux-gnueabihf -o - | \
+; RUN: FileCheck --check-prefixes=CHECK,CHECK-LARGE %s
+
+;--- main.ll
+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"}
+
+;--- a.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 1296}
+
+;--- b.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 4296}
+
+; CHECK: mrc p15, #0, [[REG1:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG1]], [[REG1]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #200]
+; CHECK: bl baz
+; CHECK: mrc p15, #0, [[REG2:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG2]], [[REG2]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #200]
Index: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -250,6 +250,13 @@
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+return;
+ }
+
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb1InstrInfo.cpp
@@ -135,6 +135,11 @@
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ assert(M.getStackProtectorGuard() != "tls" &&
+ "TLS stack protector not supported for Thumb1 targets");
+
if (TM.isPositionIndependent())
expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::tLDRi);
else
Index: llvm/lib/Target/ARM/ARMInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMInstrInfo.cpp
@@ -95,6 +95,12 @@
MachineFunction = *MI->getParent()->getParent();
const ARMSubtarget = MF.getSubtarget();
const TargetMachine = MF.getTarget();
+ Module = *MF.getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+return;
+ }
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -4891,40 +4891,70 @@
MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
- const GlobalValue *GV =
- cast((*MI->memoperands_begin())->getValue());
- bool IsIndirect = Subtarget.isGVIndirectSymbol(GV);
MachineInstrBuilder
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
peter.smith added a reviewer: ostannard. peter.smith added a comment. Adding ostannard to reviewers list. I'm going to be on vacation next week and Oliver is more familiar with this area than I am. To prevent the option in Clang for targets that don't support Thumb2 it may be worth looking into clang/lib/Basic/Targets/ARM.cpp for example https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L174 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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb updated this revision to Diff 383390.
ardb edited the summary of this revision.
ardb added a comment.
- split off LOAD_STACK_GUARD conversion
- deal with guard offsets >= 4096 bytes
- reject offsets < 0 or >= 1 MiB
- add backend test to check that the MRC/LDR sequence is emitted twice
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112768/new/
https://reviews.llvm.org/D112768
Files:
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/stack-protector-guard.c
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMInstrInfo.cpp
llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
llvm/test/CodeGen/ARM/stack-guard-tls.ll
Index: llvm/test/CodeGen/ARM/stack-guard-tls.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/stack-guard-tls.ll
@@ -0,0 +1,38 @@
+; RUN: split-file %s %t
+; RUN: cat %t/main.ll %t/a.ll > %t/a2.ll
+; RUN: cat %t/main.ll %t/b.ll > %t/b2.ll
+; RUN: llc %t/a2.ll -mtriple=armv7-unknown-linux-gnueabihf -o - | \
+; 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
+
+;--- main.ll
+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"}
+
+;--- a.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 1296}
+
+;--- b.ll
+!2 = !{i32 2, !"stack-protector-guard-offset", i32 4296}
+
+; CHECK: mrc p15, #0, [[REG1:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG1]], [[REG1]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG1]], {{\[}}[[REG1]], #200]
+; CHECK: bl baz
+; CHECK: mrc p15, #0, [[REG2:r[0-9]+]], c13, c0, #3
+; CHECK-SMALL-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #1296]
+; CHECK-LARGE-NEXT: add{{(\.w)?}} [[REG2]], [[REG2]], #4096
+; CHECK-LARGE-NEXT: ldr{{(\.w)?}} [[REG2]], {{\[}}[[REG2]], #200]
Index: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -250,6 +250,14 @@
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
+ MachineBasicBlock = *MI->getParent();
+ Module = *MBB.getParent()->getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+return;
+ }
+
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
Index: llvm/lib/Target/ARM/ARMInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMInstrInfo.cpp
@@ -95,6 +95,13 @@
MachineFunction = *MI->getParent()->getParent();
const ARMSubtarget = MF.getSubtarget();
const TargetMachine = MF.getTarget();
+ MachineBasicBlock = *MI->getParent();
+ Module = *MBB.getParent()->getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+return;
+ }
const GlobalValue *GV =
cast((*MI->memoperands_begin())->getValue());
@@ -117,7 +124,6 @@
return;
}
- MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
MachineInstrBuilder MIB;
Index: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -4891,40 +4891,67 @@
MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
- const GlobalValue *GV =
- cast((*MI->memoperands_begin())->getValue());
- bool IsIndirect = Subtarget.isGVIndirectSymbol(GV);
MachineInstrBuilder MIB;
+ unsigned int Offset = 0;
- unsigned TargetFlags = ARMII::MO_NO_FLAG;
- if (Subtarget.isTargetMachO()) {
-TargetFlags |= ARMII::MO_NONLAZY;
- } else if (Subtarget.isTargetCOFF()) {
-if (GV->hasDLLImportStorageClass())
- TargetFlags |= ARMII::MO_DLLIMPORT;
-else if (IsIndirect)
- TargetFlags |= ARMII::MO_COFFSTUB;
- } else if (Subtarget.isGVInGOT(GV)) {
-TargetFlags |= ARMII::MO_GOT;
- }
+ if (LoadImmOpc == ARM::MRC || LoadImmOpc == ARM::t2MRC) {
+
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
nickdesaulniers added inline comments.
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:250
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
what about `Thumb1InstrInfo::expandLoadStackGuard`? Do we have `mrc` available
in Thumb[1] as well?
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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
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
[PATCH] D112768: [ARM] implement support for TLS register based stack protector
ardb created this revision.
ardb added reviewers: nickdesaulniers, peter.smith, nathanchance, kees.
Herald added subscribers: hiraditya, kristof.beyls.
ardb requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
Implement support for loading the stack canary from a memory location held in
the TLS register, with an optional offset applied. This is used by the Linux
kernel to implement per-task stack canaries, which is impossible on SMP systems
when using a global variable for the stack canary.
This also involves implementing LOAD_STACK_GUARD for other ARM targets than
Mach-O.
Suggestions for how to test this and where to insert the test code into the
tree are kindly welcomed.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D112768
Files:
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/stack-protector-guard.c
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
llvm/lib/Target/ARM/ARMISelLowering.cpp
llvm/lib/Target/ARM/ARMInstrInfo.cpp
llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
Index: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
===
--- llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -250,7 +250,20 @@
void Thumb2InstrInfo::expandLoadStackGuard(
MachineBasicBlock::iterator MI) const {
MachineFunction = *MI->getParent()->getParent();
- if (MF.getTarget().isPositionIndependent())
+ MachineBasicBlock = *MI->getParent();
+ Module = *MBB.getParent()->getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::t2MRC, ARM::t2LDRi12);
+return;
+ }
+
+ const GlobalValue *GV =
+cast((*MI->memoperands_begin())->getValue());
+
+ if (MF.getSubtarget().isGVInGOT(GV))
+expandLoadStackGuardBase(MI, ARM::tLDRLIT_ga_pcrel, ARM::t2LDRi12);
+ else if (MF.getTarget().isPositionIndependent())
expandLoadStackGuardBase(MI, ARM::t2MOV_ga_pcrel, ARM::t2LDRi12);
else
expandLoadStackGuardBase(MI, ARM::t2MOVi32imm, ARM::t2LDRi12);
Index: llvm/lib/Target/ARM/ARMInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMInstrInfo.cpp
@@ -95,8 +95,18 @@
MachineFunction = *MI->getParent()->getParent();
const ARMSubtarget = MF.getSubtarget();
const TargetMachine = MF.getTarget();
+ MachineBasicBlock = *MI->getParent();
+ Module = *MBB.getParent()->getFunction().getParent();
+
+ if (M.getStackProtectorGuard() == "tls") {
+expandLoadStackGuardBase(MI, ARM::MRC, ARM::LDRi12);
+return;
+ }
- if (!Subtarget.useMovt()) {
+ const GlobalValue *GV =
+ cast((*MI->memoperands_begin())->getValue());
+
+ if (!Subtarget.useMovt() || Subtarget.isGVInGOT(GV)) {
if (TM.isPositionIndependent())
expandLoadStackGuardBase(MI, ARM::LDRLIT_ga_pcrel, ARM::LDRi12);
else
@@ -109,15 +119,11 @@
return;
}
- const GlobalValue *GV =
- cast((*MI->memoperands_begin())->getValue());
-
if (!Subtarget.isGVIndirectSymbol(GV)) {
expandLoadStackGuardBase(MI, ARM::MOV_ga_pcrel, ARM::LDRi12);
return;
}
- MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg = MI->getOperand(0).getReg();
MachineInstrBuilder MIB;
Index: llvm/lib/Target/ARM/ARMISelLowering.cpp
===
--- llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ -20687,9 +20687,8 @@
return InsertFencesForAtomic;
}
-// This has so far only been implemented for MachO.
bool ARMTargetLowering::useLoadStackGuardNode() const {
- return Subtarget->isTargetMachO();
+ return true;
}
void ARMTargetLowering::insertSSPDeclarations(Module ) const {
Index: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
===
--- llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -1682,8 +1682,6 @@
bool ARMBaseInstrInfo::expandPostRAPseudo(MachineInstr ) const {
if (MI.getOpcode() == TargetOpcode::LOAD_STACK_GUARD) {
-assert(getSubtarget().getTargetTriple().isOSBinFormatMachO() &&
- "LOAD_STACK_GUARD currently supported only for MachO.");
expandLoadStackGuard(MI);
MI.getParent()->erase(MI);
return true;
@@ -4884,8 +4882,6 @@
return true;
}
-// LoadStackGuard has so far only been implemented for MachO. Different code
-// sequence is needed for other targets.
void ARMBaseInstrInfo::expandLoadStackGuardBase(MachineBasicBlock::iterator MI,
unsigned LoadImmOpc,
unsigned LoadOpc) const {
@@ -4895,27 +4891,48 @@
MachineBasicBlock = *MI->getParent();
DebugLoc DL = MI->getDebugLoc();
Register Reg =
