[PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Sourabh Singh Tomar]

Consider following C test case:
```
cat test.c
int main() {
        int a = OUT;
}
```
Compiled as:
`clang -gdwarf-5 -fdebug-macro test.c`

Loading this bin into GDB and trying to access macro info,
GDB is unable to extract and display macro info correctly.
```
$gdb a.out
(gdb) start
(gdb) info macro OUT
Temporary breakpoint 1 at 0x2016a8: file test.c, line 3.
Starting program: /home/a.out
(gdb) info macro OUT
The symbol `OUT' has no definition as a C/C++ preprocessor macro
at /home/test.c:-1
```

The problems starts when processing file and dir indexes from DWARFv5
line table.  DWARFv5 specifies:
  - The current directory is explicitly present in the
    directories field.
  - The current compilation filename is explicitly present and
    has index 0.
dir entry 0 contains the absoute path(Not including source
filename).
file entry 0 contains the source file name and refers dir 0.

As one may notice that dir index `0` will always contain
absoulte path(Not including the file name as), this causes
GDB to append the path + filename to construt macro info
`main_file->filename`. This leads to overriding of the macro filename
which will cause macro info lookup failure due to mismatch in
`main_file->filename` and `sal.symtab->filename`.

This problem does not pops up with GCC compiled binaries, since
GCC does not emits DWARFv5 compliant line tables.

Testing:
- NO regressing observed in `gdb.base` and associated macro based
tests.

gdb/ChangeLog
2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>

       * dwarf2/line-header.c: Updated the comment and extended
       check in function `line_header::file_file_name` for
       DWARFv5 line tables.

gdb/testsuite/ChangeLog
2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>

       * gdb.base/dwarf5-macro.exp: Add new tests.
       * gdb.base/dwarf5-macro.c: Likewise.

Change-Id: I09f39c8680fca382523f4ac72bcbd1018851b480
---
 gdb/ChangeLog                           |  5 +++
 gdb/dwarf2/line-header.c                | 11 +++++--
 gdb/testsuite/ChangeLog                 |  5 +++
 gdb/testsuite/gdb.base/dwarf5-macro.c   |  3 ++
 gdb/testsuite/gdb.base/dwarf5-macro.exp | 43 +++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dwarf5-macro.c
 create mode 100644 gdb/testsuite/gdb.base/dwarf5-macro.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d564621f4c6..98650ff8a60 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>
+
+	* dwarf2/line-header.c: Update the comment and extended
+	check in function `line_header::file_file_name` for
+	DWARFv5 line tables.
 2021-05-12  George Barrett  <bob@bob131.so>
 
 	* NEWS (Guile API): Note the addition of the new procedure.
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 7575297f966..d79051d326e 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -64,7 +64,8 @@ gdb::unique_xmalloc_ptr<char>
 line_header::file_file_name (int file) const
 {
   /* Is the file number a valid index into the line header's file name
-     table?  Remember that file numbers start with one, not zero.  */
+	table?  Remember that file numbers start with zero for DWARFv5 and one
+	for DWARFv4 .  */
   if (is_valid_file_index (file))
     {
       const file_entry *fe = file_name_at (file);
@@ -72,7 +73,10 @@ line_header::file_file_name (int file) const
       if (!IS_ABSOLUTE_PATH (fe->name))
 	{
 	  const char *dir = fe->include_dir (this);
-	  if (dir != NULL)
+	  /* The file at index 0 (valid starting with DWARF 5) is already
+	      relative to the compilation directory.  */
+
+	  if (file != 0)
 	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
 							  fe->name,
 							  (char *) NULL));
@@ -100,7 +104,8 @@ gdb::unique_xmalloc_ptr<char>
 line_header::file_full_name (int file, const char *comp_dir) const
 {
   /* Is the file number a valid index into the line header's file name
-     table?  Remember that file numbers start with one, not zero.  */
+	table?  Remember that file numbers start with zero for DWARFv5 and one
+	for DWARFv4 .  */
   if (is_valid_file_index (file))
     {
       gdb::unique_xmalloc_ptr<char> relative = file_file_name (file);
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index e0f64965d94..66b395b1e4c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>
+
+	* gdb.base/dwarf5-macro.exp: Add new tests.
+	* gdb.base/dwarf5-macro.c: Likewise.
+
 2021-05-12  George Barrett  <bob@bob131.so>
 
 	* gdb.guile/scm-value.exp (test_value_in_inferior): Add test for
diff --git a/gdb/testsuite/gdb.base/dwarf5-macro.c b/gdb/testsuite/gdb.base/dwarf5-macro.c
new file mode 100644
index 00000000000..0a7dc98c620
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dwarf5-macro.c
@@ -0,0 +1,3 @@
+#define HELLO_GDB 1
+int main() {
+}
diff --git a/gdb/testsuite/gdb.base/dwarf5-macro.exp b/gdb/testsuite/gdb.base/dwarf5-macro.exp
new file mode 100644
index 00000000000..cf622cebe32
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dwarf5-macro.exp
@@ -0,0 +1,43 @@
+# Copyright 2020-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/>.
+
+standard_testfile
+set additional_flags {debug}
+file copy -force $srcdir/$subdir/$srcfile [standard_output_file $srcfile]
+gdb_compile outputs/gdb.base/dwarf5-macro/dwarf5-macro.c $binfile executable {debug}
+
+get_compiler_info
+if { [test_compiler_info "gcc-*"] } {
+    set options "additional_flags=-g3"
+} elseif { [test_compiler_info "clang-*"] } {
+    set options "additional_flags=-gdwarf-5 -fdebug-macro"
+}
+
+if { [prepare_for_testing "failed to prepare" \
+   ${testfile} ${srcfile}] } {
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return
+}
+
+set filepat {dwarf5-macro.[ch]}
+set definition {}
+set location {}
+set nonzero {[1-9][0-9]*}
+gdb_test "info macro HELLO_GDB" \
+	     ".*define HELLO_GDB 1"
-- 
2.17.1

RE: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Tomar, Sourabh Singh]

[AMD Public Use]

Hello Simon,

Thanks for the review comments.

There was some configuration issue with my git send-mail thing*, apparently the patch arrived but missed
the reply to your review comments.

I'll try to address them here:

Comments:
> Sorry for the delay, there are just so many patches to look at.

No Problem, thanks for taking out time for reviewing this!

>Ok, I think you're right.  We want line_header::file_file_name to return the path to the file, relative to the compilation directory.  In DWARF5, at least with clang, the file at index 0 appears to always be >relative to the compilation directory (directory at index 0).
If I do:
    $ clang-11 test.c -o test -gdwarf-5 -fdebug-macro && llvm-dwarfdump -F -color -debug-line test
I get:
....
If I move up a directory:
    $ clang-11 gdb/test.c -o test -gdwarf-5 -fdebug-macro && llvm-dwarfdump -F -color -debug-line test
    ...
> So it looks like by returning the filename as-is, we'll indeed return a path to the file relative to the compilation directory.

Yes, thanks for acknowledging the issue.

> I made a test (took gdb.base/template.exp and modified it) with:
>    standard_testfile
>    file copy -force $srcdir/$subdir/$srcfile [standard_output_file $srcfile]
>    gdb_compile outputs/gdb.base/template/template.c $binfile executable {debug}
> This copies the source file in the "output" directory of the test, and compiles it from there using a relative path.  You can run it with clang & DWARF5 with:
>    $ make check TESTS="gdb.base/template.exp" RUNTESTFLAGS="--target_board=unix/gdb:debug_flags=-gdwarf-5 CC_FOR_TARGET=clang"
> So I think you could make a test based on this.  Note that the test should work with gcc too, where you need to use -g3 in order to get macro information.  A few tests (gdb.base/info-macros.exp, >gdb.base/macscp.exp, gdb.base/style.exp,
>gdb.linespec/macro-relative.exp) already deal with this, you can take a look.  Speaking of gdb.linespec/macro-relative.exp, it would be worth looking into it, it kind of looks related, but I am not sure.

Thanks for the pointers, I've added a minimal test case that passes after this patch.

> For next time, could you please try to send your patch using git-send-email?  It makes it easier to comment inline.

Sure, patch is sent through git-send-email

Review comment addressed in this patch:
> Use present tense (e.g. Update, not Updated).
> Watch the indentation.
> You can shorten this comment to:
> /* The file at index 0 (valid starting with DWARF 5) is already relative
>   to the compilation directory.  */
> Use `file != 0`.

gdb/ChangeLog
    2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>

           * dwarf2/line-header.c: Updated the comment and extended
           check in function `line_header::file_file_name` for
           DWARFv5 line tables.

    gdb/testsuite/ChangeLog
    2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>

           * gdb.base/dwarf5-macro.exp: Add new tests.
           * gdb.base/dwarf5-macro.c: Likewise.


Thanks,
Sourabh Singh tomar

-----Original Message-----
From: Tomar, Sourabh Singh <SourabhSingh.Tomar@amd.com> 
Sent: Wednesday, May 12, 2021 10:47 PM
To: gdb-patches@sourceware.org
Cc: Tomar, Sourabh Singh <SourabhSingh.Tomar@amd.com>
Subject: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

Consider following C test case:
```
cat test.c
int main() {
        int a = OUT;
}
```
Compiled as:
`clang -gdwarf-5 -fdebug-macro test.c`

Loading this bin into GDB and trying to access macro info, GDB is unable to extract and display macro info correctly.
```
$gdb a.out
(gdb) start
(gdb) info macro OUT
Temporary breakpoint 1 at 0x2016a8: file test.c, line 3.
Starting program: /home/a.out
(gdb) info macro OUT
The symbol `OUT' has no definition as a C/C++ preprocessor macro at /home/test.c:-1 ```

