Re: [PATCH 13/14] libdw: Apply DWARF package file section offsets where appropriate

[Mark Wielaard <mark@klomp.org>]

Hi Omar,

On Wed, Sep 27, 2023 at 11:21:02AM -0700, Omar Sandoval wrote:
> The final piece of DWARF package file support is that offsets have to be
> interpreted relative to the section offset from the package index.
> .debug_abbrev.dwo is already covered, so sprinkle around calls to
> dwarf_cu_dwp_section_info for the remaining sections: .debug_line.dwo,
> .debug_loclists.dwo/.debug_loc.dwo, .debug_str_offsets.dwo,
> .debug_macro.dwo/.debug_macinfo.dwo, and .debug_rnglists.dwo.  With all
> of that in place, we can finally test various libdw functions on dwp
> files.

This looks good, but obviously needs some earlier patches.

> diff --git a/libdw/ChangeLog b/libdw/ChangeLog
> index f37d9411..08b2c3e6 100644
> --- a/libdw/ChangeLog
> +++ b/libdw/ChangeLog
> @@ -12,10 +12,13 @@
>  	* dwarf_getmacros.c (get_macinfo_table): Replace assignment of
>  	table->is_64bit with assignments of table->address_size and
>  	table->offset_size.  Assume default DW_AT_stmt_list of 0 for split
> -	DWARF.  Set table->dbg.
> +	DWARF.  Set table->dbg.  Call dwarf_cu_dwp_section_info and add offset
> +	to line_offset.
>  	(get_table_for_offset): Ditto.
>  	(read_macros): Get fake CU offset_size from table->offset_size instead
>  	of table->is_64bit.
> +	(get_offset_from): Call dwarf_cu_dwp_section_info and add offset to
> +	*retp.
>  	* dwarf_macro_getsrcfiles.c (dwarf_macro_getsrcfiles): Get
>  	address_size for __libdw_getsrclines from table->address_size instead
>  	of table->is_64bit.  Get dbg for __libdw_getsrclines from table->dbg
> @@ -36,6 +39,9 @@
>  	(__libdw_set_debugdir): New declaration.
>  	(__libdw_dwp_findcu_id): New declaration.
>  	(__libdw_link_skel_split): Handle .debug_addr for dwp.
> +	(str_offsets_base_off): Call dwarf_cu_dwp_section_info and add offset.
> +	(__libdw_cu_ranges_base): Ditto.
> +	(__libdw_cu_locs_base): Ditto.
>  	* dwarf_begin_elf.c (dwarf_scnnames): Add IDX_debug_cu_index and
>  	IDX_debug_tu_index.
>  	(scn_to_string_section_idx): Ditto.
> @@ -60,6 +66,8 @@
>  	__libdw_dwp_find_unit, and use it to adjust abbrev_offset and assign
>  	newp->dwp_row.
>  	* dwarf_cu_dwp_section_info.c: New file.
> +	* dwarf_getlocation.c (initial_offset): Call dwarf_cu_dwp_section_info
> +	and add offset to start_offset.
>  
>  2023-02-22  Mark Wielaard  <mark@klomp.org>
>  
> diff --git a/libdw/dwarf_getlocation.c b/libdw/dwarf_getlocation.c
> index 553fdc98..37b32fc1 100644
> --- a/libdw/dwarf_getlocation.c
> +++ b/libdw/dwarf_getlocation.c
> @@ -812,6 +812,12 @@ initial_offset (Dwarf_Attribute *attr, ptrdiff_t *offset)
>  			    : DWARF_E_NO_DEBUG_LOCLISTS),
>  			    NULL, &start_offset) == NULL)
>  	return -1;
> +
> +      Dwarf_Off loc_off;
> +      if (INTUSE(dwarf_cu_dwp_section_info) (attr->cu, DW_SECT_LOCLISTS,
> +					     &loc_off, NULL) != 0)
> +	return -1;
> +      start_offset += loc_off;
>      }
>  
>    *offset = start_offset;

OK. This already handled DW_FORM_loclistx, now also other loclist
refs.

> diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
> index a3a78884..2667eb45 100644
> --- a/libdw/dwarf_getmacros.c
> +++ b/libdw/dwarf_getmacros.c
> @@ -47,7 +47,15 @@ get_offset_from (Dwarf_Die *die, int name, Dwarf_Word *retp)
>      return -1;
>  
>    /* Offset into the corresponding section.  */
> -  return INTUSE(dwarf_formudata) (&attr, retp);
> +  if (INTUSE(dwarf_formudata) (&attr, retp) != 0)
> +    return -1;
> +
> +  Dwarf_Off offset;
> +  if (INTUSE(dwarf_cu_dwp_section_info) (die->cu, DW_SECT_MACRO, &offset, NULL)
> +      != 0)
> +    return -1;
> +  *retp += offset;
> +  return 0;
>  }

OK.

>  static int
> @@ -131,6 +139,14 @@ get_macinfo_table (Dwarf *dbg, Dwarf_Word macoff, Dwarf_Die *cudie)
>    else if (cudie->cu->unit_type == DW_UT_split_compile
>  	   && dbg->sectiondata[IDX_debug_line] != NULL)
>      line_offset = 0;
> +  if (line_offset != (Dwarf_Off) -1)
> +    {
> +      Dwarf_Off dwp_offset;
> +      if (INTUSE(dwarf_cu_dwp_section_info) (cudie->cu, DW_SECT_LINE,
> +					     &dwp_offset, NULL) != 0)
> +	return NULL;
> +      line_offset += dwp_offset;
> +    }

OK.
 
>    Dwarf_Macro_Op_Table *table = libdw_alloc (dbg, Dwarf_Macro_Op_Table,
>  					     macinfo_data_size, 1);
> @@ -188,6 +204,14 @@ get_table_for_offset (Dwarf *dbg, Dwarf_Word macoff,
>  	if (unlikely (INTUSE(dwarf_formudata) (attr, &line_offset) != 0))
>  	  return NULL;
>      }
> +  if (line_offset != (Dwarf_Off) -1 && cudie != NULL)
> +    {
> +      Dwarf_Off dwp_offset;
> +      if (INTUSE(dwarf_cu_dwp_section_info) (cudie->cu, DW_SECT_LINE,
> +					     &dwp_offset, NULL) != 0)
> +	return NULL;
> +      line_offset += dwp_offset;
> +    }
>  
>    uint8_t address_size;
>    if (cudie != NULL)

OK.

> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index 16355274..bdbf5e5a 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -1111,25 +1111,27 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
>  	cu = first_cu;
>      }
>  
> +  Dwarf_Off off = 0;
>    if (cu != NULL)
>      {
>        if (cu->str_off_base == (Dwarf_Off) -1)
>  	{
> +	  dwarf_cu_dwp_section_info (cu, DW_SECT_STR_OFFSETS, &off, NULL);
>  	  Dwarf_Die cu_die = CUDIE(cu);
>  	  Dwarf_Attribute attr;
>  	  if (dwarf_attr (&cu_die, DW_AT_str_offsets_base, &attr) != NULL)
>  	    {
> -	      Dwarf_Word off;
> -	      if (dwarf_formudata (&attr, &off) == 0)
> +	      Dwarf_Word base;
> +	      if (dwarf_formudata (&attr, &base) == 0)
>  		{
> -		  cu->str_off_base = off;
> +		  cu->str_off_base = off + base;
>  		  return cu->str_off_base;
>  		}
>  	    }
>  	  /* For older DWARF simply assume zero (no header).  */
>  	  if (cu->version < 5)
>  	    {
> -	      cu->str_off_base = 0;
> +	      cu->str_off_base = off;
>  	      return cu->str_off_base;
>  	    }
>  
> @@ -1142,7 +1144,6 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
>  
>    /* No str_offsets_base attribute, we have to assume "zero".
>       But there could be a header first.  */
> -  Dwarf_Off off = 0;
>    if (dbg == NULL)
>      goto no_header;
>  
> @@ -1184,7 +1185,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
>    /* padding */
>    read_2ubyte_unaligned_inc (dbg, readp);
>  
> -  off = (Dwarf_Off) (readp - start);
> +  off += (Dwarf_Off) (readp - start);
>  
>   no_header:
>    if (cu != NULL)


OK, so the dwp section offset gets added to the base attribute offset.

> @@ -1222,18 +1223,22 @@ __libdw_cu_ranges_base (Dwarf_CU *cu)
>  	}
>        else
>  	{
> +	  Dwarf_Off dwp_offset = 0;
> +	  dwarf_cu_dwp_section_info (cu, DW_SECT_RNGLISTS, &dwp_offset, NULL);

Shouldn't we check there wasn't an error?

> +	  offset = dwp_offset;
> +
>  	  if (dwarf_attr (&cu_die, DW_AT_rnglists_base, &attr) != NULL)
>  	    {
>  	      Dwarf_Word off;
>  	      if (dwarf_formudata (&attr, &off) == 0)
> -		offset = off;
> +		offset += off;
>  	    }
>  
>  	  /* There wasn't an rnglists_base, if the Dwarf does have a
>  	     .debug_rnglists section, then it might be we need the
>  	     base after the first header. */
>  	  Elf_Data *data = cu->dbg->sectiondata[IDX_debug_rnglists];
> -	  if (offset == 0 && data != NULL)
> +	  if (offset == dwp_offset && data != NULL)
>  	    {
>  	      Dwarf *dbg = cu->dbg;
>  	      const unsigned char *readp = data->d_buf;
> @@ -1279,8 +1284,8 @@ __libdw_cu_ranges_base (Dwarf_CU *cu)
>  	      if (unit_length - 8 < needed)
>  		goto no_header;
>  
> -	      offset = (Dwarf_Off) (offset_array_start
> -				    - (unsigned char *) data->d_buf);
> +	      offset += (Dwarf_Off) (offset_array_start
> +				     - (unsigned char *) data->d_buf);
>  	    }
>  	}
>      no_header:

OK, same for ranges.

> @@ -1297,21 +1302,24 @@ __libdw_cu_locs_base (Dwarf_CU *cu)
>  {
>    if (cu->locs_base == (Dwarf_Off) -1)
>      {
> -      Dwarf_Off offset = 0;
> +      Dwarf_Off dwp_offset = 0;
> +      dwarf_cu_dwp_section_info (cu, DW_SECT_LOCLISTS, &dwp_offset, NULL);

No error check?

> +      Dwarf_Off offset = dwp_offset;
> +
>        Dwarf_Die cu_die = CUDIE(cu);
>        Dwarf_Attribute attr;
>        if (dwarf_attr (&cu_die, DW_AT_loclists_base, &attr) != NULL)
>  	{
>  	  Dwarf_Word off;
>  	  if (dwarf_formudata (&attr, &off) == 0)
> -	    offset = off;
> +	    offset += off;
>  	}
>  
>        /* There wasn't an loclists_base, if the Dwarf does have a
>  	 .debug_loclists section, then it might be we need the
>  	 base after the first header. */
>        Elf_Data *data = cu->dbg->sectiondata[IDX_debug_loclists];
> -      if (offset == 0 && data != NULL)
> +      if (offset == dwp_offset && data != NULL)
>  	{
>  	  Dwarf *dbg = cu->dbg;
>  	  const unsigned char *readp = data->d_buf;
> @@ -1357,8 +1365,8 @@ __libdw_cu_locs_base (Dwarf_CU *cu)
>  	  if (unit_length - 8 < needed)
>  	    goto no_header;
>  
> -	  offset = (Dwarf_Off) (offset_array_start
> -				- (unsigned char *) data->d_buf);
> +	  offset += (Dwarf_Off) (offset_array_start
> +				 - (unsigned char *) data->d_buf);
>  	}
>  
>      no_header:

OK. Same for loclists.

> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 97261444..93503f57 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,6 +1,7 @@
>  2023-09-27  Omar Sandoval  <osandov@fb.com>
>  
> -	* run-varlocs.sh: Add entry PC to split units.
> +	* run-varlocs.sh: Add entry PC to split units.  Check testfile-dwp-5
> +	and testfile-dwp-4.
>  	* Makefile.am (check_PROGRAMS): Add cu-dwp-section-info.
>  	(TESTS): Add run-cu-dwp-section-info.sh
>  	(EXTRA_DIST): Add run-cu-dwp-section-info.sh and new test files.
> @@ -20,6 +21,11 @@
>  	(getmacros): New function.
>  	(main): If second argument is empty, call getmacros on every unit.
>  	Otherwise, call getmacros on given unit.
> +	* run-all-dwarf-ranges.sh: Check testfile-dwp-5 and testfile-dwp-4.
> +	* run-dwarf-getmacros.sh: Check testfile-dwp-5 and
> +	testfile-dwp-4-strict.
> +	* run-get-units-split.sh: Check testfile-dwp-5, testfile-dwp-4, and
> +	testfile-dwp-4-strict.

Nice collection of tests.

Thanks,

Mark