[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

[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

[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

[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:

[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

[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;

[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() <<

[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:

[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

[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

[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:

[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:

[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

[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)

[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`

[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

[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