RE: [PATCH v4 1/4] gdb, dwarf: add support for DW_AT_trampoline in DWARF reader

["Ijaz, Abdul B" <abdul.b.ijaz@intel.com>]

Hi Tom,

Thanks for the feedback.

Tom> I didn't realize this until starting to read patch 2, but it seems strange to use the name here.  It seems to me it would be better to use the address attached to the referenced subroutine DIE The "bad"
scenario would be where there are multiple functions with the same name here, and gdb's symbol lookup returns the wrong one

For this case of  value of attributes for this DIE may contain mangled name of the target. So for such cases we are preferring the linkage name to store. Shouldn't it cover such issues. Otherwise let me know then I may update accordingly.
As per dwarf spec: "
If the value is of class string, then the value is the (possibly mangled) name  of the target subprogram.
"

Thanks & Best Regards
Abdul Basit

-----Original Message-----
From: Tom Tromey <tom@tromey.com> 
Sent: Wednesday, August 2, 2023 10:13 PM
To: Abdul Basit Ijaz via Gdb-patches <gdb-patches@sourceware.org>
Cc: Ijaz, Abdul B <abdul.b.ijaz@intel.com>; JiniSusan.George@amd.com; tom@tromey.com; eliz@gnu.org; blarsen@redhat.com; Nils-Christian Kempke <nils-christian.kempke@intel.com>
Subject: Re: [PATCH v4 1/4] gdb, dwarf: add support for DW_AT_trampoline in DWARF reader

>>>>> "Abdul" == Abdul Basit Ijaz via Gdb-patches <gdb-patches@sourceware.org> writes:

Abdul> +      else if (attr->form_is_ref ())
Abdul> +	{
Abdul> +	  die_info *target_die;
Abdul> +	  dwarf2_cu *target_cu = cu;
Abdul> +
Abdul> +	  target_die = follow_die_ref (die, attr, &target_cu);
Abdul> +	  const char *target_name;
Abdul> +	  /* Prefer the mangled name; otherwise compute the demangled one.  */
Abdul> +	  target_name = dw2_linkage_name (target_die, target_cu);
Abdul> +	  if (target_name == NULL)
Abdul> +	    target_name = dwarf2_physname (NULL, target_die, target_cu);
Abdul> +	  if (target_name == nullptr)
Abdul> +	    {
Abdul> +	      complaint (_("DW_AT_trampoline target DIE has no name for"
Abdul> +			 "referencing DIE %s [in module %s]"),
Abdul> +			 sect_offset_str (die->sect_off),
Abdul> +			 objfile_name (objfile));
Abdul> +	    }
Abdul> +	  TYPE_TRAMPOLINE_TARGET (ftype)->set_target_name 
Abdul> +(target_name);

I didn't realize this until starting to read patch 2, but it seems strange to use the name here.  It seems to me it would be better to use the address attached to the referenced subroutine DIE.  The "bad"
scenario would be where there are multiple functions with the same name here, and gdb's symbol lookup returns the wrong one.  You can normally trigger these bugs by carefully ordering shared libraries.

Tom
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928