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

[Caroline Tice <cmtice@google.com>]

Ping?  Could somebody look at this please?  Thanks!

-- Caroline Tice
cmtice@google.com

On Thu, Mar 4, 2021 at 7:48 AM Caroline Tice <cmtice@google.com> wrote:
>
> 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), 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.
>
> As an alternative I created 4 different versions of the test: x86_64,
> i386,  arm32 and aarch64.   I have attached a new version of the patch
> with these changes.
>
>  Is this ok to commit now?
>
> -- Caroline
> cmtice@google.com
>
> gdb/ChangeLog
>
> 2021-03-04  Caroline Tice  <cmtice@google.com>
>
>         * 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
>
> 2021-03-04  Caroline Tice <cmtice@google.com>
>
>         * gdb.dwarf2/fission-relative-dwo.c: New file.
>         * gdb.dwarf2/fission-relative-dwo-x86_64.S: New file.
>         * gdb.dwarf2/fission-relative-dwo-x86_64.exp: New file.
>         * gdb.dwarf2/fission-relative-dwo-i386.S: New file.
>         * gdb.dwarf2/fission-relative-dwo-i386.exp: New file.
>         * gdb.dwarf2/fission-relative-dwo-aarch64.S: New file.
>         * gdb.dwarf2/fission-relative-dwo-aarch64.exp: New file.
>         * gdb.dwarf2/fission-relative-dwo-arm32.S: New file.
>         * gdb.dwarf2/fission-relative-dwo-arm32.exp: New file.
>
>
>
> On Wed, Mar 3, 2021 at 1:53 AM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> >
> > * Caroline Tice via Gdb-patches <gdb-patches@sourceware.org> [2021-03-02 16:42:08 -0800]:
> >
> > > 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.
> > >
> > > I have run this through the regression testsuite with no regressions,
> > > and I have verified that my new testcase passes with
> > > this patch and fails without it.
> > >
> > > Is this patch OK to commit?
> > >
> > > -- Caroline Tice
> > > cmtice@google.com
> > >
> > > gdb/ChangeLog
> > >
> > > 2021-03-02  Caroline Tice  <cmtice@google.com>
> > >
> > >         * 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
> > >
> > > 2021-03-02  Caroline Tice <cmtice@google.com>
> > >
> > >         * gdb.dwarf2/fission-relative-dwo.c: New file.
> > >         * gdb.dwarf2/fission-relative-dwo.S: New file.
> > >         * gdb.dwarf2/fission-relative-dwo.exp: New file.
> >
> > > From 83056b931839107f131c0059377a011876df051b Mon Sep 17 00:00:00 2001
> > > From: Caroline Tice <cmtice@google.com>
> > > Date: Tue, 2 Mar 2021 16:15:54 -0800
> > > Subject: [PATCH] Add directory containing executable to relative search paths
> > >  for .dwo files.
> > >
> > > 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,
> > > which 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/dwarf2/read.c                             |   6 +
> > >  .../gdb.dwarf2/fission-relative-dwo.S         | 407 ++++++++++++++++++
> > >  .../gdb.dwarf2/fission-relative-dwo.c         |  10 +
> > >  .../gdb.dwarf2/fission-relative-dwo.exp       |  52 +++
> > >  4 files changed, 475 insertions(+)
> > >  create mode 100644 gdb/testsuite/gdb.dwarf2/fission-relative-dwo.S
> > >  create mode 100644 gdb/testsuite/gdb.dwarf2/fission-relative-dwo.c
> > >  create mode 100644 gdb/testsuite/gdb.dwarf2/fission-relative-dwo.exp
> > >
> > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> > > index d4f13229ced..7cac5027d41 100644
> > > --- a/gdb/dwarf2/read.c
> > > +++ b/gdb/dwarf2/read.c
> > > @@ -12809,6 +12809,12 @@ try_open_dwop_file (dwarf2_per_objfile *per_objfile,
> > >    else
> > >      search_path = debug_file_directory;
> > >
> > > +  /* 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, (char *) NULL));
> > > +  search_path = search_path_holder.get();
> >
> > Missing whitespace before many of the '(' in this code.  Also I think
> > you should replace '(char *) NULL' with just 'nullptr'.
> >
> > > +
> > >    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..4c5f0b88b0b
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.c
> > > @@ -0,0 +1,10 @@
> > > +
> > > +int main (int argc, char **argv)
> > > +{
> > > +  int x = 10;
> > > +  char y[3] = { 'a', 'b', 'c' };
> > > +
> > > +  x += (int) y[0] + (int) y[1];
> > > +
> > > +  return 0;
> > > +}
> >
> > Test source files should include a copyright header, and should follow
> > GNU/GDB coding standard, unless the deviation is critical for the test
> > (which is not the case here).
> >
> >
> > > 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..3d0b9b9fb5f
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.dwarf2/fission-relative-dwo.exp
> > > @@ -0,0 +1,52 @@
> > > +# Copyright 2013-2021 Free Software Foundation, Inc.
> >
> > The from date should represent the first time you posted this patch to
> > the mailing list.  I didn't check to see if this patch was posted in
> > 2013.
> >
> > > +
> > > +# 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
> > > +
> > > +# We run objcopy locally to split out the .dwo file.
> > > +if [is_remote host] {
> > > +    return 0
> > > +}
> > > +
> > > +# This test can only be run on targets which support DWARF-2 and use gas.
> > > +if ![dwarf2_support] {
> > > +    return 0
> > > +}
> > > +
> > > +# This test can only be run on x86-64 targets.
> > > +if {![istarget x86_64-*] || ![is_lp64_target]} {
> > > +    return 0
> > > +}
> > > +
> > > +standard_testfile .S
> > > +
> > > +set dwo [standard_output_file "fission-relative-dwo.dwo"]
> > > +
> > > +if [build_executable_from_fission_assembler \
> > > +     "$testfile.exp" "$binfile" "$srcfile" \
> > > +     [list nodebug additional_flags=-DDWO=$dwo]] {
> > > +    return -1
> > > +}
> >
> > Rather than using a fixed assembler file I would like to suggest you
> > write this test using GDB's DWARF assembler functionality.  This will
> > allow the test to run for any target, not just x86-64.
> >
> > It's certainly easy to set the DW_AT_comp_dir to any value you need
> > using that tool.  From your patch description I don't know what else
> > you would need to modify in the DWARF, maybe nothing?
> >
> > If you look for .exp files containing 'Dwarf::assemble' you'll find
> > loads of examples that you can draw inspiration from, but feel free to
> > ask if you have specific questions.
> >
> > > +
> > > +clean_restart $binfile
> > > +
> > > +if ![runto_main] {
> > > +    return -1
> > > +}
> > > +
> > > +# Verify gdb can find argc.
> > > +
> > > +gdb_test "p argc" " = 1"
> >
> > I guess you're looking for argc just to prove that GDB has found the
> > DWO file.
> >
> > If you switch to GDB's DWARF assembler then generating debug
> > information to find a local variable is harder.  I would suggest that
> > instead, you make the test file something like this:
> >
> >   int global_var;
> >
> >   int
> >   main (void)
> >   {
> >     asm ("main_label: .globl main_label");
> >     return 0;
> >   }
> >
> > Then in the generated DWARF give global_var a typedef type, your test
> > then becomes something like:
> >
> >   gdb_test "ptype global_var" "type = ....."
> >
> > Thanks,
> > Andrew