Re: [PATCH, v2] Add directory containing executable to relative search paths for .dwo files.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Caroline Tice <cmtice@google.com> [2021-03-25 08:09:21 -0700]:

> Hi Andrew,
> 
> As I said previously, your suggested patch works well and fixes the
> problem.  I thought you were going to submit it, but I have not seen it  go
> out for review or be committed.  Is there something else you need me to
> do?  I would be happy to send your version for review if you would
> like?

I'll take a look at this tomorrow.

Thanks,
Andrew


> 
> -- Caroline
> cmtice@google.com
> 
> 
> On Wed, Mar 17, 2021 at 5:26 PM Andrew Burgess <andrew.burgess@embecosm.com>
> wrote:
> 
> > * Caroline Tice <cmtice@google.com> [2021-03-04 07:48:37 -0800]:
> >
> > > Hi Andrew,
> > >
> > > I updated my change to gdb/dwarf2/read.c as you suggested (spaces
> > > before '(' and use nullptr).
> > >
> > > I spent a long time looking into using Dwarf::assemble in my testcase,
> > > to make it hardware agnostic (although I'd like to point out
> > > that most of the rest of the fission tests specifically only work for
> > > x86_64),
> >
> > Most of these were written before the Dwarf assembler was available.
> > If they were being written today then we'd be using the Dwarf
> > assembler.
> >
> > >           but in the end found that it would not work for me.   The
> > > problems that I was unable to make this work for was that I needed to
> > > generate not only the binary but the .dwo file; I needed reasonably
> > > valid DWARF in both of them; and the path name for the .dwo file had
> > > to be relative not absolute -- the way gdb compiles the test suites it
> > > always uses complete absolute paths for all of its files.
> >
> > I don't understand what problem you ran into here.  The path to the
> > DWO file is encoded into the DWARF of the executable in a
> > DW_AT_GNU_dwo_name attribute, which would be entirely generated within
> > the .exp file, and so could contain whatever you like.  Likewise for
> > the DW_AT_comp_dir.
> >
> > I made an attempt at writing a test using the dwarf assembler.  You
> > will need to start by applying this patch to a clean GDB tree:
> >
> >    https://sourceware.org/pipermail/gdb-patches/2021-March/177049.html
> >
> > Then grab the patch below and give it a try.  It includes your fix,
> > and a brand new test case.  Let me know if you don't think this is
> > covering your change correctly, I'm sure we can get it working as
> > needed.
> >
> > Thanks,
> > Andrew
> >
> > ---
> >
> > commit 47bf19a7ed4c96a86d67329efb7e5ce983debddd
> > Author: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date:   Wed Mar 17 22:54:22 2021 +0000
> >
> >     gdb: handle relative paths to DWO files
> >
> >     **** NOTE ****
> >
> >     The fix in dwarf2/read.c includes a hack to allow the fix to be
> >     toggled on/off using environment variable GDB_NO_FIX.  This must be
> >     removed before pushing upstream.
> >
> >     **** END NOTE ****
> >
> >     DWARF allows .dwo file paths to be relative rather than absolute.
> >
> >     When they are relative, DWARF uses DW_AT_comp_dir to find the .dwo
> >     file.  DW_AT_comp_dir can also be relative, making the entire search
> >     patch for the .dwo file relative.
> >
> >     In this case, GDB currently searches relative to its current working
> >     directory, i.e. the directory from which the debugger was launched,
> >     but not relative to the directory containing the built binary.  This
> >     cannot be right, as the compiler, when generating the relative paths,
> >     knows where it's building the binary but can have no idea where the
> >     debugger will be launched.
> >
> >     The correct thing is to add the directory containing the binary to the
> >     search paths used for resolving relative locations of dwo files. That
> >     is what this patch does.
> >
> >     gdb/ChangeLog:
> >
> >             * dwarf2/read.c (try_open_dwop_file): Add path for the binary
> > to
> >             the search paths used resolve relative location of .dwo file.
> >
> >     gdb/testsuite/ChangeLog:
> >
> >             * gdb.dwarf2/fission-relative-dwo.c: New file.
> >             * gdb.dwarf2/fission-relative-dwo.exp: New file.
> >             * lib/dwarf.exp (extract_dwo_information): New proc.
> >             (strip_dwo_information): New proc.
> >             (build_executable_from_fission_assembler): Use
> >             extract_dwo_information and strip_dwo_information.
> >             (Dwarf::_debug_addr_index): New variable.
> >             (Dwarf::_cu_is_fission): New variable.
> >             (Dwarf::_handle_DW_FORM): Handle DW_OP_GNU_addr_index.
> >             (Dwarf::_default_form): Supply a default for
> > DW_AT_GNU_addr_base.
> >             (Dwarf::_handle_macro_at_range): Use form
> > DW_FORM_GNU_addr_index
> >             if this is a fission CU.
> >             (Dwarf::_location): Handle DW_OP_GNU_addr_index.
> >             (Dwarf::debug_addr_index): New proc.
> >             (Dwarf::cu): Initialise _cu_is_fission.
> >             (Dwarf::tu): Likewise.
> >             (Dwarf::assemble): Initialise _debug_addr_index.
> >
> > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> > index d6881300fa6..522ccc2506e 100644
> > --- a/gdb/dwarf2/read.c
> > +++ b/gdb/dwarf2/read.c
> > @@ -12581,6 +12581,15 @@ try_open_dwop_file (dwarf2_per_objfile
> > *per_objfile,
> >    else
> >      search_path = debug_file_directory;
> >
> > +  if (getenv ("GDB_NO_FIX") == nullptr)
> > +    {
> > +      /* Add the path for the executable binary to the list of search
> > paths.  */
> > +      search_path_holder.reset
> > +       (concat (ldirname (per_objfile->objfile->original_name).c_str (),
> > +                dirname_separator_string, search_path, nullptr));
> > +      search_path = search_path_holder.get ();
> > +    }
> > +
> >    openp_flags flags = OPF_RETURN_REALPATH;
> >    if (is_dwp)
> >      flags |= OPF_SEARCH_IN_PATH;
> > diff --git a/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.c
> > b/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.c
> > new file mode 100644
> > index 00000000000..27f7f0dfb4b
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.c
> > @@ -0,0 +1,28 @@
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > +   Copyright 2021 Free Software Foundation, Inc.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see
> > +   <http://www.gnu.org/licenses/>.  */
> > +
> > +/* Our fake object.  */
> > +int global_var[100];
> > +
> > +int
> > +main (int argc, char **argv)
> > +{
> > +  asm ("main_label: .globl main_label");
> > +
> > +  return 0;
> > +}
> > diff --git a/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.exp
> > b/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.exp
> > new file mode 100644
> > index 00000000000..f9ab3b8c6a4
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.exp
> > @@ -0,0 +1,145 @@
> > +# Copyright 2021 Free Software Foundation, Inc.
> > +
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +load_lib dwarf.exp
> > +
> > +# This test can only be run on targets which support DWARF-2 and use gas.
> > +if {![dwarf2_support]} {
> > +    return 0
> > +}
> > +
> > +standard_testfile .c -dw.S
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> > +    return -1
> > +}
> > +
> > +set asm_file [standard_output_file $srcfile2]
> > +Dwarf::assemble $asm_file {
> > +    global srcfile gdb_test_file_name
> > +
> > +    set debug_addr_base -1
> > +
> > +    # The information that will be split out into the .dwo file.
> > +    cu {fission 1} {
> > +
> > +       # Capture the current index into .debug_addr so we can fill in
> > +       # DW_AT_GNU_addr_base later.
> > +       set debug_addr_base [debug_addr_index]
> > +
> > +       compile_unit {
> > +            {language @DW_LANG_C}
> > +            {name ${srcfile}}
> > +           {DW_AT_comp_dir .}
> > +           {DW_AT_GNU_dwo_id 0x1234 DW_FORM_data8}
> > +        } {
> > +           declare_labels int4_type struct_type
> > +
> > +           int4_type: DW_TAG_base_type {
> > +               {DW_AT_byte_size 4 DW_FORM_sdata}
> > +               {DW_AT_encoding  @DW_ATE_signed}
> > +               {DW_AT_name      integer}
> > +           }
> > +
> > +           struct_type: DW_TAG_structure_type {
> > +               {DW_AT_name "foo_t"}
> > +               {DW_AT_byte_size 12 DW_FORM_sdata}
> > +           } {
> > +               member {
> > +                   {name "aa"}
> > +                   {type :$int4_type}
> > +                   {data_member_location 0 data1}
> > +               }
> > +               member {
> > +                   {name "bb"}
> > +                   {type :$int4_type}
> > +                   {data_member_location 4 data1}
> > +               }
> > +               member {
> > +                   {name "cc"}
> > +                   {type :$int4_type}
> > +                   {data_member_location 8 data1}
> > +               }
> > +           }
> > +
> > +           DW_TAG_variable {
> > +               {DW_AT_name global_var}
> > +               {DW_AT_type :$struct_type}
> > +               {DW_AT_location {
> > +                   DW_OP_GNU_addr_index [gdb_target_symbol global_var]
> > +               } SPECIAL_expr}
> > +               {external 1 flag}
> > +           }
> > +
> > +           subprogram {
> > +               {external 1 flag}
> > +               {DW_AT_name main DW_FORM_string}
> > +               {MACRO_AT_func {main}}
> > +           }
> > +       }
> > +    }
> > +
> > +    # The information that will remain in the .o file.
> > +    cu {} {
> > +       compile_unit {
> > +            {DW_AT_GNU_dwo_name ${gdb_test_file_name}.dwo DW_FORM_strp}
> > +           {DW_AT_comp_dir .}
> > +           {DW_AT_GNU_dwo_id 0x1234 DW_FORM_data8}
> > +           {DW_AT_GNU_addr_base $debug_addr_base}
> > +        } {
> > +           # Nothing.
> > +       }
> > +    }
> > +}
> > +
> > +# Assemble the file containing the .dwo information, we can then split
> > +# this out into a separate .dwo file.  Once we've extracted the DWO
> > +# information from this object file we're done with it, we recompile
> > +# this again in the BUILD_EXECUTABLE call below.
> > +set object_file [standard_output_file ${binfile}-dwo.o]
> > +if { [gdb_compile $asm_file $object_file object {nodebug}] != "" } {
> > +    return -1
> > +}
> > +
> > +# Split out the dwo sections into a dwo file.
> > +if { [extract_dwo_information $object_file ${binfile}.dwo] == -1 } {
> > +    perror "failed to extract dwo information"
> > +    return -1
> > +}
> > +
> > +# Build the executable, but don't use prepare_for_testing as we don't
> > +# want to start GDB just yet.
> > +if { [build_executable "failed to prepare" "${testfile}" \
> > +         [list $srcfile $asm_file] {nodebug}] } {
> > +    return -1
> > +}
> > +
> > +# Just so we can't be found cheating, strip all the dwo information
> > +# from the executable.
> > +if { [strip_dwo_information ${binfile}] == -1 } {
> > +    perror "failed to strip dwo information"
> > +    return -1
> > +}
> > +
> > +# Now we can start GDB.
> > +clean_restart ${testfile}
> > +
> > +if ![runto_main] {
> > +    return -1
> > +}
> > +
> > +# Print the type of global_var.  This type information is entirely
> > +# fictional, it only exists in the DWARF.  If we don't have the DWARF
> > +# information then there's no way we can print this.
> > +gdb_test "p global_var" " = \\{aa = 0, bb = 0, cc = 0\\}"
> > diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
> > index 4cd5e16c604..a41366f1747 100644
> > --- a/gdb/testsuite/lib/dwarf.exp
> > +++ b/gdb/testsuite/lib/dwarf.exp
> > @@ -28,6 +28,38 @@ proc dwarf2_support {} {
> >      return 0
> >  }
> >
> > +# Use 'objcopy --extract-dwo to extract DWO information from
> > +# OBJECT_FILE and place it into DWO_FILE.
> > +#
> > +# Return 0 on success, otherwise, return -1.
> > +proc extract_dwo_information { object_file dwo_file } {
> > +    set objcopy [gdb_find_objcopy]
> > +    set command "$objcopy --extract-dwo $object_file $dwo_file"
> > +    verbose -log "Executing $command"
> > +    set result [catch "exec $command" output]
> > +    verbose -log "objcopy --extract-dwo output: $output"
> > +    if { $result == 1 } {
> > +       return -1
> > +    }
> > +    return 0
> > +}
> > +
> > +# Use 'objcopy --strip-dwo to remove DWO information from
> > +# FILENAME.
> > +#
> > +# Return 0 on success, otherwise, return -1.
> > +proc strip_dwo_information { filename } {
> > +   set objcopy [gdb_find_objcopy]
> > +    set command "$objcopy --strip-dwo $filename"
> > +    verbose -log "Executing $command"
> > +    set result [catch "exec $command" output]
> > +    verbose -log "objcopy --strip-dwo output: $output"
> > +    if { $result == 1 } {
> > +       return -1
> > +    }
> > +    return 0
> > +}
> > +
> >  # Build an executable from a fission-based .S file.
> >  # This handles the extra work of splitting the .o into non-dwo and dwo
> >  # pieces, making sure the .dwo is available if we're using
> > cc-with-tweaks.sh
> > @@ -54,26 +86,17 @@ proc build_executable_from_fission_assembler {
> > testname executable sources optio
> >      set object_file ${output_base}.o
> >      set dwo_file ${output_base}.dwo
> >      set object_options "object $options"
> > -    set objcopy [gdb_find_objcopy]
> >
> >      set result [gdb_compile $source_file $object_file object $options]
> >      if { "$result" != "" } {
> >         return -1
> >      }
> >
> > -    set command "$objcopy --extract-dwo $object_file $dwo_file"
> > -    verbose -log "Executing $command"
> > -    set result [catch "exec $command" output]
> > -    verbose -log "objcopy --extract-dwo output: $output"
> > -    if { $result == 1 } {
> > +    if { [extract_dwo_information $object_file $dwo_file] == -1 } {
> >         return -1
> >      }
> >
> > -    set command "$objcopy --strip-dwo $object_file"
> > -    verbose -log "Executing $command"
> > -    set result [catch "exec $command" output]
> > -    verbose -log "objcopy --strip-dwo output: $output"
> > -    if { $result == 1 } {
> > +    if { [strip_dwo_information $object_file] == -1 } {
> >         return -1
> >      }
> >
> > @@ -329,6 +352,14 @@ namespace eval Dwarf {
> >      # The address size for debug ranges section.
> >      variable _debug_ranges_64_bit
> >
> > +    # The index into the .debug_addr section (used for fission
> > +    # generation).
> > +    variable _debug_addr_index
> > +
> > +    # Flag, true if the current CU is contains fission information,
> > +    # otherwise false.
> > +    variable _cu_is_fission
> > +
> >      proc _process_one_constant {name value} {
> >         variable _constants
> >         variable _AT
> > @@ -486,6 +517,18 @@ namespace eval Dwarf {
> >                 _op .${_cu_addr_size}byte $value
> >             }
> >
> > +           DW_FORM_GNU_addr_index {
> > +               variable _debug_addr_index
> > +               variable _cu_addr_size
> > +
> > +               _op .uleb128 ${_debug_addr_index}
> > +               incr _debug_addr_index
> > +
> > +               _defer_output .debug_addr {
> > +                   _op .${_cu_addr_size}byte $value
> > +               }
> > +           }
> > +
> >             DW_FORM_data2 -
> >             DW_FORM_ref2 {
> >                 _op .2byte $value
> > @@ -553,7 +596,6 @@ namespace eval Dwarf {
> >             DW_FORM_strx3 -
> >             DW_FORM_strx4 -
> >
> > -           DW_FORM_GNU_addr_index -
> >             DW_FORM_GNU_str_index -
> >
> >             default {
> > @@ -609,6 +651,9 @@ namespace eval Dwarf {
> >             DW_AT_name {
> >                 return DW_FORM_string
> >             }
> > +           DW_AT_GNU_addr_base {
> > +               return DW_FORM_sec_offset
> > +           }
> >         }
> >         return ""
> >      }
> > @@ -649,6 +694,8 @@ namespace eval Dwarf {
> >      # Handle macro attribute MACRO_AT_range.
> >
> >      proc _handle_macro_at_range { attr_value } {
> > +       variable _cu_is_fission
> > +
> >         if {[llength $attr_value] != 1} {
> >             error "usage: MACRO_AT_range { func }"
> >         }
> > @@ -658,10 +705,14 @@ namespace eval Dwarf {
> >         set src ${srcdir}/${subdir}/${srcfile}
> >         set result [function_range $func $src]
> >
> > -       _handle_attribute DW_AT_low_pc [lindex $result 0] \
> > -           DW_FORM_addr
> > +       set form DW_FORM_addr
> > +       if { $_cu_is_fission } {
> > +           set form DW_FORM_GNU_addr_index
> > +       }
> > +
> > +       _handle_attribute DW_AT_low_pc [lindex $result 0] $form
> >         _handle_attribute DW_AT_high_pc \
> > -           "[lindex $result 0] + [lindex $result 1]" DW_FORM_addr
> > +           "[lindex $result 0] + [lindex $result 1]" $form
> >      }
> >
> >      # Handle macro attribute MACRO_AT_func.
> > @@ -929,6 +980,18 @@ namespace eval Dwarf {
> >                     _op .${addr_size}byte [lindex $line 1]
> >                 }
> >
> > +               DW_OP_GNU_addr_index {
> > +                   variable _debug_addr_index
> > +                   variable _cu_addr_size
> > +
> > +                   _op .uleb128 ${_debug_addr_index}
> > +                   incr _debug_addr_index
> > +
> > +                   _defer_output .debug_addr {
> > +                       _op .${_cu_addr_size}byte [lindex $line 1]
> > +                   }
> > +               }
> > +
> >                 DW_OP_regx {
> >                     _op .uleb128 [lindex $line 1]
> >                 }
> > @@ -1050,6 +1113,14 @@ namespace eval Dwarf {
> >         }
> >      }
> >
> > +    # Return the current value of _DEBUG_ADDR_INDEX.  The return value
> > +    # of this function can be used for the value of
> > +    # DW_AT_GNU_addr_base.
> > +    proc debug_addr_index {} {
> > +       variable _debug_addr_index
> > +       return ${_debug_addr_index}
> > +    }
> > +
> >      # Emit a DWARF CU.
> >      # OPTIONS is a list with an even number of elements containing
> >      # option-name and option-value pairs.
> > @@ -1073,12 +1144,13 @@ namespace eval Dwarf {
> >         variable _cu_version
> >         variable _cu_addr_size
> >         variable _cu_offset_size
> > +       variable _cu_is_fission
> >
> >         # Establish the defaults.
> >         set is_64 0
> >         set _cu_version 4
> >         set _cu_addr_size default
> > -       set fission 0
> > +       set _cu_is_fission 0
> >         set section ".debug_info"
> >         set _abbrev_section ".debug_abbrev"
> >
> > @@ -1088,7 +1160,7 @@ namespace eval Dwarf {
> >                 is_64 { set is_64 $value }
> >                 version { set _cu_version $value }
> >                 addr_size { set _cu_addr_size $value }
> > -               fission { set fission $value }
> > +               fission { set _cu_is_fission $value }
> >                 default { error "unknown option $name" }
> >             }
> >         }
> > @@ -1100,7 +1172,7 @@ namespace eval Dwarf {
> >             }
> >         }
> >         set _cu_offset_size [expr { $is_64 ? 8 : 4 }]
> > -       if { $fission } {
> > +       if { $_cu_is_fission } {
> >             set section ".debug_info.dwo"
> >             set _abbrev_section ".debug_abbrev.dwo"
> >         }
> > @@ -1180,12 +1252,13 @@ namespace eval Dwarf {
> >         variable _cu_version
> >         variable _cu_addr_size
> >         variable _cu_offset_size
> > +       variable _cu_is_fission
> >
> >         # Establish the defaults.
> >         set is_64 0
> >         set _cu_version 4
> >         set _cu_addr_size default
> > -       set fission 0
> > +       set _cu_is_fission 0
> >         set section ".debug_types"
> >         set _abbrev_section ".debug_abbrev"
> >
> > @@ -1194,7 +1267,7 @@ namespace eval Dwarf {
> >                 is_64 { set is_64 $value }
> >                 version { set _cu_version $value }
> >                 addr_size { set _cu_addr_size $value }
> > -               fission { set fission $value }
> > +               fission { set _cu_is_fission $value }
> >                 default { error "unknown option $name" }
> >             }
> >         }
> > @@ -1206,7 +1279,7 @@ namespace eval Dwarf {
> >             }
> >         }
> >         set _cu_offset_size [expr { $is_64 ? 8 : 4 }]
> > -       if { $fission } {
> > +       if { $_cu_is_fission } {
> >             set section ".debug_types.dwo"
> >             set _abbrev_section ".debug_abbrev.dwo"
> >         }
> > @@ -2056,6 +2129,7 @@ namespace eval Dwarf {
> >         variable _line_saw_program
> >         variable _line_header_end_label
> >         variable _debug_ranges_64_bit
> > +       variable _debug_addr_index
> >
> >         if {!$_initialized} {
> >             _read_constants
> > @@ -2074,6 +2148,8 @@ namespace eval Dwarf {
> >         set _line_saw_program 0
> >         set _debug_ranges_64_bit [is_64_target]
> >
> > +       set _debug_addr_index 0
> > +
> >         # Not "uplevel" here, because we want to evaluate in this
> >         # namespace.  This is somewhat bad because it means we can't
> >         # readily refer to outer variables.
> >