gdb/arm: Fix epilogue frame id

Message ID 20240123215229.2385404-1-thiago.bauermann@linaro.org
State New
Headers
Series gdb/arm: Fix epilogue frame id |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Thiago Jung Bauermann Jan. 23, 2024, 9:52 p.m. UTC
  arm_epilogue_frame_this_id has a comment saying that it fall backs to using
the current PC if the function start address can't be identified, but it
actually uses only the PC to make the frame id.

This patch makes the code match the comment.  Another hint that it's what
is intended is that arm_prologue_this_id, a function almost identical to
it, does that.

The problem was found by code inspection.  It fixes the following testsuite
failures:

FAIL: gdb.base/unwind-on-each-insn.exp: foo: instruction 9: check frame-id matches
FAIL: gdb.reverse/solib-reverse.exp: reverse-next third shr1
FAIL: gdb.reverse/solib-reverse.exp: reverse-next second shr1
FAIL: gdb.reverse/solib-reverse.exp: reverse-next first shr1
FAIL: gdb.reverse/solib-reverse.exp: reverse-next generic
FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function one
FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function one
FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function two
FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function two

Tested on arm-linux-gnueabi-hf.
---
 gdb/arm-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Luis Machado Jan. 24, 2024, 7:40 a.m. UTC | #1
On 1/23/24 21:52, Thiago Jung Bauermann wrote:
> arm_epilogue_frame_this_id has a comment saying that it fall backs to using
> the current PC if the function start address can't be identified, but it
> actually uses only the PC to make the frame id.
> 
> This patch makes the code match the comment.  Another hint that it's what
> is intended is that arm_prologue_this_id, a function almost identical to
> it, does that.
> 
> The problem was found by code inspection.  It fixes the following testsuite
> failures:
> 
> FAIL: gdb.base/unwind-on-each-insn.exp: foo: instruction 9: check frame-id matches
> FAIL: gdb.reverse/solib-reverse.exp: reverse-next third shr1
> FAIL: gdb.reverse/solib-reverse.exp: reverse-next second shr1
> FAIL: gdb.reverse/solib-reverse.exp: reverse-next first shr1
> FAIL: gdb.reverse/solib-reverse.exp: reverse-next generic
> FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function one
> FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function one
> FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function two
> FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function two
> 
> Tested on arm-linux-gnueabi-hf.
> ---
>  gdb/arm-tdep.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index f1aa730579bc..0d0431e0d1cd 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -3252,7 +3252,7 @@ arm_epilogue_frame_this_id (frame_info_ptr this_frame,
>  
>    arm_gdbarch_tdep *tdep
>      = gdbarch_tdep<arm_gdbarch_tdep> (get_frame_arch (this_frame));
> -  *this_id = frame_id_build (arm_cache_get_prev_sp_value (cache, tdep), pc);
> +  *this_id = frame_id_build (arm_cache_get_prev_sp_value (cache, tdep), func);
>  }
>  
>  /* Implementation of function hook 'prev_register' in

Thanks. This is OK.

Approved-By: Luis Machado <luis.machado@arm.com>
  
Thiago Jung Bauermann Jan. 24, 2024, 2:48 p.m. UTC | #2
Hello Luis,

Luis Machado <luis.machado@arm.com> writes:

> On 1/23/24 21:52, Thiago Jung Bauermann wrote:
>> arm_epilogue_frame_this_id has a comment saying that it fall backs to using
>> the current PC if the function start address can't be identified, but it
>> actually uses only the PC to make the frame id.
>> 
>> This patch makes the code match the comment.  Another hint that it's what
>> is intended is that arm_prologue_this_id, a function almost identical to
>> it, does that.
>> 
>> The problem was found by code inspection.  It fixes the following testsuite
>> failures:
>> 
>> FAIL: gdb.base/unwind-on-each-insn.exp: foo: instruction 9: check frame-id matches
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-next third shr1
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-next second shr1
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-next first shr1
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-next generic
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function one
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function one
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-step into solib function two
>> FAIL: gdb.reverse/solib-reverse.exp: reverse-step within solib function two
>> 
>> Tested on arm-linux-gnueabi-hf.
>> ---
>>  gdb/arm-tdep.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index f1aa730579bc..0d0431e0d1cd 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -3252,7 +3252,7 @@ arm_epilogue_frame_this_id (frame_info_ptr this_frame,
>>  
>>    arm_gdbarch_tdep *tdep
>>      = gdbarch_tdep<arm_gdbarch_tdep> (get_frame_arch (this_frame));
>> -  *this_id = frame_id_build (arm_cache_get_prev_sp_value (cache, tdep), pc);
>> +  *this_id = frame_id_build (arm_cache_get_prev_sp_value (cache, tdep), func);
>>  }
>>  
>>  /* Implementation of function hook 'prev_register' in
>
> Thanks. This is OK.

Thanks for the quick review. Pushed as commit 44acb01769b0.

> Approved-By: Luis Machado <luis.machado@arm.com>

Sorry, I just noticed that I forgot to add your tag to the commit
message.
  
Luis Machado Jan. 24, 2024, 2:53 p.m. UTC | #3
On 1/24/24 14:48, Thiago Jung Bauermann wrote:
> 
> Sorry, I just noticed that I forgot to add your tag to the commit
> message.
> 

Don't worry. Not a big deal.
  

Patch

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index f1aa730579bc..0d0431e0d1cd 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -3252,7 +3252,7 @@  arm_epilogue_frame_this_id (frame_info_ptr this_frame,
 
   arm_gdbarch_tdep *tdep
     = gdbarch_tdep<arm_gdbarch_tdep> (get_frame_arch (this_frame));
-  *this_id = frame_id_build (arm_cache_get_prev_sp_value (cache, tdep), pc);
+  *this_id = frame_id_build (arm_cache_get_prev_sp_value (cache, tdep), func);
 }
 
 /* Implementation of function hook 'prev_register' in