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).
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?
Peter
