RE: [PATCH 1/3] gdb: add support for DW_AT_trampoline in DWARF reader

["Kempke, Nils-Christian" <nils-christian.kempke@intel.com>]

Hi Jini,

Thanks for the feedback!

I have a few questions regarding your comments as I inlined below.

Cheers,
Nils

> -----Original Message-----
> From: George, Jini Susan <JiniSusan.George@amd.com>
> Sent: Wednesday, July 13, 2022 9:40 AM
> To: Kempke, Nils-Christian <nils-christian.kempke@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH 1/3] gdb: add support for DW_AT_trampoline in DWARF
> reader
> 
> [Public]
> 
> Hi Nils,
> 
> Thanks for this. A few questions/ comments.
> 
> * Is there any common ground we can find with the way solib (linker PLT)
> trampolines are handled ? I am OK if we can't, but I think it might be better to
> check and rule it out.

I don't quite understand what you mean here but am also not super familiar
with the solib handling part of GDB.  The attached patches treat any function
that was marked as DW_AT_Trampoline in DWARF by the compiler.  As far as I
know not a single compiler (except ifx) emits this attribute up until now.  If a
compiler were to generate DWARF for the solib trampolines and if it were to
add the attribute, we could also treat solib trampolines with this approach.
But generally, the DW_AT_trampoline is independent of any other GDB
trampoline handling.
Not sure whether this answers your question.  Maybe you can elaborate a bit
more?

> 
> * Also, do we want to resolve and display the target of the trampoline code
> (if possible) in the disassembled code also ?
> 

So you mean, when I am about to step into a wrapper/step through a wrapper
I would display the target at the respective call statement with some note like

(gdb) disassemble
...
0x080483fa <+0>: push %ebp
0x080483fb <+1>: mov %esp,%ebp
0x080483fd <+3>: sub $0x8,%esp
=> 0x08048400 <+6>: movl $0x2,0x4(%esp)
0x08048408 <+14>: movl $0x1,(%esp)
0x0804840f <+21>: call 0x80483ed <func> (trampoline to: 0x12345 <other>)

?
So a change in the pretty_printer for assembly I assume (or somewhere deeper).

This would mean, GDB would have to check every time a call appears
somewhere in the disassembly whether or not the target is a trampoline, and,
if so, GDB would need to resolve this.

Generally, I think this might be a nice feature.  I'll have a look whether it can be
done without too much effort, but if it turns out to be considerably difficult I'd
rather like to leave this for a different patch.
If this is already done in the other trampoline cases (solibs and signals) it might
be needed for the completion of this patch though.

> * Also, do we want to refer to these trampolines differently so that these
> can be distinguished from the linker PLT trampolines and the signal
> trampolines ?
> 

I hoped that the patches made the difference clear enough.  Or can you
pinpoint something that seems unclear?  How would you instead refer to
them, I am a bit lost here, to be honest.  They are, after all, according to
their DWARF/debug info trampolines.  Referring to them differently would
in my opinion maybe obscure things?  But maybe you had something in mind.

> * In read.c, in read_subroutine_type(), where the DW_AT_trampoline
> attribute is of form reference, we seem to be storing the value
> corresponding to the  DW_AT_name attribute, whereas in many cases (C++
> and other languages), it might make sense to store the mangled name
> typically obtained from the DW_AT_linkage_name attribute. Given this it
> might be good to have a C++ test case for this with trampoline target
> functions inside namespaces also. Maybe with the same target function
> name under multiple namespaces. Also, the comment in gdbtypes.h (Line
> 1883) mentions mangled name, which does not seem to match ?

Yes, I think initially the comment was intended to only describe the
DW_AT_trampoline when its FORM is string.  That string could have been
both, mangled and demangled.  But I get your point.  I am not sure whether
storing the linkage name or the 'normal' name is correct here.  I think these are
two different issues: which name should be stored and should it be
demangled or not.  But I'll check this again.  I guess I'll have to check symbol
lookup again for this and see what makes most sense.

> 
> * In gdbtypes.h:
> 
> 1817
> 1818     unsigned int is_trampoline : 1;
> 
> Might be better to position this field as the 3rd field for better structure
> packing ?

