tejohnson 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)">,
----------------
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.
================
Comment at: llvm/test/LTO/AArch64/stack-protector-guard-module-attrs.ll:2-5
+; RUN: not llvm-link %t/a.ll %t/b.ll 2>&1 | FileCheck
--check-prefix=CHECK-KIND %s
+; RUN: not llvm-link %t/c.ll %t/d.ll 2>&1 | FileCheck --check-prefix=CHECK-REG
%s
+; RUN: not llvm-link %t/e.ll %t/f.ll 2>&1 | FileCheck
--check-prefix=CHECK-OFFSET %s
+; RUN: llvm-link %t/g.ll %t/h.ll
----------------
nickdesaulniers wrote:
> I used `llvm-link` here, but please let me know if it would be preferable to
> convert these to use `llvm-lto2` instead?
>
> Also, is there any issue with this new dir, `llvm/test/LTO/AArch64/`? Do I
> need to modify any lit cfg files so that the tests don't run for non-aarch64?
>
> Also, I guess this test isn't really specific to aarch64; I don't do any
> codegen and don't specify the target triple (though, these module attributes
> only will be emitted from the front end for x86 and aarch64 at the moment;
> perhaps riscv and ppc64le in the future).
Yeah I think you will want to add the aarch64 equivalent of
llvm/test/LTO/X86/lit.local.cfg, even if not needed for this initial test.
================
Comment at: llvm/test/LTO/AArch64/stack-protector-guard-module-attrs.ll:2-5
+; RUN: not llvm-link %t/a.ll %t/b.ll 2>&1 | FileCheck
--check-prefix=CHECK-KIND %s
+; RUN: not llvm-link %t/c.ll %t/d.ll 2>&1 | FileCheck --check-prefix=CHECK-REG
%s
+; RUN: not llvm-link %t/e.ll %t/f.ll 2>&1 | FileCheck
--check-prefix=CHECK-OFFSET %s
+; RUN: llvm-link %t/g.ll %t/h.ll
----------------
nickdesaulniers wrote:
> tejohnson wrote:
> > nickdesaulniers wrote:
> > > I used `llvm-link` here, but please let me know if it would be preferable
> > > to convert these to use `llvm-lto2` instead?
> > >
> > > Also, is there any issue with this new dir, `llvm/test/LTO/AArch64/`? Do
> > > I need to modify any lit cfg files so that the tests don't run for
> > > non-aarch64?
> > >
> > > Also, I guess this test isn't really specific to aarch64; I don't do any
> > > codegen and don't specify the target triple (though, these module
> > > attributes only will be emitted from the front end for x86 and aarch64 at
> > > the moment; perhaps riscv and ppc64le in the future).
> > Yeah I think you will want to add the aarch64 equivalent of
> > llvm/test/LTO/X86/lit.local.cfg, even if not needed for this initial test.
> Perhaps I should move this test under llvm/test/tools/llvm-link/ or
> llvm/test/Linker/ ? llvm/test/Linker/ has lots of tests that invoke
> `llvm-link`.
That would be fine too, I don't have a strong opinion either way. I suppose
since you don't need any code gen putting it under Linker would be fine (I
think I'd prefer that to it going under tools/llvm-link since you aren't
testing that tool specifically but rather the module linker behavior.
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