The problems starts when processing file and dir indexes from DWARFv5 line table.  DWARFv5 specifies:
  - The current directory is explicitly present in the
    directories field.
  - The current compilation filename is explicitly present and
    has index 0.
dir entry 0 contains the absoute path(Not including source filename).
file entry 0 contains the source file name and refers dir 0.

As one may notice that dir index `0` will always contain absoulte path(Not including the file name as), this causes GDB to append the path + filename to construt macro info `main_file->filename`. This leads to overriding of the macro filename which will cause macro info lookup failure due to mismatch in `main_file->filename` and `sal.symtab->filename`.

This problem does not pops up with GCC compiled binaries, since GCC does not emits DWARFv5 compliant line tables.

Testing:
- NO regressing observed in `gdb.base` and associated macro based tests.

gdb/ChangeLog
2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>

       * dwarf2/line-header.c: Updated the comment and extended
       check in function `line_header::file_file_name` for
       DWARFv5 line tables.

gdb/testsuite/ChangeLog
2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>

       * gdb.base/dwarf5-macro.exp: Add new tests.
       * gdb.base/dwarf5-macro.c: Likewise.

Change-Id: I09f39c8680fca382523f4ac72bcbd1018851b480
---
 gdb/ChangeLog                           |  5 +++
 gdb/dwarf2/line-header.c                | 11 +++++--
 gdb/testsuite/ChangeLog                 |  5 +++
 gdb/testsuite/gdb.base/dwarf5-macro.c   |  3 ++
 gdb/testsuite/gdb.base/dwarf5-macro.exp | 43 +++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 3 deletions(-)  create mode 100644 gdb/testsuite/gdb.base/dwarf5-macro.c
 create mode 100644 gdb/testsuite/gdb.base/dwarf5-macro.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog index d564621f4c6..98650ff8a60 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>
