[PATCH] D88631: [X86] Support customizing stack protector guard

Xiang Zhang via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 19 17:17:25 PDT 2020


xiangzhangllvm added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3006
+
+  // TBD: Currently we just supported "tls" and "global" for X86 target.
+  // the "sysreg" will be supported in AArch64 later.
----------------
MaskRay wrote:
> nickdesaulniers wrote:
> > xiangzhangllvm wrote:
> > > MaskRay wrote:
> > > > We use TODO instead of TBD. 
> > > > 
> > > > Suggest: TODO: Support "sysreg" for AArch64.
> > > > 
> > > > If AArch64 is not supported yet, please don't accept an AArch64 specific value.
> > > I tend to fix it, thanks your review!
> > > 
> > > And please let me sync with nick, because he has some suggestions here before.
> > > 
> > > @nickdesaulniers 
> > > I find gcc 10.2 didn't support sysreg, So if you don't mind, I tend to remove it in next update.
> > > 
> > > ```
> > > $gcc -O2 -fstack-protector-all  -mstack-protector-guard=sysreg t.c -S -o -
> > > gcc: error: unrecognized argument in option ‘-mstack-protector-guard=sysreg’
> > > gcc: note: valid arguments to ‘-mstack-protector-guard=’ are: global tls
> > > ```
> > That's because:
> > 1. it's currently only implemented for `aarch64-linux-gnu-gcc`, try with that and you'll see.
> > 2. GCC is architecturally different than clang in that GCC needs to be configured to target one backend, always, while Clang defaults to all backends enabled but can be reconfigured to disable backends.
> > 
> > Maybe worthwhile to warn then that `-mstack-protector-guard=sysreg` is not supported by "this target" when this target is x86.
> The driver should match what has actually been implemented. Since only x86 supports the toggle, just don't accept AArch64 specific values.
> 
> For this case, just don't mention AArch64 or sysreg in the code. You can freely add a TODO.
In my eye, I think MaskRay make sense.  @nickdesaulniers, I'll mark TODO here for "sysreg", it is better to add it when AArch64 want fully implemented it.


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

https://reviews.llvm.org/D88631



More information about the llvm-commits mailing list