[V4,1/8] gas: scfi: make scfi_state_restore_reg function more precise

Message ID 20240701025404.3361349-2-indu.bhagat@oracle.com
State New
Headers
Series Add SCFI support for aarch64 |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Indu Bhagat July 1, 2024, 2:53 a.m. UTC
  [No changes in V3, V4]
  PS: This patch needs a global maintainer review/approval.

[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.
---
 gas/scfi.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
  

Comments

Indu Bhagat July 12, 2024, 3:03 p.m. UTC | #1
[PING]


On 6/30/24 7:53 PM, Indu Bhagat wrote:
> [No changes in V3, V4]
>    PS: This patch needs a global maintainer review/approval.
> 
> [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.
> ---
>   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
>
  

Patch

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