+
+	* dwarf2/line-header.c: Update the comment and extended
+	check in function `line_header::file_file_name` for
+	DWARFv5 line tables.
 2021-05-12  George Barrett  <bob@bob131.so>
 
 	* NEWS (Guile API): Note the addition of the new procedure.
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c index 7575297f966..d79051d326e 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -64,7 +64,8 @@ gdb::unique_xmalloc_ptr<char>  line_header::file_file_name (int file) const  {
   /* Is the file number a valid index into the line header's file name
-     table?  Remember that file numbers start with one, not zero.  */
+	table?  Remember that file numbers start with zero for DWARFv5 and one
+	for DWARFv4 .  */
   if (is_valid_file_index (file))
     {
       const file_entry *fe = file_name_at (file); @@ -72,7 +73,10 @@ line_header::file_file_name (int file) const
       if (!IS_ABSOLUTE_PATH (fe->name))
 	{
 	  const char *dir = fe->include_dir (this);
-	  if (dir != NULL)
+	  /* The file at index 0 (valid starting with DWARF 5) is already
+	      relative to the compilation directory.  */
+
+	  if (file != 0)
 	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
 							  fe->name,
 							  (char *) NULL));
@@ -100,7 +104,8 @@ gdb::unique_xmalloc_ptr<char>  line_header::file_full_name (int file, const char *comp_dir) const  {
   /* Is the file number a valid index into the line header's file name
-     table?  Remember that file numbers start with one, not zero.  */
+	table?  Remember that file numbers start with zero for DWARFv5 and one
+	for DWARFv4 .  */
   if (is_valid_file_index (file))
     {
       gdb::unique_xmalloc_ptr<char> relative = file_file_name (file); diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index e0f64965d94..66b395b1e4c 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-12  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>
+
+	* gdb.base/dwarf5-macro.exp: Add new tests.
+	* gdb.base/dwarf5-macro.c: Likewise.
+
 2021-05-12  George Barrett  <bob@bob131.so>
 
 	* gdb.guile/scm-value.exp (test_value_in_inferior): Add test for diff --git a/gdb/testsuite/gdb.base/dwarf5-macro.c b/gdb/testsuite/gdb.base/dwarf5-macro.c
new file mode 100644
index 00000000000..0a7dc98c620
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dwarf5-macro.c
@@ -0,0 +1,3 @@
+#define HELLO_GDB 1
+int main() {
+}
diff --git a/gdb/testsuite/gdb.base/dwarf5-macro.exp b/gdb/testsuite/gdb.base/dwarf5-macro.exp
new file mode 100644
index 00000000000..cf622cebe32
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dwarf5-macro.exp
@@ -0,0 +1,43 @@
+# Copyright 2020-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/>.
+
+standard_testfile
+set additional_flags {debug}
+file copy -force $srcdir/$subdir/$srcfile [standard_output_file 
+$srcfile] gdb_compile outputs/gdb.base/dwarf5-macro/dwarf5-macro.c 
+$binfile executable {debug}
+
+get_compiler_info
+if { [test_compiler_info "gcc-*"] } {
+    set options "additional_flags=-g3"
+} elseif { [test_compiler_info "clang-*"] } {
+    set options "additional_flags=-gdwarf-5 -fdebug-macro"
+}
+
+if { [prepare_for_testing "failed to prepare" \
+   ${testfile} ${srcfile}] } {
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return
+}
+
+set filepat {dwarf5-macro.[ch]}
+set definition {}
+set location {}
+set nonzero {[1-9][0-9]*}
+gdb_test "info macro HELLO_GDB" \
+	     ".*define HELLO_GDB 1"
--
2.17.1

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Tom Tromey]

>>>>> ">" == Sourabh Singh Tomar via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +standard_testfile
>> +set additional_flags {debug}
>> +file copy -force $srcdir/$subdir/$srcfile [standard_output_file $srcfile]
>> +gdb_compile outputs/gdb.base/dwarf5-macro/dwarf5-macro.c $binfile executable {debug}

If the copying is really needed, then a comment explaining why would be
helpful.

I'm not sure if "file copy" can be used.  You may want to see what other
tests do, I think maybe dejagnu provides some wrapper.  Not really sure.

This set 'additional_flags' but doesn't use it.  You could just drop the
assignment.

>> +if { [test_compiler_info "gcc-*"] } {
>> +    set options "additional_flags=-g3"
>> +} elseif { [test_compiler_info "clang-*"] } {
>> +    set options "additional_flags=-gdwarf-5 -fdebug-macro"

It seems like you'd want to pass -gdwarf-5 to gcc as well?

>> +if { [prepare_for_testing "failed to prepare" \
>> +   ${testfile} ${srcfile}] } {

No need to break this line, I think.

>> +set filepat {dwarf5-macro.[ch]}
>> +set definition {}
>> +set location {}
>> +set nonzero {[1-9][0-9]*}

None of these are used.

Tom

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Simon Marchi]

On 2021-05-12 1:16 p.m., Sourabh Singh Tomar via Gdb-patches wrote:
> Consider following C test case:
> ```
> cat test.c
> int main() {
>         int a = OUT;
> }
> ```
> Compiled as:
> `clang -gdwarf-5 -fdebug-macro test.c`
> 
> Loading this bin into GDB and trying to access macro info,
> GDB is unable to extract and display macro info correctly.
> ```
> $gdb a.out
> (gdb) start
> (gdb) info macro OUT
> Temporary breakpoint 1 at 0x2016a8: file test.c, line 3.
> Starting program: /home/a.out
> (gdb) info macro OUT
> The symbol `OUT' has no definition as a C/C++ preprocessor macro
> at /home/test.c:-1
> ```
> 
> The problems starts when processing file and dir indexes from DWARFv5
> line table.  DWARFv5 specifies:
>   - The current directory is explicitly present in the
>     directories field.
>   - The current compilation filename is explicitly present and
>     has index 0.
> dir entry 0 contains the absoute path(Not including source
> filename).

absoute -> absolute

You could say that it contains the "compilation directory", typically
the current working directory of the compiler.

> diff --git a/gdb/testsuite/gdb.base/dwarf5-macro.c b/gdb/testsuite/gdb.base/dwarf5-macro.c
> new file mode 100644
> index 00000000000..0a7dc98c620
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dwarf5-macro.c
> @@ -0,0 +1,3 @@
> +#define HELLO_GDB 1
> +int main() {
> +}

Please add a copyright notice, like in the other .c files.

> diff --git a/gdb/testsuite/gdb.base/dwarf5-macro.exp b/gdb/testsuite/gdb.base/dwarf5-macro.exp
> new file mode 100644
> index 00000000000..cf622cebe32
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/dwarf5-macro.exp
> @@ -0,0 +1,43 @@
> +# Copyright 2020-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/>.

For others in the future to be able to understand why this test exists,
please add a comment here explaining what this test case is about, what
is it specifically meant to check.  It would be good to note that for
the bug to reproduce, the source file must be supplied as a relative
path, so that the file_names[0] entry in .debug_line ends up relative
and not absolute.  That will help justify why we do a copy below, and
why we pass such an hardcoded path to gdb_compile.  This is unusual, so
somebody might be tempted to "clean this up", breaking the purpose of
the test.

> +
> +standard_testfile
> +set additional_flags {debug}
> +file copy -force $srcdir/$subdir/$srcfile [standard_output_file $srcfile]
> +gdb_compile outputs/gdb.base/dwarf5-macro/dwarf5-macro.c $binfile executable {debug}

We typically check for gdb_compile failures, check for examples
elsewhere in the testsuite.

As Tom said, please comment adequatly to explain why we do it this way.

It would perhaps make sense to move this test to gdb.dwarf2?  Also, the
name "dwarf5-macro" is a bit broad.  If we want to add other tests
related to macro handling in DWARF5, we'll be a bit stuck (we'll have to
rename this one).  I would suggest giving it a more specific name, like
dwarf5-macro-relative-path", something like that.

> +
> +get_compiler_info
> +if { [test_compiler_info "gcc-*"] } {
> +    set options "additional_flags=-g3"
> +} elseif { [test_compiler_info "clang-*"] } {
> +    set options "additional_flags=-gdwarf-5 -fdebug-macro"
> +}
> +
> +if { [prepare_for_testing "failed to prepare" \

prepare_for_testing builds the executable, doesn't it?  And it ends up
overwriting the file you compiled above, don't you see that?  The test
fails for me because of this, I think.

Simon

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Keith Seitz]

Coincidentally, I happened to be looking at gdb/27306 about a similar
problem, and I was curious to see how our patches interacted.

On 5/12/21 10:16 AM, Sourabh Singh Tomar via Gdb-patches wrote:

> As one may notice that dir index `0` will always contain
> absoulte path(Not including the file name as), this causes
> GDB to append the path + filename to construt macro info
> `main_file->filename`. This leads to overriding of the macro filename
> which will cause macro info lookup failure due to mismatch in
> `main_file->filename` and `sal.symtab->filename`.

gdb/27306 highlights a similar problem with gcc using -gdwarf-5:

cat -n undef-macro.c
     1	#define FOO 1
     2	
     3	int main(int argc, char **argv)
     4	{
     5	  return FOO;
     6	}
$ gcc -gdwarf-5 undef-macro.c -o undef-macro
$ ./gdb -nx -q undef-macro -ex start -ex "p FOO"
(gdb) start
Temporary breakpoint 1 at 0x401111: file foo.c, line 5.
Starting program: undef-macro 

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdd48) at undef-macro.c:5
5	  return FOO;
No symbol "FOO" in current context.

The problem is that file name returned by file_file_name is /not/ relative to
the compilation directory. Ever.

From file_file_name:

      const file_entry *fe = file_name_at (file);

      if (!IS_ABSOLUTE_PATH (fe->name))
        {
          const char *dir = fe->include_dir (this);
          /* The file at index 0 (valid starting with DWARF 5) is already
              relative to the compilation directory.  */

          if (file != 0)
            return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
                                                          fe->name,
                                                          (char *) NULL));

Even with your patch, gdb will still return an absolute path for
the above test.

As you've already mentioned, macro_start_file and macro_lookup_inclusion
require that macro_source_file.filename be relative to the compliation
directory. Furthermore, the documentation for macro_source_file.filename
is stricter:

  /* A source file --- possibly a header file.  This filename is relative to
     the compilation directory (table->comp_dir), it exactly matches the
     symtab->filename content.  */
  const char *filename;

So if symtab->filename and macro_source_filename are required to be the
same, why are we bothering with prepending directories? We already have
access to the symtab in `fe'.

This is the version I was playing with...

diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 7575297f966..117c5a42cc5 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
     {
       const file_entry *fe = file_name_at (file);
 
-      if (!IS_ABSOLUTE_PATH (fe->name))
-	{
-	  const char *dir = fe->include_dir (this);
-	  if (dir != NULL)
-	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
-							  fe->name,
-							  (char *) NULL));
-	}
-      return make_unique_xstrdup (fe->name);
+      /* macro_source_file requires: "This filename is relative to the
+	 compilation directory, it exactly matches the symtab->filename
+	 content."  */
+      return make_unique_xstrdup (fe->symtab->filename);
     }
   else
     {

By happy coincidence, this has the same effect of your patch, but it also
fixes gdb/27306.

I checked the origins of file_file_name and it appears that this function
was split out from file_full_name, but this directory prepending bit was
never removed to fulfill the "relative" requirement of the stated documentation.
[This is from commit 233d95b548ec948c4a6d01cd05c307385dd615fb.]

I hate to post counter-patches, but in this case, I think we are really
both attempting to solve the same design problem with line_header::file_file_name.

> This problem does not pops up with GCC compiled binaries, since
> GCC does not emits DWARFv5 compliant line tables.

This test (as written) does not pass using gcc:

$ make check TESTS=gdb.base/dwarf5-macro.exp
[snip]
Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/dwarf5-macro.exp ...
FAIL: gdb.base/dwarf5-macro.exp: info macro HELLO_GDB

		=== gdb Summary ===

# of unexpected failures	1

However, if I force -gdwarf-5 for gcc, too:

$ make check TESTS=gdb.base/dwarf5-macro.exp RUNTESTFLAGS="unix/-g3/-gdwarf-5"
[snip]
Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/dwarf5-macro.exp ...

		=== gdb Summary ===

# of expected passes		1

I suggest that be corrected -- just force gcc to emit dwarf-5 debuginfo as
well. IMO, the correct, compiler-agnostic way of dealing with this is via
the DWARF assembler, but I am not going to suggest you do that.

I'd be happy if the test simply didn't FAIL on non-clang targets.

Keith

RE: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Tomar, Sourabh Singh]

[AMD Public Use]


Hi Keith,

Thanks for the review!

>$ gcc -gdwarf-5 undef-macro.c -o undef-macro $ ./gdb -nx -q undef-macro -ex start -ex "p FOO"
>(gdb) start
>Temporary breakpoint 1 at 0x401111: file foo.c, line 5.
>Starting program: undef-macro
>Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdd48) at undef-macro.c:5
>5         return FOO;
>No symbol "FOO" in current context

