[PATCH v5 3/5] objdump: Add variable types and structure for DWARF info parsing

[Guillaume VACHERIAS - foss]

> On 26.09.2025 14:22, Guillaume VACHERIAS wrote:
> > @@ -2569,41 +2593,517 @@ display_lang (uint64_t uvalue)
> >      }
> >  }
> >
> > +static generic_type *
> > +get_or_create_generic_base_type (uint64_t die_offset)
> > +{
> > +  if (base_type_list.tail == NULL
> > +      || base_type_list.tail->die_type.base_type.die_offset != die_offset)
> > +    {
> > +      generic_type *ret = xmalloc (sizeof (*ret));
> 
> Isn't it potentially pretty wasteful to allocate a generic_type here
> (and elsewhere) when all you need is a base_type? You may, after all, be
> allocating very many of them.

generic_type is easier to manipulate. In the patch 4/5 where the linking
mechanism is introduced generic_type abstracts the need to determine
which type is linked to another, letting the linking mechanism be one
line assignment only in the function do_link_variable_information in patch
4/5.
> 
> > +      base_type bt = {die_offset, 0, 0, NULL};
> > +      ret->type = BASE_TYPE;
> > +      ret->die_type.base_type = bt;
> > +      ret->linked = false;
> > +      ret->next = NULL;
> > +      if (base_type_list.head == NULL)
> > +	{
> > +	  base_type_list.head = ret;
> > +	  base_type_list.tail = base_type_list.head;
> 
> Better write NULL here explicitly (and then similarly elsewhere)?

You meant ret ? In that case, yes, I'll correct it this way.
> 
> > --- a/binutils/dwarf.h
> > +++ b/binutils/dwarf.h
> > @@ -71,6 +71,174 @@ typedef struct
> >  }
> >  DWARF2_Internal_ARange;
> >
> > +/* Forward declaration */
> > +typedef struct generic_type generic_type;
> > +typedef struct base_type base_type;
> > +typedef struct type_def type_def;
> > +typedef struct enum_constant enum_constant;
> > +typedef struct enum_type enum_type;
> > +typedef struct subrange_type subrange_type;
> > +typedef struct tab_type tab_type;
> > +typedef struct member_type member_type;
> > +typedef struct member_parent member_parent;
> > +typedef struct type_ref type_ref;
> > +typedef struct variable_type variable_type;
> > +
> > +/* Container for map info information to be generated.  */
> > +struct base_type
> > +{
> > +  uint64_t die_offset;
> 
> This field existing in all struct types, better move it to generic_type?

Yes, I'll include it in generic_type.
> 
> > +  uint64_t usize;
> > +  int64_t ssize;
> 
> How come these aren't members of a union? They can't both apply at the
> same time, can they? (Same question then applies elsewhere.)

It could definitely be members of a union, I'll make the changes.
> 
> > +  const char *name;
> > +};
> > +
> > +struct type_def
> > +{
> > +  uint64_t die_offset;
> > +  uint64_t ptr_die_offset;
> > +  const char *name;
> > +
> > +  /* Field where information retrievable by resolving the type.  */
> > +  generic_type *ptr_type;
> 
> I fear I can't really parse the comment. Is there perhaps a word missing?

This field will be set during the linking mechanism, when we resolve
the type of a variable. I will add a N.B. to clarify this.
> 
> > +struct generic_type
> > +{
> > +  enum
> > +    {
> > +      BASE_TYPE,
> > +      TYPE_DEF,
> > +      ENUM_TYPE,
> > +      TAB_TYPE,
> > +      MEMBER_PARENT,
> > +      MEMBER_TYPE,
> > +      TYPE_REF,
> > +      VARIABLE_TYPE
> > +    } type;
> > +  union
> > +    {
> > +      base_type     base_type;
> > +      type_def      type_def;
> > +      enum_type     enum_type;
> > +      tab_type      tab_type;
> > +      member_parent member_parent;
> > +      member_type   member_type;
> > +      type_ref      type_ref;
> > +      variable_type variable_type;
> > +    } die_type;
> > +  bool linked; /* Treat nested structure.  */
> 
> This field is only ever set to false in this patch, and hence it's not really
> becoming clear what it's good for. The comment sadly also doesn't help.

I will definitely add a N.B. here to clarify its intent!
> 
> Also the field could be placed better to avoid unnecessary padding holes on
> (at least) 64-bit architectures (where there already is a padding hole of 4
> bytes between type and die_type).
> 
> Before you (and I) put any more work into this, I have to ask a more general
> question (Cc-ing Nick for this reason): This is a lot of new code (~1.600
> lines) for something that I'm not really sure belongs in objdump. What you
> do here goes beyond mere "dumping", imo.

I understand your concerns regarding the size of the changes, and I agree that
the functionality does go beyond "dumping" as objdump is typically used for.

That said, I'm not entirely sure where else this code would best fit within the
binutils-gdb. In the LLVM ecosystem, this feature would fit in llvm-dwarfdump,
but as far as I know, there isn't an equivalent standalone utility in GNU. After
considering the available options, objdump seemed like the most suitable place
for this functionality as it manipulates DWARF information. However, do not
hesitate to suggest a more suitable tool in the binutils-gdb suite !
 
Guillaume