The structure was poorly packed in the first place I think.  We have the
ENUM_BITFIELD in the beginning which can be either an enum or an unsigned
int.  I am not sure whether the size of an enum is defined somewhere but I'd
assume it is compiler dependent and usually the size of an int?
This is followed by another unsigned int and then by two pointers.

If we aimed at proper structure packing I'd assume the pointers (which would
usually be 8 bytes to all come first, then the (unsigned) int's with their (usually)
4 bytes, followed finally by the enum?

I considered this when implementing it but it seemed like an unrelated change
to me.  Assuming int is 4 bytes and we align to 8 bytes moving is_trampoline to
the 3rd position would also not do anything, would it?  We'd be at

4 byte
4 byte
4 byte
8 byte
8 byte
8 byte

Instead of 

4 byte
4 byte
8 byte
8 byte
4 byte
8 byte

I think switching is_trampoline and self_trampoline_target would make the
most sense here? (without touching the other members).

> * infrun.c: Line 7156: It might be better to have the '10' representing the
> number of chained trampoline traversals allowed as a macro.
> 

I also thought about this when implementing it.  I back then decided against it
but sure, I'll add that.

> A few nits below:
> * infrun.c : Nits:
>                Line 7154: spelling: certian
>          Line 9941: spelling : fucntion
> 
> *symtab.h: Nit: line 2247: spelling: wether

Thanks!
I'll change this in v2!