Ah, I also observed this with today(rebased) GCC (GNU C17 12.0.0 20210514), seems like things changed (Not for good) in GCC,
My patch was initially posted in some March21 time frame at that time this was working(for GCC) ☹

One thing I noticed that changed is that GCC is now emitting `dir0` entry in line table as per DWARF5. However this still seems
Incomplete to me:
$llvm-dwarfdump --debug-line a.out
```
include_directories[  1] = "/usr/include" // index still starting from `1` even at "-gdwarf-5"
file_names[  1]:
           name: "macro.c"
      dir_index: 0               // -> dir 0 mentioned but dir 0 is not present in list of directories, i.e index still starting from 1
       mod_time: 0x00000000
         length: 0x00000000
file_names[  2]:
           name: "stdc-predef.h"
      dir_index: 1
       mod_time: 0x00000000
         length: 0x00000000
```
Investigation: With or without my fix.
For investigating primary data-structures involve in this lookup i.e "main_file->filename, sal.symtab->filename"
A simple printf insertion in gdb/macroscope.c, provides further insights for how GDB is handling GCC generated DWARF5
Binaries.
$tot-gcc -gdwarf-5 -g3 macro.c -o a.out.gcc
$gdb a.out.gcc
(gdb) start
(gdb) info macros
(gdb) info macros
**main_file->filename ->    // Empty**
sal.symtab->filename -> macro.c

So it seems like GCC's incorrect info that's causing GDB to confuse.

My patch only touches the file `0` DWARFv5 case, CLANG is emitting DWARFv5 compliant line & macro tables
you may choose to have a look on the diff of line & macro tables for above case using GCC & CLANG.

Thanks,
Sourabh.

-----Original Message-----
From: Keith Seitz <keiths@redhat.com> 
Sent: Friday, May 14, 2021 8:26 PM
To: Tomar, Sourabh Singh <SourabhSingh.Tomar@amd.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[CAUTION: External Email]

Coincidentally, I happened to be looking at gdb/27306 about a similar problem, and I was curious to see how our patches interacted.

On 5/12/21 10:16 AM, Sourabh Singh Tomar via Gdb-patches wrote:

> As one may notice that dir index `0` will always contain absoulte 
> path(Not including the file name as), this causes GDB to append the 
> path + filename to construt macro info `main_file->filename`. This 
> leads to overriding of the macro filename which will cause macro info 
> lookup failure due to mismatch in `main_file->filename` and 
> `sal.symtab->filename`.

gdb/27306 highlights a similar problem with gcc using -gdwarf-5:

cat -n undef-macro.c
     1  #define FOO 1
     2
     3  int main(int argc, char **argv)
     4  {
     5    return FOO;
     6  }
$ gcc -gdwarf-5 undef-macro.c -o undef-macro $ ./gdb -nx -q undef-macro -ex start -ex "p FOO"
(gdb) start
Temporary breakpoint 1 at 0x401111: file foo.c, line 5.
Starting program: undef-macro

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdd48) at undef-macro.c:5
5         return FOO;
No symbol "FOO" in current context.

The problem is that file name returned by file_file_name is /not/ relative to the compilation directory. Ever.

From file_file_name:

      const file_entry *fe = file_name_at (file);

      if (!IS_ABSOLUTE_PATH (fe->name))
        {
          const char *dir = fe->include_dir (this);
          /* The file at index 0 (valid starting with DWARF 5) is already
              relative to the compilation directory.  */

          if (file != 0)
            return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
                                                          fe->name,
                                                          (char *) NULL));

Even with your patch, gdb will still return an absolute path for the above test.

As you've already mentioned, macro_start_file and macro_lookup_inclusion require that macro_source_file.filename be relative to the compliation directory. Furthermore, the documentation for macro_source_file.filename is stricter:

  /* A source file --- possibly a header file.  This filename is relative to
     the compilation directory (table->comp_dir), it exactly matches the
     symtab->filename content.  */
  const char *filename;

