nickdesaulniers added inline comments.
================
Comment at: clang/include/clang/Driver/Options.td:3429
HelpText<"Use the given reg for addressing the stack-protector guard">,
- MarshallingInfoString<CodeGenOpts<"StackProtectorGuardReg">, [{"none"}]>;
+ MarshallingInfoString<CodeGenOpts<"StackProtectorGuardReg">>;
def mfentry : Flag<["-"], "mfentry">, HelpText<"Insert calls to fentry at
function entry (x86/SystemZ only)">,
----------------
tejohnson wrote:
> nickdesaulniers wrote:
> > tejohnson wrote:
> > > nickdesaulniers wrote:
> > > > tejohnson wrote:
> > > > > nickdesaulniers wrote:
> > > > > > tejohnson wrote:
> > > > > > > What's the effect of or reason for this change?
> > > > > > Of the 3 options added in D88631 (`mstack_protector_guard_EQ`,
> > > > > > `mstack_protector_guard_offset_EQ`,
> > > > > > `mstack_protector_guard_reg_EQ`) 2 are strings
> > > > > > (`mstack_protector_guard_EQ` and `mstack_protector_guard_reg_EQ`).
> > > > > > It was inconsistent that one could be unspecified, where as the
> > > > > > other could be unspecified or `"none"` (but those 2 values were
> > > > > > equivalent).
> > > > > >
> > > > > > Without this change, in clang/lib/CodeGen/CodeGenModule.cpp I'd
> > > > > > need to check that `StackProtectorGuardReg != "none"` rather than
> > > > > > `!StackProtectorGuardReg.empty()` below.
> > > > > >
> > > > > > I can change it back, but I think the asymmetry between
> > > > > > `mstack_protector_guard_EQ` and `mstack_protector_guard_reg_EQ` in
> > > > > > D88631, and I missed that in code review.
> > > > > >
> > > > > > I don't think there would be any other observers of such a change.
> > > > > I see. Does unspecified mean something like just
> > > > > "-mstack-protector-guard-reg=" with nothing after the =? I didn't
> > > > > realize that was supported.
> > > > It looks like we validate for that case in the front end already.
> > > > Specifically, `RenderAnalyzerOptions` in
> > > > clang/lib/Driver/ToolChains/Clang.cpp.
> > > >
> > > > $ clang -mstack-protector-guard-reg= ...
> > > > clang-13: error: invalid value '' in 'mstack-protector-guard-reg='
> > > Does that mean that without the "none" handling there is no way to
> > > disable? I.e. overriding an earlier value. Not sure how important this is.
> > Oh, that's a great point. I guess I'm not really sure of the intention of
> > `"none"` then, @xiangzhangllvm can you comment whether that was the
> > intention?
> >
> > A quick test in GCC shows that GCC does not accept the value `"none"` for
> > either `-mstack-protector-guard=` or `-mstack-protector-guard-reg=`.
> >
> > We could strive to support disabling the command line flag once
> > respecified, but I'd rather do it for both of the above two flags, not just
> > `-mstack-protector-guard-reg=`.
> Yeah and it doesn't look like gcc supports anything like -mno-stack-...
> So this seems fine the way you have changed it, unless @xiangzhangllvm has a
> different opinion, but perhaps that could be resolved later and consistently
> between all the options as you note.
Sure, I'm happy to follow up on this if I got it wrong here. In the meantime,
this will help our CI go back green, so I've merged it. Thank you much for the
review!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102742/new/
https://reviews.llvm.org/D102742
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
