Re: [PATCH] [gdb/record] Fix syscall exit recording for arm

[Luis <luis.machado.foss@gmail.com>]

Hi Tom,

Sorry, I was out on time off.

On 10/03/2026 14:36, Tom de Vries wrote:
> [ Submitted earlier [1] with $subject: "[PATCH] [gdb/record] Fix syscall
> recording for arm". ]
> 
> On arm-linux, I run into:
> ...
> (gdb) continue^M
> Continuing.^M
> The next instruction is syscall exit_group.  It will make the program exit.  \
>    Do you want to stop the program?([y] or n) yes^M
> Process record does not support instruction 0xdf00 at address 0xf7e8f984.^M
> Process record: failed to record execution log.^M
> ^M
> Program stopped.^M
> __libc_do_syscall () at libc-do-syscall.S:46^M
> warning: 46     libc-do-syscall.S: No such file or directory^M
> (gdb) FAIL: gdb.reverse/sigall-reverse.exp: continue to signal exit
> ...
> 
> The problem is this bit of code here in decode_insn:
> ...
>        ret = thumb2_record_decode_insn_handler (arm_record);
> 
>        if (ret != ARM_RECORD_SUCCESS)
>         {
>           arm_record_unsupported_insn (arm_record);
>           ret = -1;
>         }
> ...
> where ret == 1 is mapped to -1.
> 
> The 1 is returned by arm_linux_syscall_record and is meant to be interpreted
> using this categorization:
> - res  < 0: Process record: failed to record execution log.
> - res == 0: No failure.
> - res  > 0: Process record: inferior program stopped.
> 
> But the port interprets 1 as ARM_RECORD_FAILURE:
> ...
> enum arm_record_result
> {
>    ARM_RECORD_SUCCESS = 0,
>    ARM_RECORD_FAILURE = 1
> };
> ...
> 
> We could fix this confusion this by:
> - adding an ARM_RECORD_UNKNOWN = 2, and
> - applying translations at the appropriate points, translating:
>    - ARM_RECORD_UNKNOWN into 1 and vice versa,
>    - ARM_RECORD_FAILURE into -1 and vice versa,
>    similar to what we did for aarch64 and loongarch.
> 
> But it seems easier to adopt a go-with-the-flow approach, defining
> ARM_RECORD_FAILURE as -1, freeing up the 1 for ARM_RECORD_UNKNOWN = 1.
> 
> Then the aforementioned FAIL is fixed by simply doing:
> ...
>        if (ret == ARM_RECORD_FAILURE)
>         arm_record_unsupported_insn (arm_record);
> ...
> 
> Tested on arm-linux.
> 
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2026-February/225372.html
> ---
>   gdb/arm-tdep.c | 43 +++++++++++++++++++++++++++----------------
>   1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index dabae0aec0f..7dfde6649d9 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -11195,10 +11195,30 @@ sbo_sbz (uint32_t insn, uint32_t bit_num, uint32_t len, uint32_t sbo)
>     return 1;
>   }
>   
> +/* The record infrastructure supports the following result values:
> +   1. res  < 0: Process record: failed to record execution log.
> +   2. res == 0: No failure.
> +   3. res  > 0: Process record: inferior program stopped.
> +
> +   For aarch64, we have two distinct failure values:
> +   - AARCH64_RECORD_FAILURE:
> +     Process record: failed to record execution log.
> +   - AARCH64_RECORD_UNSUPPORTED:
> +     Process record does not support instruction $hex at address $hex.
> +     Process record: failed to record execution log.
> +
> +   For some reason for arm we don't have an UNSUPPORTED enum value, and
> +   instead treat ARM_RECORD_FAILURE like an UNSUPPORTED enum value.  */
> +

Probably historical reasons.

>   enum arm_record_result
>   {
> +  /* Process record does not support instruction $hex at address $hex.
> +     Process record: failed to record execution log.  */
> +  ARM_RECORD_FAILURE = -1,
> +  /* No failure.  */
>     ARM_RECORD_SUCCESS = 0,
> -  ARM_RECORD_FAILURE = 1
> +  /* Process record: inferior program stopped.  */
> +  ARM_RECORD_UNKNOWN = 1,
>   };
>   
>   enum arm_record_strx_t
> @@ -14576,11 +14596,8 @@ decode_insn (abstract_instruction_reader &reader,
>   	     then we need not decode it anymore.  */
>   	  ret = arm_handle_insn[insn_id] (arm_record);
>   	}
> -      if (ret != ARM_RECORD_SUCCESS)
> -	{
> -	  arm_record_unsupported_insn (arm_record);
> -	  ret = -1;
> -	}
> +      if (ret == ARM_RECORD_FAILURE)
> +	arm_record_unsupported_insn (arm_record);
>       }
>     else if (THUMB_RECORD == record_type)
>       {
> @@ -14588,11 +14605,8 @@ decode_insn (abstract_instruction_reader &reader,
>         arm_record->cond = -1;
>         insn_id = bits (arm_record->arm_insn, 13, 15);
>         ret = thumb_handle_insn[insn_id] (arm_record);
> -      if (ret != ARM_RECORD_SUCCESS)
> -	{
> -	  arm_record_unsupported_insn (arm_record);
> -	  ret = -1;
> -	}
> +      if (ret == ARM_RECORD_FAILURE)
> +	arm_record_unsupported_insn (arm_record);
>       }
>     else if (THUMB2_RECORD == record_type)
>       {
> @@ -14605,11 +14619,8 @@ decode_insn (abstract_instruction_reader &reader,
>   
>         ret = thumb2_record_decode_insn_handler (arm_record);
>   
> -      if (ret != ARM_RECORD_SUCCESS)
> -	{
> -	  arm_record_unsupported_insn (arm_record);
> -	  ret = -1;
> -	}
> +      if (ret == ARM_RECORD_FAILURE)
> +	arm_record_unsupported_insn (arm_record);
>       }
>     else
>       {
> 
> base-commit: 1add703e09f0f8d073cde4af9d11cd59996e9763

The patch looks fine for me.

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