[PATCH v3 4/5] objdump: Link and display all variables information
Jan Beulich
jbeulich@suse.com
Wed Sep 3 12:49:45 GMT 2025
On 01.09.2025 19:50, Guillaume VACHERIAS wrote:
> @@ -2599,6 +2617,16 @@ display_lang (uint64_t uvalue)
> }
> }
>
> +static void
> +display_base_type (base_type *bt)
All of the display functions would better have pointer-to-const parameters.
> +{
> + printf ("type: %s, size: ", bt->name);
> + if (bt->usize != 0)
> + printf ("0x%lx\n", bt->usize);
> + else if (bt->ssize != 0)
> + printf ("0x%lx\n", bt->ssize);
Here and below - please prefer %#lx (and alike) over 0x%lx. (Note though
that this doesn't apply when a field width is also specified.)
> @@ -2631,6 +2665,17 @@ get_or_create_type_def (uint64_t die_offset)
> return type_def_list;
> }
>
> +static void
> +display_enum (enum_constant *ec, int nb_tab)
> +{
> + if (ec != NULL)
> + {
> + display_enum (ec->next, nb_tab);
For large enumerations this can recurse pretty deeply; I'm not sure this
is a good idea. If this was last in the function, we could at least hope
for the compiler to carry this out as tail recursion, but this way the
compiler can't really do much, I fear. (Like other comments, this applies
elsewhere as well. I guess I won't repeat such a remark any further - you
want to apply respective changes throughout the series for any of the
comments.)
> @@ -2664,6 +2718,23 @@ get_or_create_enum_type (uint64_t die_offset)
> return enum_type_list;
> }
>
> +static void
> +display_subrange_type (subrange_type *st, int nb_tab)
> +{
> + if (st != NULL)
> + {
> + display_subrange_type (st->next, nb_tab);
> + if (st->usize != 0)
> + {
> + printf ("[%ld]", st->usize);
%lu? Yet then - isn't ->usize uint64_t? Then it would need to be PRIu64,
to also be correct for 32-bit builds. Obviously this may apply elsewhere
as well.
> + }
> + else if (st->ssize != 0)
> + {
> + printf ("[%ld]", st->ssize);
> + }
No need for the inner figure braces.
> @@ -2696,6 +2775,16 @@ get_or_create_tab (uint64_t die_offset)
> return tab_type_list;
> }
>
> +static void
> +display_member_type (member_type *mt, int nb_tab)
> +{
> + if (mt == NULL)
> + return;
I think this style of NULL check would be preferable elsewhere as well,
as it keeps down the amount of needed indentation for the remaining
body. (As alluded to elsewhere, doing things consistently at least
within a patch is also desirable.)
> @@ -2714,6 +2803,31 @@ get_or_create_member_type (member_type *head, uint64_t die_offset)
> return head;
> }
>
> +static void
> +display_member_parent (member_parent *mp, int is_struct)
> +{
> + if (!is_struct)
> + {
> + printf ("type: union %s, size: ", mp->name);
> + if (mp->ssize != 0)
> + printf ("0x%lx\n", mp->ssize);
> + else if (mp->usize != 0)
> + printf ("0x%lx\n", mp->usize);
> + }
> + else
> + {
> + printf ("type: struct %s, size: ", mp->name);
> + if (mp->ssize != 0)
> + printf ("0x%lx\n", mp->ssize);
> + else if (mp->usize != 0)
> + printf ("0x%lx\n", mp->usize);
> + else
> + printf ("Unknown\n");
Why is this different for struct (vs union)? I would have expected most
code here to be common for both.
> @@ -2750,6 +2876,130 @@ get_or_create_type_ref (type_ref **head, uint64_t die_offset)
> return *head;
> }
>
> +static void
> +display_variable_type (void *ptr_die, enum types type,
> + int nb_tab)
> +{
> + if (ptr_die == NULL)
> + return;
> +
> + switch (type)
> + {
> + case UNKNOWN_TYPE:
As before - case labels want to be indented the same as the opening brace.
Here it's the brace though that is misplaced.
> + fprintf (stderr,
> + _ ("display_variable_type: UNKNOWN_TYPE variable\n"));
> + free_mapping_info_struct ();
> + xexit (1);
> + case BASE_TYPE:
> + PRINT_SPACE (nb_tab);
> + display_base_type ((base_type *) ptr_die);
Why the cast?
> + break;
> + case TYPE_DEF:
> + PRINT_SPACE (nb_tab);
> + display_type_def ((type_def *) ptr_die);
> + PRINT_LBRACE (nb_tab);
> + display_variable_type (((type_def *) ptr_die)->ptr_type,
> + ((type_def *) ptr_die)->type,
Instead of repeating such casts, please make yourself local variables.
> @@ -13692,6 +13942,346 @@ struct dwarf_section_display debug_displays[] =
> /* A static assertion. */
> extern int debug_displays_assert[ARRAY_SIZE (debug_displays) == max ? 1 : -1];
>
> +static void
> +do_link_variable_information (void *ptr_die_type,
> + enum types die_type,
> + void *target_die_type,
> + enum types die_type_targeted)
> +{
> + switch (die_type)
> + {
> + case UNKNOWN_TYPE:
> + case BASE_TYPE:
> + case ENUM_TYPE:
> + case STRUCT_TYPE:
> + case UNION_TYPE:
> + break;
> + case MEMBER_TYPE:
> + ((member_type *) ptr_die_type)->type = die_type_targeted;
> + ((member_type *) ptr_die_type)->ptr_type = target_die_type;
This is awkward, it being just a single assignment (each) would be better.
And this remark goes along with my earlier one towards it not being possible
to identify the type from just the structure. You did drop the odd union (in
patch 3), but that wasn't really what I was after. Instead I had tried to
hint at the union wanting to gain a common, identifying field (along the
lines of what allows a union to become a transparent union, when making use
of that gcc extension).
> +static bool
> +link_variable_information (void *ptr_die_type,
> + enum types die_type,
> + uint64_t ptr_die_offset)
> +{
> + if (ptr_die_type == NULL)
> + return false;
> +
> + base_type *i_base_type = base_type_list;
> + type_def *i_type_def = type_def_list;
> + enum_type *i_enum_type = enum_type_list;
> + tab_type *i_tab_type = tab_type_list;
> + member_parent *i_struct_type = struct_type_list;
> + member_parent *i_union_type = union_type_list;
> + type_ref *i_ptr_type = ptr_type_list;
> + type_ref *i_const_type = const_type_list;
> + type_ref *i_volatile_type = volatile_type_list;
> + variable_type *i_variable_type = variable_type_list;
> +
> + if (is_nested_structure (ptr_die_type, die_type))
> + return true;
> +
> + while (true)
> + {
> + if (i_base_type != NULL)
> + {
> + if (i_base_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_base_type, BASE_TYPE);
> + break;
> + }
> + i_base_type = i_base_type->next;
> + }
> + if (i_type_def != NULL)
> + {
> + if (i_type_def->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_type_def, TYPE_DEF);
> + link_variable_information (i_type_def, TYPE_DEF,
> + i_type_def->ptr_die_offset);
> + break;
> + }
> + i_type_def = i_type_def->next;
> + }
> + if (i_enum_type != NULL)
> + {
> + if (i_enum_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_enum_type, ENUM_TYPE);
> + break;
> + }
> + i_enum_type = i_enum_type->next;
> + }
> + if (i_tab_type != NULL)
> + {
> + if (i_tab_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_tab_type, TAB_TYPE);
> + link_variable_information (i_tab_type, TAB_TYPE,
> + i_tab_type->ptr_die_offset);
> + break;
> + }
> + i_tab_type = i_tab_type->next;
> + }
> + if (i_struct_type != NULL)
> + {
> + if (i_struct_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_struct_type, STRUCT_TYPE);
> + member_type *mt = i_struct_type->members;
> + while (mt != NULL)
> + {
> + link_variable_information (mt,
> + MEMBER_TYPE,
> + mt->ptr_die_offset);
> + mt = mt->next;
> + }
> + break;
> + }
> + i_struct_type = i_struct_type->next;
> + }
> + if (i_union_type != NULL)
> + {
> + if (i_union_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_union_type, UNION_TYPE);
> + member_type *mt = i_union_type->members;
> + while (mt != NULL)
> + {
> + link_variable_information (mt,
> + MEMBER_TYPE,
> + mt->ptr_die_offset);
> + mt = mt->next;
> + }
> + break;
> + }
> + i_union_type = i_union_type->next;
> + }
> + if (i_ptr_type != NULL)
> + {
> + if (i_ptr_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_ptr_type, PTR_TYPE);
> + link_variable_information (i_ptr_type, PTR_TYPE,
> + i_ptr_type->ptr_die_offset);
> + break;
> + }
> + i_ptr_type = i_ptr_type->next;
> + }
> + if (i_const_type != NULL)
> + {
> + if (i_const_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_const_type, CONST_TYPE);
> + link_variable_information (i_const_type, CONST_TYPE,
> + i_const_type->ptr_die_offset);
> + break;
> + }
> + i_const_type = i_const_type->next;
> + }
> + if (i_volatile_type != NULL)
> + {
> + if (i_volatile_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_volatile_type, VOLATILE_TYPE);
> + link_variable_information (i_volatile_type, VOLATILE_TYPE,
> + i_volatile_type->ptr_die_offset);
> + break;
> + }
> + i_volatile_type = i_volatile_type->next;
> + }
> + if (i_variable_type != NULL)
> + {
> + if (i_variable_type->die_offset == ptr_die_offset)
> + {
> + do_link_variable_information (ptr_die_type, die_type,
> + i_variable_type, VARIABLE_TYPE);
> + link_variable_information (i_variable_type, VARIABLE_TYPE,
> + i_variable_type->ptr_die_offset);
> + break;
> + }
> + i_variable_type = i_variable_type->next;
> + }
> + if (i_base_type == NULL && i_type_def == NULL
> + && i_enum_type == NULL && i_tab_type == NULL
> + && i_struct_type == NULL && i_union_type == NULL
> + && i_ptr_type == NULL && i_ptr_type == NULL
> + && i_const_type == NULL && i_volatile_type == NULL
> + && i_variable_type == NULL)
> + return false;
Why is this one big loop instead of many, more efficient small ones?
> + }
> + return true;
> +}
> +
> +static uint64_t
> +compute_variable_ttsize (void *ptr_die, enum types type)
What is "tt"?
> +{
> + uint64_t size = 0;
> + if (ptr_die == NULL)
> + return 0;
> +
> + switch (type)
> + {
Brace placement again.
Jan
More information about the Binutils
mailing list