[PATCH] D100919: [AArch64] Support customizing stack protector guard

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3110
+    }
+    if (EffectiveTriple.isAArch64() && Value != "sysreg" && Value != "global") {
+      D.Diag(diag::err_drv_invalid_value_with_suggestion)
----------------
Shouldn't this also allow "tls"? At least that's what the previous code works out to, I don't know if that actually works on AArch64 or if it just didn't error.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3138
     }
+    if (EffectiveTriple.isAArch64() && Value != "sp_el0") {
+      D.Diag(diag::err_drv_invalid_value_with_suggestion)
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > TODO: can we re-use `AArch64SysReg::lookupSysRegByName` in the frontend?
> > > > I don't think so because `llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h` is under lib/ not include/. Not sure if I should just remove reg validation?
> > > Guidance provided by @echristo and @jyknight was that we should avoid such linkage requirements on Target/, so instead I'll work on adding a helper to clang/lib/Driver/ToolChains/Arch/AArch64.cpp that duplicates logic from `AArch64SysReg::lookupSysRegByName`.
> > It looks like there's ~1000 possible sysregs for aarch64 ATM; do we really want to add all of those to clang?
> I'm going to post that as a separate commit/review on top of this series, that way it doesn't pollute this code review. This is ready to be reviewed.
If the number of different registers people actually use with this option is somewhere < 10 I'd just hardcode the names here as needed. (a large amount of those sysregs won't be suitable for this purpose anyway)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100919