Re: [PATCH v2] gdb/dwarf2: Add symbols for function declarations

[Andrew Burgess <aburgess@redhat.com>]

Kevin Buettner <kevinb@redhat.com> writes:

> This commit was motivated by comments 3 and 4 for bug 31563:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=31563#c3
>
> When a program is built with -g3, macro information is available to
> GDB; for errno, the macro defined in /usr/include/errno.h (provided by
> GLIBC) looks like this:
>
>     # define errno (*__errno_location ())
>
> However, up to now, GDB doesn't know the type of __errno_location,
> despite (sometimes) having a DIE representing a declaration providing
> its type.  In any case, apparently not knowing the return type of
> __errno_location, GDB was unable to perform the inferior function call
> specified by the errno macro:
>
>     (gdb) p errno
>     '__errno_location' has unknown return type; cast the call to its
>     declared return type
>
> But, for some compilers, GDB *should* be able to know the type.  These
> are the DIEs related to the __errno_location declaration from the
> "macros" case for the gdb.base/errno.exp test:
>
>  <1><37>: Abbrev Number: 2 (DW_TAG_subprogram)
>     <38>   DW_AT_external    : 1
>     <38>   DW_AT_name        : (indirect string, offset: 0x20e4):
>                                __errno_location
>     <3c>   DW_AT_decl_file   : 2
>     <3d>   DW_AT_decl_line   : 37
>     <3e>   DW_AT_decl_column : 13
>     <3f>   DW_AT_prototyped  : 1
>     <3f>   DW_AT_type        : <0x43>
>     <43>   DW_AT_declaration : 1
>  <1><43>: Abbrev Number: 3 (DW_TAG_pointer_type)
>     <44>   DW_AT_byte_size   : 8
>     <45>   DW_AT_type        : <0x49>
>  <1><49>: Abbrev Number: 4 (DW_TAG_base_type)
>     <4a>   DW_AT_byte_size   : 4
>     <4b>   DW_AT_encoding    : 5	(signed)
>     <4c>   DW_AT_name        : int
>
> If you wish to see this for yourself, from your gdb build directory,
> do:
>
>     make check TESTS=gdb.base/errno.exp
>     readelf -w testsuite/outputs/gdb.base/errno/errno-macros | less
>
> With this commit in place, using gcc as the C compiler, 8 XFAILs in
> gdb.base/errno.exp turn into PASSes.  They are:
>
>     XFAIL: gdb.base/errno.exp: macros: print (int) errno
>     XFAIL: gdb.base/errno.exp: macros: print errno
>     XFAIL: gdb.base/errno.exp: pthreads-macros: print (int) errno
>     XFAIL: gdb.base/errno.exp: pthreads-macros: print errno
>     XFAIL: gdb.base/errno.exp: pthreads-static-macros: print (int) errno
>     XFAIL: gdb.base/errno.exp: pthreads-static-macros: print errno
>     XFAIL: gdb.base/errno.exp: static-macros: print (int) errno
>     XFAIL: gdb.base/errno.exp: static-macros: print errno
>
> For the example shown earlier, GDB is now able to print the correct
> value for errno.
>
> As mentioned earlier, it doesn't work for all compliers.  In
> particular, when clang is used instead, there's (currently) no change
> in results in the errno.exp test since clang doesn't provide the
> necessary declaration(s) in its DWARF output.
>
> Perhaps even more compelling is being able to call functions like
> malloc() without having debug info for the C library.  To demonstrate
> this, I'll use the test program from gdb.base/break.exp.  After
> starting the program (and not letting debuginfod fetch GLIBC's
> symbols), an unpatched GDB will show:
>
>     (gdb) ptype malloc
>     type = <unknown return type> ()
>     (gdb) p malloc(4)
>     'malloc' has unknown return type; cast the call to its declared
>     return type
>
> However, with this commit, we now see:
>
>     (gdb) ptype malloc
>     type = void *(unsigned long)
>     (gdb) p malloc(4)
>     $1 = (void *) 0x4042a0
>
> This commit changes the name of read_func_scope in gdb/dwarf2/read.c
> to read_func_scope_or_decl, changing all callers.  I also added a
> comment for this function.
>
> It introduces a new function, die_is_func_decl_p and uses it in
> read_func_scope_or_decl().  If the call to die_is_func_decl_p()
> returns true, the code in read_func_scope_or_decl which attempts to
> get the function bounds is skipped and, after existing code which
> attempts to do some template related stuff happens, a new symbol with
> address class LOC_UNRESOLVED will be added.
>
> If just this change alone is made and regression testing is performed,
> there are quite a few regressions (well over 50, as I recall), mostly
> due to the fact that the PLT symbol / declaration is now found in
> various cases, perhaps ahead of the symbol for the function
> definition.  I'll go into depth regarding the various cases, below.
>
> Many of the regressions were fixed by making the LOC_UNRESOLVED case
> in language_defn::read_var_value in gdb/findvar.c prefer "normal"
> symbols over PLT symbols, though the PLT symbol will be used if no
> normal symbol is found.
>
> This change contains a (perhaps) surprising addition to deal with GNU
> ifunc symbols:
>
> 			if (bmsym.minsym->type () == mst_text_gnu_ifunc)
> 			  {
> 			    /* GNU ifunc code elsewhere in GDB depends
> 			       on the symbol's type being set as shown
> 			       below.  But, coming into this function,
> 			       VAR might have an arguably better type
> 			       obtained from a declaration, i.e.
> 			       DW_AT_declaration.  In this case, the
> 			       PLT (solib trampoline) symbol is
> 			       usually found first; see above.
> 			       Nevertheless, we change the type to
> 			       what the rest of GDB expects in order
> 			       for the rest of the GNU ifunc related
> 			       code in GDB to work.  */
> 			    type = builtin_type (objfile)
> 				     ->nodebug_text_gnu_ifunc_symbol;
> 			  }
>
> Hopefully, the comment adequately describes what this is about, but
> I'll note that without this particular bit of code, we see the
> following GNU ifunc related failures:
>
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: gdb-command<p (int) gnu_ifunc (3)>
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: gdb-command<p gnu_ifunc (3)>
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: p gnu_ifunc executing
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: p gnu_ifunc()
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: resolver received HWCAP
>
> There are 17 more, but they're essentially repeats of the above, with
> varying resolver_attr, resolver_debug, and final_debug cases.
>
> The change to info_address_command in gdb/printcmd.c forces execution
> into the minimal symbol lookup case when presented with a
> LOC_UNRESOLVED function symbol.  Without this change, there were 12
> falures in gdb.base/gnu-ifunc.exp, two of which look like this:
>
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: info addr gnu_ifunc
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: info sym <gnu_ifunc-address>
>
> The remaining failures are similar, only differing in the values
> for resolver_attr, resolver_debug, and final_debug.
>
> With regard to the failure itself, for the first one, the log output
> looks like this:
>
>     info addr gnu_ifunc
>     Symbol "gnu_ifunc" is static storage at address 0x7ffff7fbb389.
>     (gdb) FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=0:
>       final_debug=0: info addr gnu_ifunc
>
> The expected message from "info addr gnu_ifunc" was:
>
>     Symbol "gnu_ifunc" is at 0x7ffff7fbb389 in a file compiled without
>     debugging.
>
> I don't think that the FAILing message is wrong, but I think that the
> PASSing message (regarding being in a file without debugging) is more
> helpful to the user.
>
> It bothered me that the only tests which caught this problem were
> in gdb.base/gnu-ifunc.exp.  There is now an "info addr foo" test
> in the new test case gdb.dwarf2/func-decl.exp which also performs
> this test.
>
> With the above change in place, we then see these failures:
>
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1:
>       final_debug=0: info addr gnu_ifunc
>     FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=0: resolver_debug=1:
>       final_debug=1: info addr gnu_ifunc
>
> (There are two others for "info sym <gnu_ifunc-address>".)
>
> In each case, we now see a message like this...
>
>     Symbol "gnu_ifunc" is at 0x7ffff7fbb389 in a file compiled without
>     debugging.
>
> ...when we should in fact see:
>
>     Symbol "gnu_ifunc" is a function at address 0x7ffff7fbb389.
>
> Note that this is for the resolver_debug=1 case; for this case, the
> resolver library has symbols, so the latter message makes sense and
> the "failing" message is just plain wrong.
>
> These new failures are fixed by the change to
> lookup_global_or_static_symbol in gdb/symtab.c.  In this change,
> normal function symbols are preferred to those whose address class is
> LOC_UNRESOLVED.  I used a similar approach to that for
> language_defn::read_var_value, discussed earlier.
>
> Again, it seemed to me that there should be a non-gnu-ifunc test
> for this, so I added one; it'll be tested by:
>
>     gdb.dwarf2/func-decl.exp: lib_debug: info addr foo
>
> There were also regressions in gdb.base/info-fun.exp and
> gdb.mi/mi-sym-info.exp:
>
>     FAIL: gdb.base/info-fun.exp: n_flag=0: IN: info fun  foo
>     FAIL: gdb.base/info-fun.exp: n_flag=0: NO: info fun  foo
>     FAIL: gdb.base/info-fun.exp: n_flag=0: SEP: info fun  foo
>     FAIL: gdb.base/info-fun.exp: n_flag=1: IN: info fun -n foo
>     FAIL: gdb.base/info-fun.exp: n_flag=1: NO: info fun -n foo
>     FAIL: gdb.base/info-fun.exp: n_flag=1: SEP: info fun -n foo
>     FAIL: gdb.mi/mi-sym-info.exp: List all functions matching pattern f3
>       (unexpected output)
>
> For each of these failures, there was more output than expected.  For
> example, for one of the failing cases...
>
>     (gdb) info fun  foo
>     All functions matching regular expression "foo":
>
>     File .../gdb/testsuite/gdb.base/info-fun.c:
>     16:	int foo(void);
>
>     Non-debugging symbols:
>     0x0000000000400370  foo@plt
>     0x00007ffff7fbb389  foo
>     (gdb) FAIL: gdb.base/info-fun.exp: n_flag=0: NO: info fun  foo
>
> The "passing" output looks like this:
>
>     (gdb) info fun  foo
>     All functions matching regular expression "foo":
>
>     Non-debugging symbols:
>     0x0000000000400370  foo@plt
>     0x00007ffff7fbb389  foo
>     (gdb) PASS: gdb.base/info-fun.exp: n_flag=0: NO: info fun  foo
>
> At first glance, the "failing" output looks useful; perhaps it could
> be, but I'll note that the extra lines being output are for a
> declaration for a function which is not in the CU where the function
> is defined.  I have a hunch that we might be overwhelmed by extra
> output in a program with many libraries - it's conceivable that for
> some symbols, each library would have its own declaration.
>
> In any case, I was able to obtain the original / passing behavior
> by discarding LOC_UNRESOLVED symbols when searching in the
> function domain in global_symbol_searcher::add_matching_symbols.
>
> Finally, there were two regressions in gdb.base/completion.exp:
>
>     FAIL: gdb.base/completion.exp: complete break break.c:ma
>     FAIL: gdb.base/completion.exp: tab complete break break.c:ma (timeout)
>
> The log file for these failing tests is not especially helpful, but I
> debugged it by throwing a "gdb_interact" into the test to see what
> was going on.  As I recall, when trying to complete "break.c:ma",
> "marker1", "marker2", "marker3", "marker4", and "malloc" were all
> being found in addition to "main", which is what the what the testcase
> was expecting to be the sole completion.
>
> This problem was fixed by adjusting completion_skip_symbol in
> symtab.h.
>
> The new test case, gdb.dwarf2/func-decl.exp, contains, in addition to
> the tests already discussed, two tests which will fail in a GDB built
> without this commit and pass in a GDB built with it...
>
>     PASS: gdb.dwarf2/func-decl.exp: no_lib_debug:
>       gdb-command<print foo ("abc", 5)>
>     PASS: gdb.dwarf2/func-decl.exp: no_lib_debug: ptype foo
>
> The remaining tests in gdb.dwarf2/func-decl.exp should all pass in a
> GDB built with or without this commit.  They will only fail if one of
> the relevant changes discussed above is missing or becomes broken for
> some reason (perhaps due to some future change to this area of the
> code).
>
> Regarding the use of the DWARF assembler in the test...  Using some
> version(s) of GNU C, it's possible to write a test which causes a
> suitable declaration DIE to be placed in the DWARF output.  In fact, I
> originally wrote most of the new test without the DWARF assembler.
> But not all compilers do this, e.g. clang does not, and I wanted a
> test which would test this functionality regardless of whether the
> compiler generates the DWARF required for this test.
>
> I've tested on Fedora 42 w/ architectures x86_64, aarch64, riscv,
> s390x, and ppc64le.  On x86_64 Fedora 42, I've also tested with
> --target_board=unix/-m32, --target_board=native-gdbserver, and
> --target_board=native-extended-gdbserver.  No regressions found.
>
> After skimming version 1 of this commit, Tom Tromey suggested that
> there should also be changes to the indexer.  This version 2 commit
> adds that by making DW_TAG_subprogram declarations "interesting" to
> the indexer.  The changes which do this are in gdb/dwarf2/abbrev.c
> and gdb/dwarf2/cooked-indexer.c.  I also added a test to the new
> test case which attempts to do "ptype foo" prior to starting the
> program.  This failed when using version 1 of this commit, but
> passes now.

