[PATCH v3 2/5] objdump: New feature display of global variable information based on DWARF info section

[Guillaume VACHERIAS]

On 01.09.2025 19:50, Guillaume VACHERIAS wrote:
> Add in objdump option --map-global-vars to display global variable information

As for patch 1, some words on what good this does would be nice.

Ok ! Will do in v4

> binutils/
> 
> 	* objdump.c (dump_global_vars_info): New function. Entry point of
> 	display of variable information option.
> 	(dump_global_variable_info): New function. Load and go through dwarf
> 	info section to process information.
> 	* dwarf.c (insert_element_in_list): New function. Retrieve valuable
> 	information from dwarf attributes of all DWARF debug information
> 	entries.
> 	(get_location_expression): New function. Useful to retrieve
> 	DW_AT_location value which gives the data object location.
> 	(read_and_display_attr_value): Add arguments useful to identify DWARF
> 	information entry, and instrument with function
> 	insert_element_in_list.
> 	(read_and_display_attr): Add arguments useful to identify DWARF
> 	information entry.
> 	(process_debug_info): Add argument to process DWARF information entry
> 	for the display of global variable information.
> 	(display_variable_mapping_info): New function.
> 
> binutils/doc/
> 
> 	* binutils.texi: Add documentation for new option --map-global-vars.
> 
> Signed-off-by: Guillaume VACHERIAS <guillaume.vacherias@foss.st.com>
> ---
>  binutils/ChangeLog         |  22 ++-
>  binutils/doc/binutils.texi |   7 +
>  binutils/dwarf.c           | 382 +++++++++++++++++++++++++++++++++++--
>  binutils/dwarf.h           |   1 +
>  binutils/objdump.c         |  86 ++++++++-
>  5 files changed, 478 insertions(+), 20 deletions(-)
> 
> diff --git a/binutils/ChangeLog b/binutils/ChangeLog
> index 0c061efcb17..72f0ed9eb0a 100644
> --- a/binutils/ChangeLog
> +++ b/binutils/ChangeLog
> @@ -1,8 +1,28 @@
>  2025-09-01  Guillaume Vacherias <guillaume.vacherias@foss.st.com>
> -	* dwarf.c (DO_LOC, DO_TYPES): Define.
> +	* dwarf.c (DO_LOC, DO_TYPES, DO_GLOBAL_VARS): Define.
>  	(process_debug_info): Change arguments do_loc and do_types
>  	to a single unsigned int do_flags.
> +	(insert_element_in_list): New function.  Retrieve valuable
> +	information from DWARF attributes of all DWARF debug information
> +	entries.
> +	(get_location_expression): New function.  Useful to retrieve
> +	DW_AT_location value which gives the data object location.
> +	(read_and_display_attr_value): Add arguments useful to identify DWARF
> +	information entry, and instrument with function
> +	insert_element_in_list.
>  	(find_cu_tu_set_v2): Change parameter do_types from int to bool.
> +	(read_and_display_attr): Add arguments useful to identify DWARF
> +	information entry.
> +	(process_debug_info): Add argument to process DWARF information entry
> +	for the display of global variable information.
> +	(display_variable_mapping_info): New function.
> +
> +	* objdump.c (dump_global_vars_info): New function.  Entry point of
> +	display of variable information option.
> +	(dump_global_variable_info): New function.  Load and go through DWARF
> +	info section to process information.
> +
> +	* doc/binutils.texi: Add documentation for new option --map-global-vars.

First, see my respective comment on patch 1. Then, even if you were to modify
the file directly, you definitely don't want to alter an existing entry.

Yes, I'll undo the modifications on ChangeLog file.

