[PATCH v2] Refrain from asking debug stubs to read invalid memory

[Andrew Burgess]

Kévin Le Gouguec <legouguec@adacore.com> writes:

> Some stubs take exception to this.
>
> For example we observe RTEMS's libdebugger freezing when asked to examine
> address zero on aarch64/xilinx_zynqmp_lp64_qemu.  As of 2024-02-02 "gdb,
> types: Resolve pointer types dynamically" (f18fc7e56fb) this happens as
> early as 'target remote'.  Ordinarily we would be greeted with…
>
>     _User_extensions_Thread_switch (executing=0x0, heir=<optimized out>)
>     at […]/cpukit/include/rtems/score/userextimpl.h:382
>
> … but now, as language_defn::read_var_value calls resolve_dynamic_type with
> a "dummy" address and value, resolve_dynamic_type_internal receives a
> similarly "dummy" addr_stack, and attempts to read memory address zero:
> guard against that.

The problem with this approach is that there are targets out there that
can read from address 0, so detecting 0 and handling it differently as
you propose is not the right approach.

But *sigh* I see we already have code that's special casing 0 in this
function, so maybe adding yet more is just something I'll need to
accept.

Ideally, if we wanted to support not reading memory then we should carry
around a std::optional<CORE_ADDR> instead of just a CORE_ADDR.

As for the target freezing when trying to read from 0 ... that's 100% a
target bug.  If the actual h/w doesn't support reading 0 then the
controller (e.g. OpenOCD) should take care of filtering out the access
requests.  Or in your case (qemu) then the emulator should just be fixed
to return an error on these accesses.  The other option is for the
remote to provide GDB with a memory map[1].

Anyway, I do have a more substantial question, I guess when you talk
about the resolve_dynamic_type calls in ::read_var_value, you are
talking about the lines like this:

  type = resolve_dynamic_type (type, view, /* Unused address.  */ 0);

The problem you see is that these end up calling:

  read_memory_typed_address (addr_stack->addr, type);

in resolve_dynamic_type_internal.  Now as far as I can tell this
function will throw an exception if the read fails.  So, surely, if
we're sending through '0' as an address with the expectation that we'll
not access the address ... but now we are ... then surely this would
trigger an exception on a target where 0 is not readable, but the target
doesn't hang, right?  For example, x86-64 GNU/Linux.

Which makes me think: can't we write a test case for this bug?

Thanks,
Andrew

[1] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Memory-Map-Format.html#Memory-Map-Format

>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
> New in v2: applied Thiago's suggestion to add an 'else' branch.
>
>  gdb/gdbtypes.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index f39fe3de6a4..856eff4d166 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -2804,8 +2804,10 @@ resolve_dynamic_type_internal (struct type *type,
>  	    if (addr_stack->valaddr.data () != NULL)
>  	      pinfo.addr = extract_typed_address (addr_stack->valaddr.data (),
>  						  type);
> -	    else
> +	    else if (addr_stack->addr != 0)
>  	      pinfo.addr = read_memory_typed_address (addr_stack->addr, type);
> +	    else
> +	      pinfo.addr = 0;
>  	    pinfo.next = addr_stack;





>  
>  	    /* Special case a NULL pointer here -- we don't want to
> -- 
> 2.34.1