Thanks for the detailed explanation.  I took a first pass through the
changes and just has a couple of really minor thoughts.  I'll take a
deeper look next week.

>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31563
> ---
>  gdb/dwarf2/abbrev.c                      |   3 +
>  gdb/dwarf2/cooked-indexer.c              |   5 +
>  gdb/dwarf2/read.c                        | 108 ++++++++++----
>  gdb/findvar.c                            |  46 +++++-
>  gdb/printcmd.c                           |   4 +-
>  gdb/symtab.c                             |  43 ++++--
>  gdb/symtab.h                             |  20 ++-
>  gdb/testsuite/gdb.dwarf2/func-decl-lib.c |  24 +++
>  gdb/testsuite/gdb.dwarf2/func-decl.c     |  35 +++++
>  gdb/testsuite/gdb.dwarf2/func-decl.exp   | 182 +++++++++++++++++++++++
>  10 files changed, 427 insertions(+), 43 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dwarf2/func-decl-lib.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/func-decl.c
>  create mode 100644 gdb/testsuite/gdb.dwarf2/func-decl.exp
>
> diff --git a/gdb/dwarf2/abbrev.c b/gdb/dwarf2/abbrev.c
> index 5cfff69cc3b..2ca80ab8a6a 100644
> --- a/gdb/dwarf2/abbrev.c
> +++ b/gdb/dwarf2/abbrev.c
> @@ -240,6 +240,9 @@ abbrev_table::read (struct dwarf2_section_info *section,
>  	     the correct scope.  */
>  	  cur_abbrev->interesting = true;
>  	}
> +      else if (has_hardcoded_declaration
> +	       && cur_abbrev->tag == DW_TAG_subprogram)
> +	cur_abbrev->interesting = true;
>        else if (has_hardcoded_declaration
>  	       && (cur_abbrev->tag != DW_TAG_variable || !has_external))
>  	cur_abbrev->interesting = false;
> diff --git a/gdb/dwarf2/cooked-indexer.c b/gdb/dwarf2/cooked-indexer.c
> index c093984bae0..710ef82ed0d 100644
> --- a/gdb/dwarf2/cooked-indexer.c
> +++ b/gdb/dwarf2/cooked-indexer.c
> @@ -301,6 +301,11 @@ cooked_indexer::scan_attributes (dwarf2_per_cu *scanning_per_cu,
>  	   || abbrev->tag == DW_TAG_namespace)
>  	  && abbrev->has_children)
>  	*flags |= IS_TYPE_DECLARATION;
> +      else if (abbrev->tag == DW_TAG_subprogram)
> +	{
> +	  /* We want to index function declarations - do nothing in order
> +	     to avoid nulling out *name, below.  */
> +	}

