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

Luis
2026-03-26T10:03:55Z

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>