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
h
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
___
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 wr
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
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 redundan
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 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() << Valu
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/DiagnosticCommonK
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 om
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
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:
>
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
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
Repos
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)
nickdesaul
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` availab
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-
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
17 matches
Mail list logo