[PATCH, V2 1/7] gas: scfi: make scfi_state_restore_reg function more precise

[Indu Bhagat]

On 6/6/24 09:09, Richard Earnshaw (lists) wrote:
> On 01/06/2024 08:34, Indu Bhagat wrote:
>> [New in V2]
>>   - This patch is orthogonal to aarch64 support.  It is included here
>>     because the testcase added for it is aarch64 specific.  It is a
>>     bugfix really.
>> [End of New in V2]
>>
>> When the SCFI machinery detects that a register has been restored from
>> stack, it makes some state changes in the SCFI state object.
>>
>> Prior to the patch, scfi_state_restore_reg () was setting a value of
>> (reg, CFI_IN_REG) for (base, state) respectively.  This was causing
>> issues in the cmp_scfi_state () function:
>>    - The default state of all (callee-saved) regs at the beginning of
>>      function is set to (0, CFI_UNDEFINED).
>>    - If a register is saved and restored on some control path, the state
>>      of reg is (reg, CFI_IN_REG) on that path.
>>    - On another control path where the register was perhaps not
>>      used (or saved/restored on stack) remains (0, CFI_UNDEFINED).
>>    - The two states should be treated equal, however, at the point in
>>      program after the register has been restored.
>>
>> Fix this by resetting the state to (0, CFI_UNDEFINED) in
>> scfi_state_restore_reg ().
>>
>> A testcase (scfi-cfg-4.s) for this is added in a subsequent commit.
>>
>> gas/
>>          * scfi.c (scfi_state_restore_reg): Reset to 0, CFI_UNDEFINED
>> 	for base, state.
> 
> This sound sensible to me, but it's outside of my maintainer role, so I can't approve it.
> 
> R.
> 

Thanks a lot Richard for taking a look.

Replying here for attention from any global maintainer.  I still need 
approval for this patch that I cannot split out of the series as then CI 
might complain about failing aarch64 SCFI test (added later in this series).

I will be posting a V4 version of the series soon.

Indu

>> ---
>>   gas/scfi.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/gas/scfi.c b/gas/scfi.c
>> index 744822d8102..5898a57b330 100644
>> --- a/gas/scfi.c
>> +++ b/gas/scfi.c
>> @@ -223,11 +223,12 @@ scfi_state_restore_reg (scfi_stateS *state, unsigned int reg)
>>     gas_assert (state->regs[reg].state == CFI_ON_STACK);
>>     gas_assert (state->regs[reg].base == REG_CFA);
>>   
>> -  state->regs[reg].base = reg;
>> +  /* PS: the register may still be on stack much after the restore.  Reset the
>> +     SCFI state to CFI_UNDEFINED, however, to indicate that the most updated
>> +     source of value is register itself from here onwards.  */
>> +  state->regs[reg].base = 0;
>>     state->regs[reg].offset = 0;
>> -  /* PS: the register may still be on stack much after the restore, but the
>> -     SCFI state keeps the state as 'in register'.  */
>> -  state->regs[reg].state = CFI_IN_REG;
>> +  state->regs[reg].state = CFI_UNDEFINED;
>>   }
>>   
>>   /* Identify if the given GAS instruction GINSN saves a register
>