Re: [PATCH 1/6] gdb/testsuite/dwarf: use single abbrev table in .dwo files

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simon.marchi@efficios.com> writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> When I wrote test gdb.dwarf2/fission-with-type-unit.exp, I did not use
> build_executable_and_dwo_files, because it wouldn't work to have
> multiple units in the .dwo file, each referring to their own abbrev
> table using labels.  build_executable_and_dwo_files extracts the .dwo
> file content from the .o using objcopy (just like gcc does, I learned),
> meaning that the .dwo file never runs through a linker.  Anything
> needing relocation (like labels pointing to abbrev tables) doesn't work.
>
> I instead opted to use gdb_compile_shlib to build the .dwo file on its
> own, so that those labels would get resolved.  That causes problems now
> that I'm trying to write a test with multiple type units in a .dwo file,
> where each type unit should be in its own .debug_types section.  Running
> the .dwo file through the linker causes all the .debug_types section to
> be collapsed into one.  And generally, I think it was a bad idea to
> generate a .dwo file using the linker, since the idea behind .dwo files
> is that they do not need to be linked (therefore improving link
> times).  We want to produce files as close to what an actual compiler
> would produce.
>
> This patch fixes this by doing what compilers do in the same situation:
> use a single abbrev table shared by all units in the .dwo file.  This
> requires the following changes in lib/dwarf.exp:
>
>  - Declare a new variable _dwo_abbrev_num, which holds the next
>    abbrev number to use in the .dwo file's abbrev section
>    (.debug_abbrev.dwo).  Initialize this variable to 1.
>  - When producing a CU or TU in a .dwo file, use 0 as the abbrev table
>    offset.
>  - When generating a DIE, return $_dwo_abbrev_num or $_abbrev_num,
>    depending on whether the current CU is in a .dwo file.
>  - After producing a CU or TU in a .dwo file, don't append the
>    terminator byte.
>  - After finishing producing the CUs and TUs, append the terminator byte
>    in .debug_abbrev.dwo if we did output anything there.
>
> Update gdb.dwarf2/fission-with-type-units.exp to use
> build_executable_and_dwo_files, as it should.
>
> Change-Id: Iabbcf00db97faf2a4fa5fc71652ad273081189f9
> ---
>  .../gdb.dwarf2/fission-with-type-unit.exp     | 29 +++-----
>  gdb/testsuite/lib/dwarf.exp                   | 69 +++++++++++++++----
>  2 files changed, 67 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp b/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
> index 58cda296c67b..5c73001ec66b 100644
> --- a/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
> +++ b/gdb/testsuite/gdb.dwarf2/fission-with-type-unit.exp
> @@ -21,25 +21,22 @@ load_lib dwarf.exp
>  # This test can only be run on targets which support DWARF-2 and use gas.
>  require dwarf2_support
>  
> -standard_testfile .c -dw.S -dwo.S
> +standard_testfile .c -dw.S
>  
> -set main_asm_file [standard_output_file $srcfile2]
> -set dwo_asm_file [standard_output_file $srcfile3]
> +set asm_file [standard_output_file $srcfile2]
>  
> -# Debug info in the main file.
> -Dwarf::assemble $main_asm_file {
> +Dwarf::assemble $asm_file {
> +    # In the main file.
>      cu {
>  	version 5
>  	dwo_id 0xF00D
>      } {
>  	compile_unit {
> -	    DW_AT_dwo_name ${::gdb_test_file_name}.dwo DW_FORM_strp
> +	    DW_AT_dwo_name ${::gdb_test_file_name}-dw.dwo DW_FORM_strp
>  	} {}
>      }
> -}
>  
> -# Debug info in the DWO file.
> -Dwarf::assemble $dwo_asm_file {
> +    # In the .dwo file.
>      tu {
>  	fission 1
>  	version 5
> @@ -79,15 +76,11 @@ Dwarf::assemble $dwo_asm_file {
>      }
>  }
>  
> -# Build main file.
> -if { [build_executable "${testfile}.exp" $binfile \
> -	[list ${srcfile} ${main_asm_file}] {nodebug}] } {
> -    return
> -}
> -
> -# Build DWO file.
> -set dwo_file [standard_output_file ${testfile}.dwo]
> -if { [gdb_compile_shlib $dwo_asm_file $dwo_file nodebug] != "" } {
> +set obj [standard_output_file "${testfile}-dw.o"]
> +set dwo_file [standard_output_file "${testfile}.dwo"]

The name of the dwo file here is now wrong.  The auto generated name
will be '${testfile}-dw.dwo', which is what you embed in the DWARF
above.

I was wondering why this wasn't causing problems for the later block in
this file (not touched in this patch):

  if { [is_remote host] } {
      gdb_remote_download host $dwo_file
  }

But it turns out that ...

> +if {[build_executable_and_dwo_files "$testfile.exp" "${binfile}" {} \

... build_executable_and_dwo_files includes this check:

    # Must be run on local host due to use of objcopy.
    if {[is_remote host]} {
	return -1
    }

So this test never gets past this point for remote host boards.

I think you should:

  1. Remove the 'set dwo_file ...' line.

  2. Remove the 'gdb_remote_download host $dwo_file' line, and its
     containing 'if' block.

  3. Add a 'require {!is_remote host}' line at the start of the file.

Otherwise, this change looks good to me (along with the additional text
you posted in the follow up, which I also replied to).

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


> +	 [list $asm_file {nodebug split-dwo} $obj] \
> +	 [list $srcfile  {nodebug}]]} {
>      return
>  }
>  
> diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> index ba8197d0bb8e..919e2e05687f 100644
> --- a/gdb/testsuite/lib/dwarf.exp
> +++ b/gdb/testsuite/lib/dwarf.exp
> @@ -563,6 +563,9 @@ namespace eval Dwarf {
>      # table.
>      variable _abbrev_num
>  
> +    # The next available abbrev number in the (single) DWO abbrev table.
> +    variable _dwo_abbrev_num
> +
>      # The string table for this assembly.  The key is the string; the
>      # value is the label for that string.
>      variable _strings
> @@ -1008,8 +1011,17 @@ namespace eval Dwarf {
>      # table.
>      proc _get_abbrev_num {} {
>  	variable _abbrev_num
> -	set res $_abbrev_num
> -	incr _abbrev_num
> +	variable _dwo_abbrev_num
> +	variable _cu_is_fission
> +
> +	if { $_cu_is_fission } {
> +	    set res $_dwo_abbrev_num
> +	    incr _dwo_abbrev_num
> +	} else {
> +	    set res $_abbrev_num
> +	    incr _abbrev_num
> +	}
> +
>  	return $res
>      }
>  
> @@ -1538,8 +1550,17 @@ namespace eval Dwarf {
>  	_section $section
>  
>  	set cu_num [incr _cu_count]
> -	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> -	set _abbrev_num 1
> +
> +	# Since .dwo files are not linked, we can't use a label to point to a
> +	# specific place in the .debug_abbrev section.  For .dwo files, we
> +	# therefore use a single abbrev table (at offset 0) shared by all units
> +	# in that file.
> +	if { $_cu_is_fission } {
> +	    set my_abbrevs 0
> +	} else {
> +	    set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> +	    set _abbrev_num 1
> +	}
>  
>  	set _cu_label [_compute_label "cu${cu_num}_begin"]
>  	set start_label [_compute_label "cu${cu_num}_start"]
> @@ -1606,9 +1627,11 @@ namespace eval Dwarf {
>  
>  	uplevel $_level $body
>  
> -	_defer_output $_abbrev_section {
> -	    # Emit the terminator.
> -	    _op .byte 0x0 "Abbrev end - Terminator"
> +	if { !$_cu_is_fission } {
> +	    _defer_output $_abbrev_section {
> +		# Emit the terminator.
> +		_op .byte 0x0 "Abbrev end - Terminator"
> +	    }
>  	}
>  
>  	define_label $end_label
> @@ -1681,8 +1704,17 @@ namespace eval Dwarf {
>  	_section $section
>  
>  	set cu_num [incr _cu_count]
> -	set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> -	set _abbrev_num 1
> +
> +	# Since .dwo files are not linked, we can't use a label to point to a
> +	# specific place in the .debug_abbrev section.  For .dwo files, we
> +	# therefore use a single abbrev table (at offset 0) shared by all units
> +	# in that file.
> +	if { $_cu_is_fission } {
> +		set my_abbrevs 0
> +	} else {
> +		set my_abbrevs [_compute_label "abbrev${cu_num}_begin"]
> +		set _abbrev_num 1
> +	}
>  
>  	set _cu_label [_compute_label "cu${cu_num}_begin"]
>  	set start_label [_compute_label "cu${cu_num}_start"]
> @@ -1736,9 +1768,11 @@ namespace eval Dwarf {
>  
>  	uplevel $_level $body
>  
> -	_defer_output $_abbrev_section {
> -	    # Emit the terminator.
> -	    _op .byte 0x0 "Abbrev end - Terminator"
> +	if { !$_cu_is_fission } {
> +	    _defer_output $_abbrev_section {
> +		# Emit the terminator.
> +		_op .byte 0x0 "Abbrev end - Terminator"
> +	    }
>  	}
>  
>  	define_label $end_label
> @@ -3507,6 +3541,7 @@ namespace eval Dwarf {
>  	variable _debug_ranges_64_bit
>  	variable _debug_addr_index
>  	variable _level
> +	variable _dwo_abbrev_num
>  
>  	if { [llength $options] == 1 } {
>  	    set options [list filename [lindex $options 0]]
> @@ -3539,8 +3574,8 @@ namespace eval Dwarf {
>  
>  	set _line_count 0
>  	set _debug_ranges_64_bit [is_64_target]
> -
>  	set _debug_addr_index 0
> +	set _dwo_abbrev_num 1
>  
>  	# Dummy CU at the start to ensure that the first CU in $body is not
>  	# the first in .debug_info.
> @@ -3562,6 +3597,14 @@ namespace eval Dwarf {
>  	    dummy_cu
>  	}
>  
> +	# If we wrote any abbrev in .debug_abbrev.dwo, write the terminator.
> +	if { $_dwo_abbrev_num > 1 } {
> +	    _defer_output .debug_abbrev.dwo {
> +		# Emit the terminator.
> +		_op .byte 0x0 "Abbrev end - Terminator"
> +	    }
> +	}
> +
>  	_write_deferred_output
>  
>  	_section .note.GNU-stack "" progbits
> -- 
> 2.51.2