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

[Jan Beulich]

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.

> +      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)?

> --- 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?

> +  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.)

> +  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?

> +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.

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.

Jan