on 2024/7/11 11:36, Peter Bergner wrote:
> On 7/10/24 1:01 AM, Kewen.Lin wrote:
>>> + if (rs6000_rop_protect)
>>> + {
>>> + /* Disallow CPU targets we don't support. */
>>> + if (!TARGET_POWER8)
>>> + error ("%<-mrop-protect%> requires %<-mcpu=power8%> or later");
>>> +
>>> + /* Disallow ABI targets we don't support. */
>>> + if (DEFAULT_ABI != ABI_ELFv2)
>>> + error ("%<-mrop-protect%> requires the ELFv2 ABI");
>>> + }
>>
>> I wonder if there is some reason to put this hunk here. IMHO we want the
>> hunk
>> in rs6000_option_override_internal instead since no optimization options can
>> affect cpu type and DEFAULT_ABI?
>
> So the original code that used to disable shrink-wrapping that looked like:
>
> /* If we are inserting ROP-protect instructions, disable shrink wrap. */
> if (rs6000_rop_protect)
> flag_shrink_wrap = 0;
>
> ...used to be in rs6000_option_override_internal, but was moved to
> rs6000_override_options_after_change as part of PR101324 (commit
> cff7879a381d).
Yeah, I noticed that, but it's for optimization option flag_shrink_wrap. If we
just disable shrink-wrap in rs6000_option_override_internal, some function
adopts
optimize attribute to enable shrink-wrap, it would be re-enabled, which is
unexpected.
But target options (this patch cares about) don't have the issue, so I think
function rs6000_option_override_internal is a better fit.
> I guess I just placed this code here since it was the correct location for
> that old usage (it's changed in the patch I'm waiting for Segher to
> re-review),
> but I will look at moving it to rs6000_option_override_internal. I think I
> thought we could use a target attribute to enable -mrop-protect, but looking
> closer, we don't actually allow that option there.
>
>
>
>> And we probably want to unset rs6000_rop_protect to align with the handlings
>> on other options?
>
> I'm not sure I know what you mean? Why would we unset rs6000_rop_protect?
> Either we've concluded the current target options allow ROP code gen or not
> and for the cases where we don't/can't allow ROP, we want to give the user
> and error to match clang's behavior and how we already handle unsupported
> ABIs. So what is it you're trying to describe here?
Sorry for the confusion, I meant for most target options when we emit some error
message meanwhile we also unset it, such as:
if (TARGET_CRYPTO && !TARGET_ALTIVEC)
{
if (rs6000_isa_flags_explicit & OPTION_MASK_CRYPTO)
error ("%qs requires %qs", "-mcrypto", "-maltivec");
rs6000_isa_flags &= ~OPTION_MASK_CRYPTO;
}
if (!TARGET_FPRND && TARGET_VSX)
{
if (rs6000_isa_flags_explicit & OPTION_MASK_FPRND)
/* TARGET_VSX = 1 implies Power 7 and newer */
error ("%qs requires %qs", "-mvsx", "-mfprnd");
rs6000_isa_flags &= ~OPTION_MASK_FPRND;
}
...
if (TARGET_DFP && !TARGET_HARD_FLOAT)
{
if (rs6000_isa_flags_explicit & OPTION_MASK_DFP)
error ("%qs requires %qs", "-mhard-dfp", "-mhard-float");
rs6000_isa_flags &= ~OPTION_MASK_DFP;
}
...
I guess this point here is to avoid the caught unexpected thing to propagate
in the following processing like causing re-error or hit some unexpected
assertion etc., although in most cases this concern won't happen, it seems
not harmful to unset it (align with the others).
BR,
Kewen
