on 2024/7/3 23:05, Peter Bergner wrote:
> On 7/3/24 4:01 AM, Kewen.Lin wrote:
>>> - if (TARGET_POWER10
>>> + if (TARGET_POWER8
>>> && info->calls_p
>>> && DEFAULT_ABI == ABI_ELFv2
>>> && rs6000_rop_protect)
>>
>> Nit: I noticed that this is the only place to change
>> info->rop_hash_size to non-zero, and ...
>>
>>> @@ -3277,7 +3277,7 @@ rs6000_emit_prologue (void)
>>> /* NOTE: The hashst isn't needed if we're going to do a sibcall,
>>> but there's no way to know that here. Harmless except for
>>> performance, of course. */
>>> - if (TARGET_POWER10 && rs6000_rop_protect && info->rop_hash_size != 0)
>>> + if (TARGET_POWER8 && rs6000_rop_protect && info->rop_hash_size != 0)
>>
>> ... this condition and ...
>>
>>> {
>>> gcc_assert (DEFAULT_ABI == ABI_ELFv2);
>>> rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
>>> @@ -5056,7 +5056,7 @@ rs6000_emit_epilogue (enum epilogue_type
>>> epilogue_type)
>>>
>>> /* The ROP hash check must occur after the stack pointer is restored
>>> (since the hash involves r1), and is not performed for a sibcall. */
>>> - if (TARGET_POWER10
>>> + if (TARGET_POWER8> && rs6000_rop_protect
>>> && info->rop_hash_size != 0
>>
>> ... here, both check info->rop_hash_size isn't zero, I think we can drop
>> these
>> two TARGET_POWER10 (TARGET_POWER8) and rs6000_rop_protect checks? Instead
>> just
>> update the inner gcc_assert (now checking DEFAULT_ABI == ABI_ELFv2) by extra
>> checkings on TARGET_POWER8 && rs6000_rop_protect?
>>
>> The other looks good to me, ok for trunk with this nit tweaked (if you agree
>> with it and re-tested well), thanks!
>
> I agree with you, because the next patch I haven't submitted yet (waiting
> on this to get in), makes that simplification as part of the adding earlier
> checking of invalid options. :-) The follow-on patch will not only remove
> the TARGET_* and the 2nd/3rd rs6000_rop_protect usage, but will also remove
> the test and asserts of ELFv2...because we've already verified valid option
> usage earlier in the normal options handling code.
>
> Therefore, I'd like to keep this patch as simple as possible and limited to
> the TARGET_POWER10 -> TARGET_POWER8 change and the cleanup of those tests is
> coming in the next patch...which has already been tested.
Looking forward to the upcoming patch, then this patch is ok for trunk, thanks!
BR,
Kewen