I wonder if the comment a few lines up, that looks like:

  /* We don't want to examine declarations, but if we found a
     declaration when handling DW_AT_specification or the like, then
     that is ok.  Similarly, we allow an external variable without a
     location; those are resolved via minimal symbols.  */

is now out of date after this change.  I don't really understand the
DW_AT_specification part though so I cannot really suggest
improvements...

>        else
>  	{
>  	  *linkage_name = nullptr;


> diff --git a/gdb/testsuite/gdb.dwarf2/func-decl.exp b/gdb/testsuite/gdb.dwarf2/func-decl.exp
> new file mode 100644
> index 00000000000..316dfe8d04c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/func-decl.exp
> @@ -0,0 +1,182 @@
> +# Copyright 2025 Free Software Foundation, Inc.
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# Test GDB's ability to access declarations for function symbols.  On
> +# Linux, using GCC, compiling the main program with -g and the shared
> +# lib source file without -g can be used to create a compelling test
> +# case without needing the DWARF assembler.  However, we don't want to
> +# count on the fact that the compiler will place a declaration for the
> +# shared library function in the DWARF info for the main program.  (E.g.
> +# when using CLANG/LLVM, these DIEs are omitted.)  Therefore, we use the
> +# DWARF assembler to create the requisite DWARF info for this test.
> +
> +load_lib dwarf.exp
> +require dwarf2_support
> +
> +standard_testfile .c -dw.S
> +
> +set asm_file [standard_output_file $srcfile2]
> +set libsrc "${srcdir}/${subdir}/${testfile}-lib.c"
> +set libobj [standard_output_file "${testfile}-lib.so"]
> +
> +# We need to know the size of integer and address types in order to
> +# write some of the debugging info we'd like to generate.
> +#
> +# For that, we ask GDB by debugging our test program.  Any program
> +# would do, but since we already have program written specifically for
> +# this testcase, we might as well use that.  Note that we need to
> +# also build the shared library that the test program uses.
> +
> +set session_options [list debug shlib=${libobj}]
> +if { [gdb_compile_shlib $libsrc $libobj [list nodebug]] != "" } {
> +    untested "failed to compile shared object"
> +    return -1
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${binfile} \
> +	  [list $srcfile] $session_options] } {
> +    return -1
> +}
> +
> +with_test_prefix "first session" {
> +    if ![runto_main] {
> +	return
> +    }
> +
> +    with_shared_gdb {
> +	# Rather than start a new session, declare the current session the
> +	# shared one.  Otherwise, get_func_info would compile an executable
> +	# in a temp dir; due to implementation details, this means that the
> +	# shared lib won't be found.
> +	share_gdb ${srcdir}/${subdir}/$srcfile $session_options
> +
> +	get_func_info main $session_options
> +
> +	# Using the running GDB session, determine sizes of several types.
> +	set int_size [get_sizeof "int" -1]
> +	set char_ptr_size [get_sizeof "char *" 8]
> +    }
> +}
> +
> +Dwarf::assemble $asm_file {
> +    cu {} {
> +	DW_TAG_compile_unit {
> +		{DW_AT_language @DW_LANG_C99}
> +		{DW_AT_name $::srcfile}
> +	} {
> +	    declare_labels int_label char_label char_ptr_label
> +
> +	    int_label: DW_TAG_base_type {
> +		{DW_AT_byte_size ${::int_size} DW_FORM_udata}
> +		{DW_AT_encoding @DW_ATE_signed}
> +		{DW_AT_name "int"}
> +	    }
> +
> +	    char_label: DW_TAG_base_type {
> +		{byte_size 1 sdata}
> +		{encoding @DW_ATE_signed_char}
> +		{name "char"}
> +	    }
> +
> +	    char_ptr_label: DW_TAG_pointer_type {
> +		{DW_AT_byte_size ${::char_ptr_size} DW_FORM_sdata}
> +		{DW_AT_type :$char_label}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{DW_AT_external 1 flag}
> +		{DW_AT_name foo}
> +		{DW_AT_prototyped 1 DW_FORM_flag_present}
> +		{DW_AT_type :$int_label}
> +		{DW_AT_declaration 1 flag}
> +	    } {
> +		DW_TAG_formal_parameter {
> +		    {DW_AT_type :$char_ptr_label}
> +		}
> +		DW_TAG_formal_parameter {
> +		    {DW_AT_type :$int_label}
> +		}
> +	    }
> +
> +	    DW_TAG_subprogram {
> +		{DW_AT_name main}
> +		{DW_AT_low_pc ${::main_start} DW_FORM_addr}
> +		{DW_AT_high_pc ${::main_end} DW_FORM_addr}
> +		{DW_AT_type :$int_label}
> +	    }
> +	}
> +    }
> +}
> +
> +# Test against a shared library built with no debugging symbols.  Due
> +# to the DWARF info provided by the DWARF assembler above, there will
> +# be a declaration for the shared lib symbol "foo" in the main
> +# program.  Thus, due to the lack of DWARF info in the shared library,
> +# GDB can't know the type from the shared library.  Instead, it must
> +# rely on the declaration of foo from the main program.
> +#
> +# Due to that declaration, it should be possible to examine its type
> +# as well as make an inferior function call.  We expect "info addr foo"
> +# to provide the address of the actual function instead of foo's PLT
> +# in the main program.
> +
> +with_test_prefix no_lib_debug {
> +    if { [gdb_compile_shlib $libsrc $libobj [list nodebug]] != "" } {
> +	untested "failed to compile shared object"
> +	return -1
> +    }
> +
> +    if { [prepare_for_testing "failed to prepare" ${binfile} \
> +	      [list $srcfile $asm_file] [list nodebug shlib=${libobj}]] } {
> +	return -1
> +    }
> +
> +    with_test_prefix "before program start" {
> +	# Verify that the type of foo is available prior to starting
> +	# the program.
> +	gdb_test "ptype foo" "^type = int \\(char \\*, int\\)"
> +    }
> +
> +    clean_restart $binfile

Is this clean_restart really needed?  I suspect not.

Thanks,
Andrew