Re: [PATCH v2] gdb/dwarf: filter discarded zero-address variables after relocation

[Kevin Buettner <kevinb@redhat.com>]

Hi Longjun,

This looks pretty good.  I just have a few nits...

On 10/15/26 9:00 PM, Longjun Luo wrote:
> A variable discarded by section GC can retain a DIE with a zero DWARF
> address. var_decode_location adds the section offset before new_symbol
> checks that address. If the objfile has already been relocated, the
> symbol survives the zero-address check and can read the ELF header as
> data.
> 
> This affects PIE executables when the CU is first expanded after
> starting the program. It also affects normally loaded shared libraries,
> including with --readnow, because their objfiles already have a load
> offset.
> 
> Have var_decode_location return the address it decoded before
> relocation, and check that address in new_symbol, as the DWARF reader
> already does for discarded functions and address ranges. Preserve the
> existing exception for objects with allocated sections at zero.
> 
> Extend gdb.dwarf2/dw2-var-zero-addr.exp with a PIE whose CU is first
> expanded after starting the program, and with a loaded shared library,
> with and without --readnow. Use a distinctive variable name to avoid
> clashes with shared-library debug symbols after the program starts.
> 
> Tested on x86_64 GNU/Linux with GCC. Without the fix, the three new
> cases fail; with it, the test passes. GDB selftests pass.
> 
> Signed-off-by: Longjun Luo <luolongjuna@gmail.com>
> ---
> v1: https://inbox.sourceware.org/gdb-patches/20261009174623.2894733-1-luolongjuna@gmail.com/
> 
> Changes in v2:
> - Use a test-specific variable name to avoid clashes with symbols from
>   shared-library debug information. No reader changes.
> 
> Linaro CI on ARM and AArch64 reported failures in the three new cases:
> 
>   (gdb) print &var
>   Can't take address of "var" which isn't an lvalue.
> 
> After the discarded test variable is filtered out, the lookup can
> find an unrelated symbol with the same name. I reproduced this on
> x86_64 with a same-named enumerator in shared-library debug
> information. The renamed test passes with and without those symbols.
> 
> Re-tested on x86_64 GNU/Linux with GCC, Clang and cc-with-gdb-index
> (7 PASS each). The three added cases still fail without the fix.
> All 195 selftests pass. A full native testsuite comparison, with
> differing cases rechecked, found no new regressions; existing failures
> and test-driver errors remained in both variants.
> 
> As in v1, I do not have an FSF copyright assignment for GDB and have
> signed off this patch under the DCO.

The GDB project does not yet accept contributions under the DCO.  (It's
something which is under discussion though; I'd very much like to see it
happen.)

IMO, this fix is of a size that is "legally significant".  We'll need a
copyright assignment, I think.

>  gdb/dwarf2/read.c                             | 27 +++++----
>  gdb/testsuite/gdb.dwarf2/dw2-var-zero-addr.S  |  2 +-
>  .../gdb.dwarf2/dw2-var-zero-addr.exp          | 60 ++++++++++++++++++-
>  3 files changed, 76 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 1fc6d7ffb4f..48c1100b645 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -514,9 +514,8 @@ show_dwarf_synchronous (struct ui_file *file, int from_tty,
>  \f
>  /* local function prototypes */
>  
> -static void var_decode_location (struct attribute *attr,
> -				 struct symbol *sym,
> -				 struct dwarf2_cu *cu);
> +static std::optional<unrelocated_addr> var_decode_location
> +  (struct attribute *attr, struct symbol *sym, struct dwarf2_cu *cu);
>  
>  static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
>  
> @@ -15160,7 +15159,11 @@ dwarf_decode_line_header (sect_offset sect_off, struct dwarf2_cu *cu,
>  				   &cu->header, comp_dir);
>  }
>  
> -static void
> +/* Decode the DW_AT_location attribute ATTR of SYM.  If ATTR is a single
> +   DW_OP_addr, DW_OP_addrx or DW_OP_GNU_addr_index, return its address
> +   before relocation.  */

Thanks for adding this comment.  Could you also mention what the
return value is when ATTR isn't one of the mentioned ATTRs? E.g.,
something like "Otherwise, return std::nullopt."

I noticed, too, that two of the three callers discard the return
value.  This might be worth a one line comment at the call site,
though there are plenty of cases where this is not done.  I'll leave it
to you to decide whether those sites deserve a comment.

Kevin