nickdesaulniers added a comment.

In D102742#2774562 <https://reviews.llvm.org/D102742#2774562>, @tejohnson wrote:

> In D102742#2774185 <https://reviews.llvm.org/D102742#2774185>, 
> @nickdesaulniers wrote:
>
>> - upgrade module merge strategy to Error
>
> Probably want a test using llvm-link or llvm-lto to check this behavior (that 
> alike flags are getting propagated as expected and that conflicting ones 
> error)

Not many tests under llvm/test/LTO or llvm/test/ThinLTO use `llvm-link`; is 
there a more appropriate subdir for such a test for conflicting module 
attributes?



================
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:
> 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.


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

Reply via email to