> @@ -2435,6 +2571,53 @@ display_lang (uint64_t uvalue)
>      }
>  }
>  
> +static void
> +insert_element_in_list (enum dwarf_tag dw_tag,
> +			enum dwarf_attribute dw_attr ATTRIBUTE_UNUSED,
> +			uint64_t die_offset ATTRIBUTE_UNUSED,
> +			uint64_t *uvalue ATTRIBUTE_UNUSED,
> +			int64_t *svalue ATTRIBUTE_UNUSED,
> +			const unsigned char *data ATTRIBUTE_UNUSED)
> +{
> +  /* TODO: Retrieve attributes information from
> +     below dwarf entries tag.  */
> +  switch (dw_tag)
> +    {
> +    /* Base type.  */
> +    case DW_TAG_base_type:
> +      break;
> +    /* typedef type.  */

With "typedef" being a keyword in C, having it all lower-case may be okay
here, but ...

> +    case DW_TAG_typedef:
> +      break;
> +    /* enumeration type.  */

"enumeration" clearly isn't a keyword, and hence wants starting with a capital
letter, just like the other comments here have it. Also, isn't this more the
comment for ...

> +    case DW_TAG_enumerator:
> +      break;
> +    case DW_TAG_enumeration_type:
> +      break;

... this case?

> +    /* Array type.  */
> +    case DW_TAG_subrange_type:
> +      break;
> +    case DW_TAG_array_type:
> +      break;

Same here - misplaced (and missing) comment?

> +    /* Member type.  */
> +    case DW_TAG_member:
> +      break;
> +    /* Structure type.  */
> +    case DW_TAG_structure_type:
> +    /* Union type.  */
> +    case DW_TAG_union_type:
> +      break;
> +    case DW_TAG_const_type:
> +    case DW_TAG_volatile_type:
> +    case DW_TAG_pointer_type:
> +      break;
> +    case DW_TAG_variable:
> +      break;

No comments for these last four? I'm not convinced of the need for these
comments, but imo if you add them, then please do so consistently.

Yes, thank you for pointing this out.

> @@ -2451,7 +2634,10 @@ read_and_display_attr_value (unsigned long attribute,
>  			     struct dwarf_section *section,
>  			     struct cu_tu_set *this_set,
>  			     char delimiter,
> -			     int level)
> +			     int level,
> +			     int do_var_map,

bool? (applicable elsewhere as well)

> @@ -2855,14 +3098,25 @@ read_and_display_attr_value (unsigned long attribute,
>  
>  	  /* We have already displayed the form name.  */
>  	  if (idx != (uint64_t) -1)
> -	    printf (_("%c(index: %#" PRIx64 "): %#" PRIx64),
> +	    {
> +	      printf (_("%c(index: %#" PRIx64 "): %#" PRIx64),

You shouldn't change this line's indentation without ...

>  		    delimiter, uvalue, idx);

... this one's.

> @@ -3143,6 +3397,76 @@ read_and_display_attr_value (unsigned long attribute,
>  	}
>      }
>  
> +  if (do_var_map && attribute != 0)

Why the check of attribute, when the default case of the switch (doing nothing)
would handle it fine?

You're right the check on attribute is useless here.

> +    {
> +      switch (attribute)
> +	{
> +	case DW_AT_frame_base:
> +	case DW_AT_location:
> +	case DW_AT_loclists_base:
> +	case DW_AT_rnglists_base:
> +	case DW_AT_str_offsets_base:
> +	case DW_AT_string_length:
> +	case DW_AT_return_addr:
> +	case DW_AT_data_member_location:
> +	case DW_AT_vtable_elem_location:
> +	case DW_AT_segment:
> +	case DW_AT_static_link:
> +	case DW_AT_use_location:
> +	case DW_AT_call_value:
> +	case DW_AT_GNU_call_site_value:
> +	case DW_AT_call_data_value:
> +	case DW_AT_GNU_call_site_data_value:
> +	case DW_AT_call_target:
> +	case DW_AT_GNU_call_site_target:
> +	case DW_AT_call_target_clobbered:
> +	case DW_AT_GNU_call_site_target_clobbered:
> +	case DW_AT_allocated:
> +	case DW_AT_associated:
> +	case DW_AT_data_location:
> +	case DW_AT_stride:
> +	case DW_AT_upper_bound:
> +	case DW_AT_lower_bound:
> +	case DW_AT_rank:
> +	  if (block_start)
> +	    {
> +	      get_location_expression (die_tag,
> +				       attribute,
> +				       block_start,
> +				       pointer_size,
> +				       offset_size,
> +				       dwarf_version,
> +				       die_offset,
> +				       uvalue,
> +				       cu_offset,
> +				       section);

You may want to make better use of line length here.

> +	    }

Omitting the figure braces also would reduce indentation a tiny bit.

(Both applicable again below.)

> @@ -3834,7 +4164,7 @@ process_debug_info (struct dwarf_section * section,
>        alloc_num_debug_info_entries = num_units;
>      }
>  
> -  if (!(do_flags & DO_LOC))
> +  if (!(do_flags & DO_LOC) || do_flags & DO_GLOBAL_VARS)

Prentheses please to visually separate the & expression from the || one.

> @@ -7683,6 +8020,13 @@ display_debug_str (struct dwarf_section *section,
>    return 1;
>  }
>  
> +static int
> +display_variable_mapping_info (struct dwarf_section *section, void *file)
> +{
> +  return process_debug_info (section, file, section->abbrev_sec,
> +			     DO_LOC | DO_GLOBAL_VARS);

Why also DO_LOC?

When DO_LOC is set, process_debug_info() scans for location lists and dwo tags.
Futhermore it does not display anything to the user. DO_GLOBAL_VARS takes
advantage of DO_LOC scanning function to retrieve dwo tag and it does not
want to display any information yet.

> @@ -4528,6 +4533,42 @@ dump_dwarf_section (bfd *abfd, asection *section,
>        }
>  }
>  
> +static void
> +dump_global_variable_info (bfd *abfd, asection *section,
> +		    void *arg)
> +{
> +  const char *name = bfd_section_name (section);
> +  const char *match;
> +  bool is_mainfile = *(bool *) arg;
> +
> +  if (*name == 0)
> +    return;
> +
> +  if (!is_mainfile && !process_links
> +      && (section->flags & SEC_DEBUGGING) == 0)
> +    return;
> +
> +  if (startswith (name, ".gnu.linkonce.wi."))
> +    match = ".debug_info";
> +  else
> +    match = name;
> +
> +  if (((strcmp (debug_displays[info].section.uncompressed_name,match) == 0
> +  || strcmp (debug_displays[info].section.compressed_name, match) == 0
> +  || strcmp (debug_displays[info].section.xcoff_name, match) == 0))
> +  && debug_displays[info].enabled != NULL
> +  && *debug_displays[info].enabled)

I'm pretty sure I did already comment on this or a similar indentation issue?

Yes, I've missed this...

> @@ -4560,6 +4601,41 @@ dump_dwarf (bfd *abfd, bool is_mainfile)
>    bfd_map_over_sections (abfd, dump_dwarf_section, (void *) &is_mainfile);
>  }
>  

> +

Please don't introduce double blank lines, as they can make patch context
less meaningful.

> +/* Dump all global variable information in memory.  */
> +
> +static void
> +dump_global_vars_info (bfd *abfd, bool is_mainfile)
> +{
> +  /* The byte_get pointer should have been set at the start of dump_bfd ().  */
> +  if (byte_get == NULL)
> +    {
> +      warn (_ ("File %s does not contain any dwarf debug information\n"),
> +	    bfd_get_filename (abfd));
> +      return;
> +    }
> +
> +  switch (bfd_get_arch (abfd))
> +    {
> +    case bfd_arch_s12z:
> +      /* S12Z has a 24 bit address space.  But the only known
> +	 producer of dwarf_info encodes addresses into 32 bits.  */
> +      eh_addr_size = 4;
> +      break;
> +
> +    default:
> +      eh_addr_size = bfd_arch_bits_per_address (abfd) / 8;
> +      break;
> +    }

I pretty strongly think such logic wants isolating into a helper rather
than having in two places, where thngs can easily go out of sync.

Ok I'll place this in a helper function in v4 !

Jan

Thank you !

Guillaume.