Re: [PATCH v2] gdb: z80: Fix endless backtrace loop and assertion crashes

[Kevin Buettner <kevinb@redhat.com>]

Hi Ronald,

Just a few nits...

On Sun,  7 Jun 2026 09:48:01 +0200
Ronald Hecht <ronald.hecht@gmx.de> wrote:

> This patch fixes two distinct issues in the Z80 frame unwind cache:
> 
> 1. An infinite loop in the stack pointer wrapping logic. The `for (;; ++sp)`
>    loop could run endlessly if the overflow condition `sp < this_base` was
>    never met due to 16-bit address space wrapping. This is fixed by
>    checking for `sp > addr_space_max` instead of masking the address.
>    Additionally, a `loop_count` is introduced to break out after 4 iterations.
>    This prevents severe slow-downs on remote targets caused by scanning
>    large portions of the address space when the stack is corrupt.
> 
> 2. A potential internal GDB assertion failure. The code previously called
>    `.addr()` directly on saved registers without verifying if they actually
>    held an address, causing crashes when encountering unexpected register
>    states (e.g. REG_UNKNOWN). Added a check for `.is_addr()` beforehand.
> 
> gdb/ChangeLog:
> 	* z80-tdep.c (z80_frame_unwind_cache): Prevent infinite loop by
> 	checking addr_space_max and adding loop_count. Check is_addr()
> 	before calling addr().
> ---
>  gdb/z80-tdep.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/z80-tdep.c b/gdb/z80-tdep.c
> index 3a0d5f7e393..832433f05cb 100644
> --- a/gdb/z80-tdep.c
> +++ b/gdb/z80-tdep.c
> @@ -595,16 +595,28 @@ z80_frame_unwind_cache (const frame_info_ptr &this_frame,
>      {
>        CORE_ADDR addr;
>        CORE_ADDR sp;
> -      CORE_ADDR sp_mask = (1 << gdbarch_ptr_bit(gdbarch)) - 1;
> +      CORE_ADDR addr_space_max = (1 << gdbarch_ptr_bit(gdbarch)) - 1;
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +      int loop_count = 0;
>        /* Assume that the FP is this frame's SP but with that pushed
>  	 stack space added back.  */
>        this_base = get_frame_register_unsigned (this_frame, Z80_SP_REGNUM);
>        sp = this_base + info->size;
>        for (;; ++sp)
>  	{
> -	  sp &= sp_mask;
> -	  if (sp < this_base)
> +	  /* Limit the scan to 4 iterations. If the unwinder's frame
> +             size calculation is slightly off (e.g. due to unpopped
> +             16-bit arguments or temporary pushes like SDCC sometimes
> +             generates), the return address might be hidden a few
> +             bytes deeper. Scanning up to 4 bytes comfortably covers
> +             one 32-bit or two 16-bit misplaced values. Scanning
> +             further (e.g. 8+ bytes) drastically increases the risk of
> +             false positives: we might wander into the caller's local
> +             variables, hit a random 0xCD (CALL) byte, and generate a
> +             corrupted backtrace. It also prevents massive slow-downs
> +             on remote serial targets if the stack is severely
> +             corrupted. */

There's a whitespace problem in the above comment.  Our whitespace
conventions use tabs in the place of (multiples of) 8 leading spaces. 
You've used spaces here with no tabs on the continuation lines.  (The
first line is correct.)

Another coding convention nit: GNU coding conventions have two spaces
following a period.

Also, I believe that the CALL matching code also matches conditional
call instructions, but you mention only the unconditional case.  Not
sure if it's worth changing the comment to cover this, but I thought
I'd mention it.

> +	  if (++loop_count > 4 || sp > addr_space_max)
>  	    { /* overflow, looks like end of stack */
>  	      sp = this_base + info->size;
>  	      break;

I'm wondering if 4 is enough to handle the eZ80 which has 24 bit addresses
in extended mode?  Should the limit change depending on the architecture?
Or, alternately, maybe make the limit depend on the address width?

It occurs to me too that the disjunct which matters most now is the
loop_count check.  The comparison against addr_space_max will rarely
if ever be triggered.  OTOH, I suppose it still could be when working
at the very end/bottom of the stack.  Regardless, it seems to me that
the "overflow, looks like end of stack" comment should be revised as
well.

Kevin