So if symtab->filename and macro_source_filename are required to be the same, why are we bothering with prepending directories? We already have access to the symtab in `fe'.

This is the version I was playing with...

diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c index 7575297f966..117c5a42cc5 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
     {
       const file_entry *fe = file_name_at (file);

-      if (!IS_ABSOLUTE_PATH (fe->name))
-       {
-         const char *dir = fe->include_dir (this);
-         if (dir != NULL)
-           return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
-                                                         fe->name,
-                                                         (char *) NULL));
-       }
-      return make_unique_xstrdup (fe->name);
+      /* macro_source_file requires: "This filename is relative to the
+        compilation directory, it exactly matches the symtab->filename
+        content."  */
+      return make_unique_xstrdup (fe->symtab->filename);
     }
   else
     {

By happy coincidence, this has the same effect of your patch, but it also fixes gdb/27306.

I checked the origins of file_file_name and it appears that this function was split out from file_full_name, but this directory prepending bit was never removed to fulfill the "relative" requirement of the stated documentation.
[This is from commit 233d95b548ec948c4a6d01cd05c307385dd615fb.]

I hate to post counter-patches, but in this case, I think we are really both attempting to solve the same design problem with line_header::file_file_name.

> This problem does not pops up with GCC compiled binaries, since GCC 
> does not emits DWARFv5 compliant line tables.

This test (as written) does not pass using gcc:

$ make check TESTS=gdb.base/dwarf5-macro.exp [snip] Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/dwarf5-macro.exp ...
FAIL: gdb.base/dwarf5-macro.exp: info macro HELLO_GDB

                === gdb Summary ===

# of unexpected failures        1

However, if I force -gdwarf-5 for gcc, too:

$ make check TESTS=gdb.base/dwarf5-macro.exp RUNTESTFLAGS="unix/-g3/-gdwarf-5"
[snip]
Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/dwarf5-macro.exp ...

                === gdb Summary ===

# of expected passes            1

I suggest that be corrected -- just force gcc to emit dwarf-5 debuginfo as well. IMO, the correct, compiler-agnostic way of dealing with this is via the DWARF assembler, but I am not going to suggest you do that.

I'd be happy if the test simply didn't FAIL on non-clang targets.

Keith

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Simon Marchi]

> One thing I noticed that changed is that GCC is now emitting `dir0` entry in line table as per DWARF5. However this still seems
> Incomplete to me:
> $llvm-dwarfdump --debug-line a.out
> ```
> include_directories[  1] = "/usr/include" // index still starting from `1` even at "-gdwarf-5"
> file_names[  1]:
>            name: "macro.c"
>       dir_index: 0               // -> dir 0 mentioned but dir 0 is not present in list of directories, i.e index still starting from 1
>        mod_time: 0x00000000
>          length: 0x00000000
> file_names[  2]:
>            name: "stdc-predef.h"
>       dir_index: 1
>        mod_time: 0x00000000
>          length: 0x00000000
> ```

It's not clear how you produced this output (which compiler and which
command).  What's the version of that section?  Because it looks like a
DWARF 4 or before section.  GCC 10 produces a DWARF 3 .debug_line
section, even when using -gdwarf-5.

In DWARF 4 and before, a dir_index of 0 referred to the compilation
directory, even if there wasn't an explicit dir[0] value.  See section
6.2.4, bullet 12, of DWARF4.pdf.  So you would look up the DW_AT_comp
attribute of the compilation unit, append "macro.c", and that would give
you the location of that file.

On my side, I see this with gcc 11 (from the Ubuntu 21.04 package):

    root@bc7daa9a3ffc:/# gcc-11 --version
    gcc-11 (Ubuntu 11.1.0-1ubuntu1~21.04) 11.1.0
    root@bc7daa9a3ffc:/# cat test.c
    #include <stdio.h>

    #define FOO

    int main() {
    }
    root@bc7daa9a3ffc:/# gcc-11 -gdwarf-5 -g3 test.c
    root@bc7daa9a3ffc:/# llvm-dwarfdump-12 --debug-line a.out
    ...
    include_directories[  0] = "/usr/include"
    include_directories[  1] = "/usr/include"
    include_directories[  2] = "/usr/include/x86_64-linux-gnu/bits"
    include_directories[  3] = "/usr/include/x86_64-linux-gnu/sys"
    include_directories[  4] = "/usr/include/x86_64-linux-gnu/gnu"
    include_directories[  5] = "/usr/lib/gcc/x86_64-linux-gnu/11/include"
    include_directories[  6] = "/usr/include/x86_64-linux-gnu/bits/types"
    file_names[  0]:
	       name: "test.c"
	  dir_index: 0
    file_names[  1]:
	       name: "test.c"
	  dir_index: 0
    file_names[  2]:
	       name: "stdc-predef.h"
	  dir_index: 1
    file_names[  3]:
	       name: "stdio.h"
	  dir_index: 1

So from what I see, GCC 11 generates a dir[0] entry and a file[0] entry.
However, the dir[0] value does not look good.  It makes it appear
as if my source file was at /usr/include/test.c, which it wasn't (it was
at /test.c).

Simon

RE: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Tomar, Sourabh Singh]

[AMD Public Use]

Hi Simon,

>It's not clear how you produced this output (which compiler and which command).  What's the version of that section?  Because it looks like a DWARF 4 or before section.  GCC 10 produces a DWARF 3 >.debug_line section, even when using -gdwarf-5.
>In DWARF 4 and before, a dir_index of 0 referred to the compilation directory, even if there wasn't an explicit dir[0] value.  See section 6.2.4, bullet 12, of DWARF4.pdf.  So you would look up the >DW_AT_comp attribute of the compilation unit, append "macro.c", and that would give you the location of that file.

Apologies for sharing sparse details of the tools/commands.
Details: Distribution : 18.04.1-Ubuntu
GCC: rebased and **built today** i.e  gcc (GCC) 12.0.0 20210514 (experimental)
Build config: 
./configure --disable-bootstrap --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu CFLAGS=-O3 CXXFLAGS=-O3 --enable-languages=c,c++,fortran --prefix=/home/sourabh/install/release/trunk-gcc/
Terminal:
```
>llvm-dev>>>sourabh@llvm-slrz2:~/work/dwarf/c_c++$ tot-gcc --version
gcc (GCC) 12.0.0 20210514 (experimental)

>llvm-dev>>>sourabh@llvm-slrz2:~/work/dwarf/c_c++$ gdb --version
GNU gdb (GDB) 11.0.50.20210512-git

>llvm-dev>>>sourabh@llvm:~/work/dwarf/c_c++$ cat macro.c
#define HELLO_GDB 1
int main() {}
>llvm-dev>>>sourabh@llvm:~/work/dwarf/c_c++$ tot-gcc -gdwarf-5 -g3 macro.c -o a.out.gcc
>llvm-dev>>>sourabh@llvm:~/work/dwarf/c_c++$ gdb a.out.gcc -q
Reading symbols from a.out.gcc...
(gdb) start
Temporary breakpoint 1 at 0x40048f: file macro.c, line 2.
Starting program: /home/sourabh/work/dwarf/c_c++/a.out

Temporary breakpoint 1, main () at macro.c:2
2       int main() {}
(gdb) info macros
>>> main_file->filename ->                     // printf
sal.symtab->filename -> macro.c          // printf
(gdb)

>llvm-dev>>>sourabh@llvm-slrz2:~/work/dwarf/c_c++$ llvm-dwarfdump --debug-line a.out.gcc
a.out.gcc:      file format elf64-x86-64

.debug_line contents:
debug_line[0x00000000]
Line table prologue:
    total_length: 0x00000058
          format: DWARF32
         **version: 2                           //** Version is 2, even at -gdwarf-5 -gstrict-dwarf, is this Okay** ?
 prologue_length: 0x0000003c
 min_inst_length: 1
 default_is_stmt: 1
       line_base: -5
      line_range: 14
     opcode_base: 13
include_directories[  1] = "/usr/include"
file_names[  1]:
           name: "macro.c"
      dir_index: 0
       mod_time: 0x00000000
         length: 0x00000000
file_names[  2]:
           name: "stdc-predef.h"
      dir_index: 1
       mod_time: 0x00000000
         length: 0x00000000