> 
> Thanks,
> Jini.
> 
> >>-----Original Message-----
> >>From: Gdb-patches <gdb-patches-
> >>bounces+jigeorge=amd.com@sourceware.org> On Behalf Of Nils-
> Christian
> >>Kempke via Gdb-patches
> >>Sent: Thursday, July 7, 2022 5:46 PM
> >>To: gdb-patches@sourceware.org
> >>Subject: [PATCH 1/3] gdb: add support for DW_AT_trampoline in DWARF
> reader
> >>
> >>[CAUTION: External Email]
> >>
> >>DW_AT_trampoline can be used to describe compiler generated functions
> that
> >>serve some intermediary purpose on making a call to another function.  A
> >>compiler can emit this tag in order to help a debugger hide the trampolines
> from
> >>a user.
> >>
> >>The attribute is only applicable to DW_TAG_subroutine and
> >>DW_TAG_inlined_subroutines tags.  It contains information about the
> >>trampoline target either as a reference to its DIE, as its address or its
> name.
> >>DW_AT_trampoline can also be a flag indicating that the DIE is a trampoline
> or
> >>not but not specifying the target if it is unknown.
> >>
> >>This patch adds support to GDB for reading the DW_AT_trampoline
> attribute.  It
> >>stores the attribute and its value in the type_specific part of a GDB type.
> This
> >>patch is done in preparation of the following patches which will add a
> >>mechanism to hide DW_AT_trampoline subroutines from the user.
> >>
> >>gdb/ChangeLog:
> >>2022-05-22  Nils-Christian Kempke  <nils-christian.kempke@intel.com>
> >>
> >>        * dwarf2/read.c (read_subroutine_type): Read in DW_AT_trampoline.
> >>        * gdbtypes.c (print_trampoline_target_info): New helper method.
> >>        (recursive_dump_type): Add trampoline info.
> >>        (copy_type_recursive): Copy trampoline info.
> >>        * gdbtypes.h (func_type): Add trampoline specific fields.
> >>        (trampoline_target_kind): New enum.
> >>        (trampoline_target): New struct for storing DW_AT_trampoline's
> >>        value.
> >>        (TYPE_IS_TRAMPOLINE): New define.
> >>        (TYPE_TRAMPOLINE_TARGET): New define.
> >>
> >>Signed-off-by: Nils-Christian Kempke <nils-christian.kempke@intel.com>
> >>---
> >> gdb/dwarf2/read.c | 43 ++++++++++++++++++++++++++
> >> gdb/gdbtypes.c    | 31 +++++++++++++++++++
> >> gdb/gdbtypes.h    | 79
> >>+++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 153 insertions(+)
> >>
> >>diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index
> >>23fe5679cbd..7f80c31d051 100644
> >>--- a/gdb/dwarf2/read.c
> >>+++ b/gdb/dwarf2/read.c
> >>@@ -16491,6 +16491,49 @@ read_subroutine_type (struct die_info *die,
> struct
> >>dwarf2_cu *cu)
> >>   if (prototyped_function_p (die, cu))
> >>     ftype->set_is_prototyped (true);
> >>
> >>+  /* If this is a trampoline function store it and its target here.  */
> >>+ attr = dwarf2_attr (die, DW_AT_trampoline, cu);  if (attr != nullptr)
> >>+    {
> >>+      CORE_ADDR baseaddr = objfile->text_section_offset ();
> >>+
> >>+      TYPE_IS_TRAMPOLINE (ftype) = true;
> >>+      TYPE_TRAMPOLINE_TARGET (ftype)
> >>+       = (trampoline_target *) TYPE_ZALLOC (ftype,
> >>+                                            sizeof
> >>+ (trampoline_target));
> >>+
> >>+      /* A DW_AT_trampoline can be either an address, a flag, a reference
> or a
> >>+        string.  */
> >>+      if (attr->form_is_string ())
> >>+       TYPE_TRAMPOLINE_TARGET (ftype)->set_target_physname
> >>+         (attr->as_string ());
> >>+      else if (attr->form_is_ref ())
> >>+       {
> >>+         die_info *target_die;
> >>+         dwarf2_cu *target_cu = cu;
> >>+
> >>+         target_die = follow_die_ref (die, attr, &target_cu);
> >>+         const char* target_name = dwarf2_name (target_die, target_cu);
> >>+         if (target_name == nullptr)
> >>+           {
> >>+             complaint (_("DW_AT_trampoline target DIE has no name for"
> >>+                        "referencing DIE %s [in module %s]"),
> >>+                        sect_offset_str (die->sect_off),
> >>+                        objfile_name (objfile));
> >>+           }
> >>+         TYPE_TRAMPOLINE_TARGET (ftype)->set_target_physname
> >>(target_name);
> >>+       }
> >>+      else if (attr->form_is_unsigned ())
> >>+       TYPE_TRAMPOLINE_TARGET (ftype)->set_target_flag (attr-
> >as_boolean ());
> >>+      else
> >>+       {
> >>+         CORE_ADDR target_addr = attr->as_address ();
> >>+         target_addr = gdbarch_adjust_dwarf2_addr (objfile->arch (),
> >>+                                                   target_addr + baseaddr);
> >>+         TYPE_TRAMPOLINE_TARGET (ftype)->set_target_physaddr
> (target_addr);
> >>+       }
> >>+    }
> >>+
> >>   /* Store the calling convention in the type if it's available in
> >>      the subroutine die.  Otherwise set the calling convention to
> >>      the default value DW_CC_normal.  */ diff --git a/gdb/gdbtypes.c
> >>b/gdb/gdbtypes.c index c8f98554859..66e04882d48 100644
> >>--- a/gdb/gdbtypes.c
> >>+++ b/gdb/gdbtypes.c
> >>@@ -5205,6 +5205,31 @@ print_fixed_point_type_info (struct type
> *type, int
> >>spaces)
> >>              type->fixed_point_scaling_factor ().str ().c_str ());  }
> >>
> >>+/* Print the contents of the TYPE's self_trampoline_target, assuming that
> its
> >>+   type-specific kind is TYPE_SPECIFIC_FUNC and is_trampoline is not 0.
> >>+*/ static void print_trampoline_target_info (struct type *type, int
> >>+spaces) {
> >>+  switch (TYPE_TRAMPOLINE_TARGET (type)->target_kind ())
> >>+    {
> >>+    case TRAMPOLINE_TARGET_PHYSADDR:
> >>+      gdb_printf ("%*starget physaddr: 0x%lx\n", spaces + 2, "",
> >>+                 TYPE_TRAMPOLINE_TARGET (type)->target_physaddr ());
> >>+      break;
> >>+    case TRAMPOLINE_TARGET_PHYSNAME:
> >>+      gdb_printf ("%*starget physname: %s\n", spaces + 2, "",
> >>+                 TYPE_TRAMPOLINE_TARGET (type)->target_physname ());
> >>+      break;
> >>+    case TRAMPOLINE_TARGET_FLAG:
> >>+      gdb_printf ("%*starget flag: %d\n", spaces + 2, "",
> >>+                 TYPE_TRAMPOLINE_TARGET (type)->target_flag ());
> >>+      break;
> >>+    default:
> >>+      gdb_assert_not_reached ("unhandled trampoline target kind");
> >>+      break;
> >>+    }
> >>+}
> >>+
> >> static struct obstack dont_print_type_obstack;
> >>
> >> /* Print the dynamic_prop PROP.  */
> >>@@ -5531,6 +5556,10 @@ recursive_dump_type (struct type *type, int
> spaces)
> >>        gdb_printf ("%*scalling_convention %d\n", spaces, "",
> >>                    TYPE_CALLING_CONVENTION (type));
> >>        /* tail_call_list is not printed.  */
> >>+       gdb_printf ("%*sis_trampoline %d\n", spaces, "",
> >>+                   TYPE_IS_TRAMPOLINE (type));
> >>+       if (TYPE_IS_TRAMPOLINE (type))
> >>+         print_trampoline_target_info (type, spaces);
> >>        break;
> >>
> >>       case TYPE_SPECIFIC_SELF_TYPE:
> >>@@ -5762,6 +5791,8 @@ copy_type_recursive (struct objfile *objfile,
> >>       TYPE_CALLING_CONVENTION (new_type) =
> TYPE_CALLING_CONVENTION
> >>(type);
> >>       TYPE_NO_RETURN (new_type) = TYPE_NO_RETURN (type);
> >>       TYPE_TAIL_CALL_LIST (new_type) = NULL;
> >>+      TYPE_IS_TRAMPOLINE (new_type) = TYPE_IS_TRAMPOLINE (type);
> >>+      TYPE_TRAMPOLINE_TARGET (new_type) =
> TYPE_TRAMPOLINE_TARGET
> >>+ (type);
> >>       break;
> >>     case TYPE_SPECIFIC_FLOATFORMAT:
> >>       TYPE_FLOATFORMAT (new_type) = TYPE_FLOATFORMAT (type); diff --
> git
> >>a/gdb/gdbtypes.h b/gdb/gdbtypes.h index 7437e1db8ab..846de1289ad
> 100644
> >>--- a/gdb/gdbtypes.h
> >>+++ b/gdb/gdbtypes.h
> >>@@ -1811,6 +1811,83 @@ struct func_type
> >>        contains the method.  */
> >>
> >>     struct type *self_type;
> >>+
> >>+    /* * For functions marked with the DW_AT_trampoline.  These are
> >>+       compiler generated wrappers that should be hidden from the user.
> >>+ */
> >>+
> >>+    unsigned int is_trampoline : 1;
> >>+
> >>+    struct trampoline_target *self_trampoline_target;  };
> >>+
> >>+/* The kind of location held by this call site target.  */ enum
> >>+trampoline_target_kind
> >>+  {
> >>+    /* An address.  */
> >>+    TRAMPOLINE_TARGET_PHYSADDR,
> >>+    /* A (mangled) name.  */
> >>+    TRAMPOLINE_TARGET_PHYSNAME,
> >>+    /* An flag (target is unknown).  */
> >>+    TRAMPOLINE_TARGET_FLAG,
> >>+  };
> >>+
> >>+struct trampoline_target
> >>+  {
> >>+    trampoline_target_kind target_kind () const
> >>+    {
> >>+      return m_target_kind;
> >>+    }
> >>+
> >>+    void set_target_physaddr (CORE_ADDR physaddr)
> >>+    {
> >>+      m_target_kind = TRAMPOLINE_TARGET_PHYSADDR;
> >>+      m_trampoline_target.physaddr = physaddr;
> >>+    }
> >>+
> >>+    CORE_ADDR target_physaddr () const
> >>+    {
> >>+      gdb_assert (m_target_kind == TRAMPOLINE_TARGET_PHYSADDR);
> >>+      return m_trampoline_target.physaddr;
> >>+    }
> >>+
> >>+    void set_target_physname (const char *physname)
> >>+    {
> >>+      m_target_kind = TRAMPOLINE_TARGET_PHYSNAME;
> >>+      m_trampoline_target.physname = physname;
> >>+    }
> >>+
> >>+    const char *target_physname () const
> >>+    {
> >>+      gdb_assert (m_target_kind == TRAMPOLINE_TARGET_PHYSNAME);
> >>+      return m_trampoline_target.physname;
> >>+    }
> >>+
> >>+    void set_target_flag (bool flag)
> >>+    {
> >>+      m_target_kind = TRAMPOLINE_TARGET_FLAG;
> >>+      m_trampoline_target.flag = flag;
> >>+    }
> >>+
> >>+    bool target_flag () const
> >>+    {
> >>+      gdb_assert (m_target_kind == TRAMPOLINE_TARGET_FLAG);
> >>+      return m_trampoline_target.flag;
> >>+    }
> >>+
> >>+  private:
> >>+
> >>+    union
> >>+    {
> >>+      /* Address.  */
> >>+      CORE_ADDR physaddr;
> >>+      /* Mangled name.  */
> >>+      const char *physname;
> >>+      /* Flag.  */
> >>+      bool flag;
> >>+    } m_trampoline_target;
> >>+
> >>+    /* * Discriminant for union m_trampoline_target.  */
> >>+    ENUM_BITFIELD (trampoline_target_kind) m_target_kind : 2;
> >>   };
> >>
> >> /* struct call_site_parameter can be referenced in callees by several
> ways.  */
> >>@@ -2172,6 +2249,8 @@ extern void set_type_vptr_basetype (struct
> type *,
> >>struct type *);  #define TYPE_CALLING_CONVENTION(thistype)
> >>TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff-
> >calling_convention
> >> #define TYPE_NO_RETURN(thistype) TYPE_MAIN_TYPE(thistype)-
> >>>type_specific.func_stuff->is_noreturn
> >> #define TYPE_TAIL_CALL_LIST(thistype) TYPE_MAIN_TYPE(thistype)-
> >>>type_specific.func_stuff->tail_call_list
> >>+#define TYPE_IS_TRAMPOLINE(thistype)
> >>+TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->is_trampoline
> >>+#define TYPE_TRAMPOLINE_TARGET(thistype)
> >>+TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff-
> >self_trampoline_tar
> >>+get
> >> #define TYPE_BASECLASS(thistype,index) ((thistype)->field (index).type
> ())
> >>#define TYPE_N_BASECLASSES(thistype) TYPE_CPLUS_SPECIFIC(thistype)-
> >>>n_baseclasses
> >> #define TYPE_BASECLASS_NAME(thistype,index) (thistype->field
> (index).name ())
> >>--
> >>2.25.1
> >>
> >>Intel Deutschland GmbH
> >>Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
> >>Tel: +49 89 99 8853-0,
> >>https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw
> ww.int
> >>el.de%2F&amp;data=05%7C01%7CJiniSusan.George%40amd.com%7C9c3f
> 28b5
> >>98cf49f888f908da601272a0%7C3dd8961fe4884e608e11a82d994e183d%7C0
> %7
> >>C0%7C637927929632561872%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLj
> >>AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C
> >>%7C&amp;sdata=MBaQ7XpSZ4XFO963dnxF%2Fy1PD0gcx6k%2FjdaifxoSUa
> Y%3D
> >>&amp;reserved=0
> >><https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw
> ww.in
> >>tel.de%2F&amp;data=05%7C01%7CJiniSusan.George%40amd.com%7C9c3
> f28b5
> >>98cf49f888f908da601272a0%7C3dd8961fe4884e608e11a82d994e183d%7C0
> %7
> >>C0%7C637927929632561872%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC
> 4wLj
> >>AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C
> >>%7C&amp;sdata=MBaQ7XpSZ4XFO963dnxF%2Fy1PD0gcx6k%2FjdaifxoSUa
> Y%3D
> >>&amp;reserved=0>
> >>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

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