nickdesaulniers added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3138
}
+ if (EffectiveTriple.isAArch64() && Value != "sp_el0") {
+ D.Diag(diag::err_drv_invalid_value_with_suggestion)
----------------
DavidSpickett wrote:
> 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)
Right, I figure that can be a separate decision from the rest of the
implementation, I've forked that off in https://reviews.llvm.org/D101327.
Whether it lands or not doesn't matter to me.
================
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)
----------------
DavidSpickett wrote:
> 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.
I don't think so; GCC seems to support `tls` for x86 but not for aarch64.
https://godbolt.org/z/6WjEPfhT5
================
Comment at: clang/test/Driver/stack-protector-guard.c:59
+// INVALID-VALUE-AARCH64: error: invalid value 'tls' in
'mstack-protector-guard=','valid arguments to '-mstack-protector-guard='
are:sysreg global'
+// INVALID-REG-AARCH64: error: invalid value 'foo' in
'mstack-protector-guard-reg=','for AArch64'
----------------
DavidSpickett wrote:
> I'm not sure if this is due to your code or the error machinery itself but
> these errors are strangely written.
>
> I'd expect:
> ```
> error: invalid value 'tls' in 'mstack-protector-guard='tls', valid arguments
> to '-mstack-protector-guard=' are:sysreg global'
> ```
>
> Maybe it's assuming that there could be multiple values and `','` means that
> list option treats the value as a list? Or it's not using the right value and
> the comma is meant to be after the `=''` as in my example.
Sure, `err_drv_invalid_value` would be better.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100919/new/
https://reviews.llvm.org/D100919
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