```

Please let me know if above info suffices the details necessary ?

>On my side, I see this with gcc 11 (from the Ubuntu 21.04 package):
>    root@bc7daa9a3ffc:/# gcc-11 --version
>    gcc-11 (Ubuntu 11.1.0-1ubuntu1~21.04) 11.1.0
>    root@bc7daa9a3ffc:/# cat test.c
>    #include <stdio.h>
>    #define FOO
>    int main() {
>    }
>    root@bc7daa9a3ffc:/# gcc-11 -gdwarf-5 -g3 test.c
>    root@bc7daa9a3ffc:/# llvm-dwarfdump-12 --debug-line a.out
    ...
>    include_directories[  0] = "/usr/include"
  ..
>    file_names[  0]:
>               name: "test.c"
>          dir_index: 0
>    file_names[  1]:
>               name: "test.c"
>          dir_index: 0
>    file_names[  2]:
>               name: "stdc-predef.h"
>          dir_index: 1
>    file_names[  3]:
>               name: "stdio.h"
>          dir_index: 1
>So from what I see, GCC 11 generates a dir[0] entry and a file[0] entry.
>However, the dir[0] value does not look good.  It makes it appear as if my source file was at /usr/include/test.c, which it wasn't (it was at /test.c).

Thanks for sharing this, since both file0 and dir0 both are present, if possible, could you please share any behavior change or anything subtle you notice before/after
applying patch ?

Thanks,
Sourabh

-----Original Message-----
From: Simon Marchi <simon.marchi@polymtl.ca> 
Sent: Saturday, May 15, 2021 12:20 AM
To: Tomar, Sourabh Singh <SourabhSingh.Tomar@amd.com>; Keith Seitz <keiths@redhat.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[CAUTION: External Email]

> One thing I noticed that changed is that GCC is now emitting `dir0` 
> entry in line table as per DWARF5. However this still seems Incomplete to me:
> $llvm-dwarfdump --debug-line a.out
> ```
> include_directories[  1] = "/usr/include" // index still starting from `1` even at "-gdwarf-5"
> file_names[  1]:
>            name: "macro.c"
>       dir_index: 0               // -> dir 0 mentioned but dir 0 is not present in list of directories, i.e index still starting from 1
>        mod_time: 0x00000000
>          length: 0x00000000
> file_names[  2]:
>            name: "stdc-predef.h"
>       dir_index: 1
>        mod_time: 0x00000000
>          length: 0x00000000
> ```

It's not clear how you produced this output (which compiler and which command).  What's the version of that section?  Because it looks like a DWARF 4 or before section.  GCC 10 produces a DWARF 3 .debug_line section, even when using -gdwarf-5.

In DWARF 4 and before, a dir_index of 0 referred to the compilation directory, even if there wasn't an explicit dir[0] value.  See section 6.2.4, bullet 12, of DWARF4.pdf.  So you would look up the DW_AT_comp attribute of the compilation unit, append "macro.c", and that would give you the location of that file.

On my side, I see this with gcc 11 (from the Ubuntu 21.04 package):

    root@bc7daa9a3ffc:/# gcc-11 --version
    gcc-11 (Ubuntu 11.1.0-1ubuntu1~21.04) 11.1.0
    root@bc7daa9a3ffc:/# cat test.c
    #include <stdio.h>

    #define FOO

    int main() {
    }
    root@bc7daa9a3ffc:/# gcc-11 -gdwarf-5 -g3 test.c
    root@bc7daa9a3ffc:/# llvm-dwarfdump-12 --debug-line a.out
    ...
    include_directories[  0] = "/usr/include"
    include_directories[  1] = "/usr/include"
    include_directories[  2] = "/usr/include/x86_64-linux-gnu/bits"
    include_directories[  3] = "/usr/include/x86_64-linux-gnu/sys"
    include_directories[  4] = "/usr/include/x86_64-linux-gnu/gnu"
    include_directories[  5] = "/usr/lib/gcc/x86_64-linux-gnu/11/include"
    include_directories[  6] = "/usr/include/x86_64-linux-gnu/bits/types"
    file_names[  0]:
               name: "test.c"
          dir_index: 0
    file_names[  1]:
               name: "test.c"
          dir_index: 0
    file_names[  2]:
               name: "stdc-predef.h"
          dir_index: 1
    file_names[  3]:
               name: "stdio.h"
          dir_index: 1

So from what I see, GCC 11 generates a dir[0] entry and a file[0] entry.
However, the dir[0] value does not look good.  It makes it appear as if my source file was at /usr/include/test.c, which it wasn't (it was at /test.c).

Simon

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Simon Marchi]

> Thanks for sharing this, since both file0 and dir0 both are present, if possible, could you please share any behavior change or anything subtle you notice before/after
> applying patch ?

That gcc is in a container, so not to practical to test with.  So I
built various gcc versions from source to investigate this:

 - 10.2.0 (I did not actually build this one, it's the one from my
   distro)
 - 11.1.0
 - 11 from today's git
 - master from today's git

With gcc 10.2.0, as seen before, I don't get a DWARF 5 .debug_line
section.

With all three others, I get the same thing (modulo dir[2] which
specific to the compiler version) that looks good:

    include_directories[  0] = "/home/simark/build/binutils-gdb/gdb"
    include_directories[  1] = "/usr/include"
    include_directories[  2] = "/opt/gcc/git/lib/gcc/x86_64-pc-linux-gnu/12.0.0/include"
    include_directories[  3] = "/usr/include/bits"
    include_directories[  4] = "/usr/include/sys"
    include_directories[  5] = "/usr/include/gnu"
    file_names[  0]:
               name: "test.c"
          dir_index: 0
    file_names[  1]:
               name: "test.c"
          dir_index: 0
    file_names[  2]:
               name: "stdc-predef.h"
          dir_index: 1
    file_names[  3]:
               name: "stdint.h"
          dir_index: 2
    file_names[  4]:
               name: "stdint.h"
          dir_index: 1

I tested again using the gcc 11.1.0 from Ubuntu, that gave me a wrong
dir[0] before.  I replicated the same directory structure
(/home/simark/build/binutils-gdb/gdb/...) in my Ubuntu container, and
things look good actually:

    include_directories[  0] = "/home/simark/build/binutils-gdb/gdb"
    include_directories[  1] = "/usr/include"
    include_directories[  2] = "/usr/lib/gcc/x86_64-linux-gnu/11/include"
    include_directories[  3] = "/usr/include/x86_64-linux-gnu/bits"
    include_directories[  4] = "/usr/include/x86_64-linux-gnu/sys"
    include_directories[  5] = "/usr/include/x86_64-linux-gnu/gnu"
    file_names[  0]:
               name: "test.c"
          dir_index: 0
    file_names[  1]:
               name: "test.c"
          dir_index: 0
    file_names[  2]:
               name: "stdc-predef.h"
          dir_index: 1

So I tried compiling a file in the root directory (/) on my
(non-container) host, and I did see the weird output:

    include_directories[  0] = "/usr/include"
    include_directories[  1] = "/usr/include"
    include_directories[  2] = "/usr/include/bits"
    include_directories[  3] = "/usr/include/sys"
    include_directories[  4] = "/usr/include/gnu"
    include_directories[  5] = "/opt/gcc/11.1.0/lib/gcc/x86_64-pc-linux-gnu/11.1.0/include"
    include_directories[  6] = "/usr/include/bits/types"
    file_names[  0]:
               name: "test.c"
          dir_index: 0
    file_names[  1]:
               name: "test.c"
          dir_index: 0
    file_names[  2]:
               name: "stdc-predef.h"
          dir_index: 1

So that looks like a gcc bug that only happens when the compilation
directory is the root.  I'll go see if there's a gcc bug for this
already and file one if there isn't.

But other than that little corner case, the gcc 11+ -gdwarf-5 output
looks good.

I did a quick test loading the binary made using gcc master in GDB, and
I wasn't able to print the macro - both without and with your patch.  I
didn't investigate further, I have to go now.

Simon

RE: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Tomar, Sourabh Singh]

[AMD Public Use]

Hello Keith,

Could you please share your plan WRT to this patch.
Do you want to take it forward ? or you want me to take this forward.

Thanks,
Sourabh

-----Original Message-----
From: Keith Seitz <keiths@redhat.com> 
Sent: Friday, May 14, 2021 8:26 PM
To: Tomar, Sourabh Singh <SourabhSingh.Tomar@amd.com>; gdb-patches@sourceware.org
Subject: Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[CAUTION: External Email]

Coincidentally, I happened to be looking at gdb/27306 about a similar problem, and I was curious to see how our patches interacted.

On 5/12/21 10:16 AM, Sourabh Singh Tomar via Gdb-patches wrote:

> As one may notice that dir index `0` will always contain absoulte 
> path(Not including the file name as), this causes GDB to append the 
> path + filename to construt macro info `main_file->filename`. This 
> leads to overriding of the macro filename which will cause macro info 
> lookup failure due to mismatch in `main_file->filename` and 
> `sal.symtab->filename`.

gdb/27306 highlights a similar problem with gcc using -gdwarf-5:

cat -n undef-macro.c
     1  #define FOO 1
     2
     3  int main(int argc, char **argv)
     4  {
     5    return FOO;
     6  }
$ gcc -gdwarf-5 undef-macro.c -o undef-macro $ ./gdb -nx -q undef-macro -ex start -ex "p FOO"
(gdb) start
Temporary breakpoint 1 at 0x401111: file foo.c, line 5.
Starting program: undef-macro

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdd48) at undef-macro.c:5
5         return FOO;
No symbol "FOO" in current context.

The problem is that file name returned by file_file_name is /not/ relative to the compilation directory. Ever.

From file_file_name:

      const file_entry *fe = file_name_at (file);

      if (!IS_ABSOLUTE_PATH (fe->name))
        {
          const char *dir = fe->include_dir (this);
          /* The file at index 0 (valid starting with DWARF 5) is already
              relative to the compilation directory.  */

          if (file != 0)
            return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
                                                          fe->name,
                                                          (char *) NULL));

Even with your patch, gdb will still return an absolute path for the above test.

As you've already mentioned, macro_start_file and macro_lookup_inclusion require that macro_source_file.filename be relative to the compliation directory. Furthermore, the documentation for macro_source_file.filename is stricter:

  /* A source file --- possibly a header file.  This filename is relative to
     the compilation directory (table->comp_dir), it exactly matches the
     symtab->filename content.  */
  const char *filename;

So if symtab->filename and macro_source_filename are required to be the same, why are we bothering with prepending directories? We already have access to the symtab in `fe'.

This is the version I was playing with...

diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c index 7575297f966..117c5a42cc5 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
     {
       const file_entry *fe = file_name_at (file);

-      if (!IS_ABSOLUTE_PATH (fe->name))
-       {
-         const char *dir = fe->include_dir (this);
-         if (dir != NULL)
-           return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
-                                                         fe->name,
-                                                         (char *) NULL));
-       }
-      return make_unique_xstrdup (fe->name);
+      /* macro_source_file requires: "This filename is relative to the
+        compilation directory, it exactly matches the symtab->filename
+        content."  */
+      return make_unique_xstrdup (fe->symtab->filename);
     }
   else
     {

By happy coincidence, this has the same effect of your patch, but it also fixes gdb/27306.

I checked the origins of file_file_name and it appears that this function was split out from file_full_name, but this directory prepending bit was never removed to fulfill the "relative" requirement of the stated documentation.
[This is from commit 233d95b548ec948c4a6d01cd05c307385dd615fb.]

I hate to post counter-patches, but in this case, I think we are really both attempting to solve the same design problem with line_header::file_file_name.

> This problem does not pops up with GCC compiled binaries, since GCC 
> does not emits DWARFv5 compliant line tables.

This test (as written) does not pass using gcc:

$ make check TESTS=gdb.base/dwarf5-macro.exp [snip] Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/dwarf5-macro.exp ...
FAIL: gdb.base/dwarf5-macro.exp: info macro HELLO_GDB

                === gdb Summary ===

# of unexpected failures        1

However, if I force -gdwarf-5 for gcc, too:

$ make check TESTS=gdb.base/dwarf5-macro.exp RUNTESTFLAGS="unix/-g3/-gdwarf-5"
[snip]
Running /home/keiths/work/gdb/branches/virgin/linux/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/dwarf5-macro.exp ...

                === gdb Summary ===

# of expected passes            1

I suggest that be corrected -- just force gcc to emit dwarf-5 debuginfo as well. IMO, the correct, compiler-agnostic way of dealing with this is via the DWARF assembler, but I am not going to suggest you do that.

I'd be happy if the test simply didn't FAIL on non-clang targets.

Keith

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Keith Seitz]

On 5/24/21 4:36 AM, Tomar, Sourabh Singh wrote:
> [AMD Public Use]
> 
> Hello Keith,
> 
> Could you please share your plan WRT to this patch.
> Do you want to take it forward ? or you want me to take this forward.
> 

I can pursue this...

In that vein, does anyone (maintainers?) have an input on my "counterpatch"
(reposted below) that removes this IS_ABSOLUTE_PREFIX stuff and copies the
symtab's filename?

I haven't officially submitted this as a patch because I'm curious whether
my reading of this is correct/complete. Maybe the documentation/comments
are incorrect or no longer valid? 

FWIW, I've tested that patch on native x86_64 Fedora 34 with no regressions.

Keith

Patch under discussion:

gdb/ChangeLog

	* dwarf2/line-header.c (line_header::file_file_name): Copy
	the symtab's filename.

diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 7575297f966..117c5a42cc5 100644
--- a/gdb/dwarf2/line-header.c
+++ b/gdb/dwarf2/line-header.c
@@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
     {
       const file_entry *fe = file_name_at (file);
 
-      if (!IS_ABSOLUTE_PATH (fe->name))
-	{
-	  const char *dir = fe->include_dir (this);
-	  if (dir != NULL)
-	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
-							  fe->name,
-							  (char *) NULL));
-	}
-      return make_unique_xstrdup (fe->name);
+      /* macro_source_file requires: "This filename is relative to the
+	 compilation directory, it exactly matches the symtab->filename
+	 content."  */
+      return make_unique_xstrdup (fe->symtab->filename);
     }
   else
     {

Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table

[Keith Seitz]

RFC ping

Anyone at all have an opinion on this direction?

Keith

On 5/24/21 11:47 AM, Keith Seitz via Gdb-patches wrote:
> On 5/24/21 4:36 AM, Tomar, Sourabh Singh wrote:
>> [AMD Public Use]
>>
>> Hello Keith,
>>
>> Could you please share your plan WRT to this patch.
>> Do you want to take it forward ? or you want me to take this forward.
>>
> 
> I can pursue this...
> 
> In that vein, does anyone (maintainers?) have an input on my "counterpatch"
> (reposted below) that removes this IS_ABSOLUTE_PREFIX stuff and copies the
> symtab's filename?
> 
> I haven't officially submitted this as a patch because I'm curious whether
> my reading of this is correct/complete. Maybe the documentation/comments
> are incorrect or no longer valid? 
> 
> FWIW, I've tested that patch on native x86_64 Fedora 34 with no regressions.
> 
> Keith
> 
> Patch under discussion:
> 
> gdb/ChangeLog
> 
> 	* dwarf2/line-header.c (line_header::file_file_name): Copy
> 	the symtab's filename.
> 
> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
> index 7575297f966..117c5a42cc5 100644
> --- a/gdb/dwarf2/line-header.c
> +++ b/gdb/dwarf2/line-header.c
> @@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
>      {
>        const file_entry *fe = file_name_at (file);
>  
> -      if (!IS_ABSOLUTE_PATH (fe->name))
> -	{
> -	  const char *dir = fe->include_dir (this);
> -	  if (dir != NULL)
> -	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
> -							  fe->name,
> -							  (char *) NULL));
> -	}
> -      return make_unique_xstrdup (fe->name);
> +      /* macro_source_file requires: "This filename is relative to the
> +	 compilation directory, it exactly matches the symtab->filename
> +	 content."  */
> +      return make_unique_xstrdup (fe->symtab->filename);
>      }
>    else
>      {
> 

[RFC] PING [Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table]

[Keith Seitz]

I think this might be something we should consider including in gdb-11.

Keith

On 6/8/21 11:48 AM, Keith Seitz via Gdb-patches wrote:
> RFC ping
> 
> Anyone at all have an opinion on this direction?
> 
> Keith
> 
> On 5/24/21 11:47 AM, Keith Seitz via Gdb-patches wrote:
>> On 5/24/21 4:36 AM, Tomar, Sourabh Singh wrote:
>>> [AMD Public Use]
>>>
>>> Hello Keith,
>>>
>>> Could you please share your plan WRT to this patch.
>>> Do you want to take it forward ? or you want me to take this forward.
>>>
>>
>> I can pursue this...
>>
>> In that vein, does anyone (maintainers?) have an input on my "counterpatch"
>> (reposted below) that removes this IS_ABSOLUTE_PREFIX stuff and copies the
>> symtab's filename?
>>
>> I haven't officially submitted this as a patch because I'm curious whether
>> my reading of this is correct/complete. Maybe the documentation/comments
>> are incorrect or no longer valid? 
>>
>> FWIW, I've tested that patch on native x86_64 Fedora 34 with no regressions.
>>
>> Keith
>>
>> Patch under discussion:
>>
>> gdb/ChangeLog
>>
>> 	* dwarf2/line-header.c (line_header::file_file_name): Copy
>> 	the symtab's filename.
>>
>> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
>> index 7575297f966..117c5a42cc5 100644
>> --- a/gdb/dwarf2/line-header.c
>> +++ b/gdb/dwarf2/line-header.c
>> @@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
>>      {
>>        const file_entry *fe = file_name_at (file);
>>  
>> -      if (!IS_ABSOLUTE_PATH (fe->name))
>> -	{
>> -	  const char *dir = fe->include_dir (this);
>> -	  if (dir != NULL)
>> -	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
>> -							  fe->name,
>> -							  (char *) NULL));
>> -	}
>> -      return make_unique_xstrdup (fe->name);
>> +      /* macro_source_file requires: "This filename is relative to the
>> +	 compilation directory, it exactly matches the symtab->filename
>> +	 content."  */
>> +      return make_unique_xstrdup (fe->symtab->filename);
>>      }
>>    else
>>      {
>>
> 

Re: [RFC] PING [Re: [PATCH] Fix macro info lookup for binaries containing DWARFv5 line table]

[Simon Marchi]

[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]

On 2021-06-22 1:01 p.m., Keith Seitz via Gdb-patches wrote:
> I think this might be something we should consider including in gdb-11.
> 
> Keith
> 
> On 6/8/21 11:48 AM, Keith Seitz via Gdb-patches wrote:
>> RFC ping
>>
>> Anyone at all have an opinion on this direction?
>>
>> Keith
>>
>> On 5/24/21 11:47 AM, Keith Seitz via Gdb-patches wrote:
>>> On 5/24/21 4:36 AM, Tomar, Sourabh Singh wrote:
>>>> [AMD Public Use]
>>>>
>>>> Hello Keith,
>>>>
>>>> Could you please share your plan WRT to this patch.
>>>> Do you want to take it forward ? or you want me to take this forward.
>>>>
>>>
>>> I can pursue this...
>>>
>>> In that vein, does anyone (maintainers?) have an input on my "counterpatch"
>>> (reposted below) that removes this IS_ABSOLUTE_PREFIX stuff and copies the
>>> symtab's filename?
>>>
>>> I haven't officially submitted this as a patch because I'm curious whether
>>> my reading of this is correct/complete. Maybe the documentation/comments
>>> are incorrect or no longer valid? 
>>>
>>> FWIW, I've tested that patch on native x86_64 Fedora 34 with no regressions.

Hi Keith,

It's a bit hard to tell whether this will always work, because different
compilers do things slightly differently.  Factor in different version,
DWARF4 vs DWARF5, that's a lot of combinations.  I wrote a little script
to test as many combinations as possible, and with your patch it all
looks good, in the sense that I can print the macros in all cases.  I'll
attach it to this message if you want to play with it.

The axis are:

 - the compiler
 - DWARF5 vs DWARF4
 - how the source file is specified on the command line

How the source file is specified on the command line can be:

 - foo.c
 - ../foo.c
 - dir/foo.c (while PWD is the parent directory)
 - /absolute/path/to/dir/foo.c (while PWD is `dir`)
 - /absolute/path/to/dir/foo.c (while PWD is the parent directory)

They all produce slightly different debug info that could lead to
something going wrong.

The important thing is that it's well tested, so ensure that as many of
these ways to specify a file on the command line are tested.  Test with
one macro in the main source file and one macro in an included file.  I
don't know if we can have a DWARF4 vs DWARF5 axis in the test, but if so
it would be nice.  And add any another possible variation you can think
of.

One more comment below...

>>>
>>> Keith
>>>
>>> Patch under discussion:
>>>
>>> gdb/ChangeLog
>>>
>>> 	* dwarf2/line-header.c (line_header::file_file_name): Copy
>>> 	the symtab's filename.
>>>
>>> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
>>> index 7575297f966..117c5a42cc5 100644
>>> --- a/gdb/dwarf2/line-header.c
>>> +++ b/gdb/dwarf2/line-header.c
>>> @@ -69,15 +69,10 @@ line_header::file_file_name (int file) const
>>>      {
>>>        const file_entry *fe = file_name_at (file);
>>>  
>>> -      if (!IS_ABSOLUTE_PATH (fe->name))
>>> -	{
>>> -	  const char *dir = fe->include_dir (this);
>>> -	  if (dir != NULL)
>>> -	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
>>> -							  fe->name,
>>> -							  (char *) NULL));
>>> -	}
>>> -      return make_unique_xstrdup (fe->name);
>>> +      /* macro_source_file requires: "This filename is relative to the
>>> +	 compilation directory, it exactly matches the symtab->filename
>>> +	 content."  */
>>> +      return make_unique_xstrdup (fe->symtab->filename);

If I understand correctly, the contract of file_file_name is that it
should never return an absolute path.  Therefore, should we be able to
stick a

    gdb_assert (!IS_ABSOLUTE_PATH (fe->symtab->filename))

in there?  If I do, it catches at least one case where it returns an
absolute path, in test-gcc-9-dwarf-4-std.

    (top-gdb) p fe.symtab.filename
    $1 = 0x62100003e960 "/home/smarchi/build/binutils-gdb/macro-test/test.c"

Printing the macros still work, but I wonder if that's expected or not.

Simon

[-- Attachment #2: macro-test.tgz --]
[-- Type: application/x-compressed-tar, Size: 661 bytes --]