[PATCH] gdb-Fix-macro-info-lookup-for-binaries-containing-DWARFv5

[PATCH] gdb-Fix-macro-info-lookup-for-binaries-containing-DWARFv5

[Tomar, Sourabh Singh]

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

[AMD Public Use]

Hello Everyone,

Requesting review on this patch, Please have a look.

Problem Description/Summary:
- clang emits DWARFv5 conformant line tables(ie. .debug_line section) which makes 2 things explicitly:
1. The current directory is explicitly present in the directories field and has index 0.
2. The current compilation filename is explicitly present and has index 0.

This leads to a subtle issue in GDB while accessing macro information:
Consider the following program:
```
#define OUT 4
int main() { int a = OUT;}
```
Compiled as `clang -gdwarf-5 -fdebug-macro test.c`
GDB session:
```
$gdb a.out -q
....
2       int main() { int a = OUT;}
(gdb) info macro OUT
The symbol `OUT' has no definition as a C/C++ preprocessor macro
at /home/test.c:-1
```

This issue cannot be reproduced using GCC compiler as GCC doesnot emit DWARFv5 compliant tables even
At `-gdwarf-5` flag.

Issue is stemming from line-header.c. When performing a file name lookup for macro filename GDB 
mistakenly appends FULL path after checking following condition, which as **one may notice** for 
directory index `0` will always evaluates `true` for DWARv5 line tables and `false` for DWARFv4 line tables.
```
If (dir != NULL)
return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
                                                          fe->name,
                                                          (char *) NULL));
```
Result of this, macro `filename` getting populated with fullname. Eventually macro lookup
results in failure due to mismatch in `main_file->filename` and   `sal.symtab->filename`.

This patch attempts to fix above problem by extending the check for file `0`, i.e for file `0`
do not append the fullname.

Testing:
- NO regressions in GDB testsuite before and after the fix.

We also tried add a test case for this fix, but was unsuccessful.
GDB test suite uses full path to the test files and this specific problem is 
not reproducible when user specifies full path. i.e
Not reproducible cases:
1. `clang -gdwarf-5 -fdebug-macro ./test.c` // macro visible inside GDB
2. `clang -gdwarf-5 -fdebug-macro /ABSOLUTE_PATH/test.c` // macro visible inside GDB

Reproducible:
1. `clang -gdwarf-5 -fdebug-macro test.c` //log shown above.

gdb/ChangeLog
2021-03-26  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.

Thank You,
Sourabh.

[-- Attachment #2: gdb-Fix-macro-info-lookup-for-binaries-containing-DWARFv.patch --]
[-- Type: application/octet-stream, Size: 4578 bytes --]

From c80a3e56e9f943844478fb34351a8c3decba413e Mon Sep 17 00:00:00 2001
From: Sourabh Singh Tomar <SourabhSingh.Tomar@amd.com>
Date: Wed, 24 Mar 2021 16:23:33 +0530
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.

2021-03-26  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/ChangeLog            |  6 ++++++
 gdb/dwarf2/line-header.c | 30 +++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 173b08bf04b..1580690a2d7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2021-03-26  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.
+
 2021-03-25  Pedro Alves  <pedro@palves.net>
 
 	* gdb.server/bkpt-other-inferior.exp: Only enable remote output
diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
index 7575297f966..dad44550531 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,29 @@ line_header::file_file_name (int file) const
       if (!IS_ABSOLUTE_PATH (fe->name))
 	{
 	  const char *dir = fe->include_dir (this);
-	  if (dir != NULL)
+     /* clang  compiler emits DWARFv5 compliant line tables i.e
+	 For any C file compiled as:
+	clang -gdwarf-5 -fdebug-macro test.c
+
+	```
+	line table:
+	include_directories[  0] = "/home/"
+	file_names[  0]:
+	name: = "test.c"
+	dir_index: 0
+
+	macro table:
+	Referring to file index 0.
+	```
+	Now the 0 dir index will never be NULL for DWARFv5.
+	Do Not append the path to file name since this will override
+	the macro filename which will cause macro info lookup failure
+	due to mismatch in `main_file->filename` and
+	`sal.symtab->filename`.
+	Note that previously this check will always be `true` for
+	DWARFv5, since index 0 will never be NULL*/
+
+	  if (dir != NULL && file)
 	    return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
 							  fe->name,
 							  (char *) NULL));
@@ -100,7 +123,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);
-- 
2.17.1

RE: [PATCH] gdb-Fix-macro-info-lookup-for-binaries-containing-DWARFv5

[Tomar, Sourabh Singh]

[AMD Public Use]

Gentle PING!

-----Original Message-----
From: Tomar, Sourabh Singh 
Sent: Friday, March 26, 2021 8:43 PM
To: Tomar, Sourabh Singh via Gdb-patches <gdb-patches@sourceware.org>
Cc: George, Jini Susan <JiniSusan.George@amd.com>; Sharma, Alok Kumar <AlokKumar.Sharma@amd.com>; Kumar N, Bhuvanendra <Bhuvanendra.KumarN@amd.com>; Achra, Nitika <Nitika.Achra@amd.com>; E, Nagajyothi <Nagajyothi.E@amd.com>
Subject: [PATCH] gdb-Fix-macro-info-lookup-for-binaries-containing-DWARFv5

[AMD Public Use]

Hello Everyone,

Requesting review on this patch, Please have a look.

Problem Description/Summary:
- clang emits DWARFv5 conformant line tables(ie. .debug_line section) which makes 2 things explicitly:
1. The current directory is explicitly present in the directories field and has index 0.
2. The current compilation filename is explicitly present and has index 0.

This leads to a subtle issue in GDB while accessing macro information:
Consider the following program:
```
#define OUT 4
int main() { int a = OUT;}
```
Compiled as `clang -gdwarf-5 -fdebug-macro test.c` GDB session:
```
$gdb a.out -q
....
2       int main() { int a = OUT;}
(gdb) info macro OUT
The symbol `OUT' has no definition as a C/C++ preprocessor macro at /home/test.c:-1 ```

This issue cannot be reproduced using GCC compiler as GCC doesnot emit DWARFv5 compliant tables even At `-gdwarf-5` flag.

Issue is stemming from line-header.c. When performing a file name lookup for macro filename GDB mistakenly appends FULL path after checking following condition, which as **one may notice** for directory index `0` will always evaluates `true` for DWARv5 line tables and `false` for DWARFv4 line tables.
```
If (dir != NULL)
return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
                                                          fe->name,
                                                          (char *) NULL)); ``` Result of this, macro `filename` getting populated with fullname. Eventually macro lookup
results in failure due to mismatch in `main_file->filename` and   `sal.symtab->filename`.

This patch attempts to fix above problem by extending the check for file `0`, i.e for file `0` do not append the fullname.

Testing:
- NO regressions in GDB testsuite before and after the fix.

We also tried add a test case for this fix, but was unsuccessful.
GDB test suite uses full path to the test files and this specific problem is not reproducible when user specifies full path. i.e Not reproducible cases:
1. `clang -gdwarf-5 -fdebug-macro ./test.c` // macro visible inside GDB 2. `clang -gdwarf-5 -fdebug-macro /ABSOLUTE_PATH/test.c` // macro visible inside GDB

Reproducible:
1. `clang -gdwarf-5 -fdebug-macro test.c` //log shown above.

gdb/ChangeLog
2021-03-26  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.

Thank You,
Sourabh.

Re: [PATCH] gdb-Fix-macro-info-lookup-for-binaries-containing-DWARFv5

[Simon Marchi]

Hi Sourabh,

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

On 2021-03-26 11:12 a.m., Tomar, Sourabh Singh via Gdb-patches wrote:
> [AMD Public Use]
> 
> Hello Everyone,
> 
> Requesting review on this patch, Please have a look.
> 
> Problem Description/Summary:
> - clang emits DWARFv5 conformant line tables(ie. .debug_line section) which makes 2 things explicitly:
> 1. The current directory is explicitly present in the directories field and has index 0.
> 2. The current compilation filename is explicitly present and has index 0.
> 
> This leads to a subtle issue in GDB while accessing macro information:
> Consider the following program:
> ```
> #define OUT 4
> int main() { int a = OUT;}
> ```
> Compiled as `clang -gdwarf-5 -fdebug-macro test.c`
> GDB session:
> ```
> $gdb a.out -q
> ....
> 2       int main() { int a = OUT;}
> (gdb) info macro OUT
> The symbol `OUT' has no definition as a C/C++ preprocessor macro
> at /home/test.c:-1
> ```
> 
> This issue cannot be reproduced using GCC compiler as GCC doesnot emit DWARFv5 compliant tables even
> At `-gdwarf-5` flag.
> 
> Issue is stemming from line-header.c. When performing a file name lookup for macro filename GDB 
> mistakenly appends FULL path after checking following condition, which as **one may notice** for 
> directory index `0` will always evaluates `true` for DWARv5 line tables and `false` for DWARFv4 line tables.
> ```
> If (dir != NULL)
> return gdb::unique_xmalloc_ptr<char> (concat (dir, SLASH_STRING,
>                                                           fe->name,
>                                                           (char *) NULL));
> ```
> Result of this, macro `filename` getting populated with fullname. Eventually macro lookup
> results in failure due to mismatch in `main_file->filename` and   `sal.symtab->filename`.
> 
> This patch attempts to fix above problem by extending the check for file `0`, i.e for file `0`
> do not append the fullname.

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:

    include_directories[  0] = "/home/simark/build/binutils-gdb/gdb"
    file_names[  0]:
               name: "test.c"
          dir_index: 0
       md5_checksum: c32bbee831da0091a8adb706e6da1b5a

If I move up a directory:

    $ clang-11 gdb/test.c -o test -gdwarf-5 -fdebug-macro && llvm-dwarfdump -F -color -debug-line test


    include_directories[  0] = "/home/simark/build/binutils-gdb"
    file_names[  0]:
               name: "gdb/test.c"
          dir_index: 0
       md5_checksum: c32bbee831da0091a8adb706e6da1b5a

So it looks like by returning the filename as-is, we'll indeed return a path to
the file relative to the compilation directory.

> 
> Testing:
> - NO regressions in GDB testsuite before and after the fix.
> 
> We also tried add a test case for this fix, but was unsuccessful.
> GDB test suite uses full path to the test files and this specific problem is 
> not reproducible when user specifies full path. i.e
> Not reproducible cases:
> 1. `clang -gdwarf-5 -fdebug-macro ./test.c` // macro visible inside GDB
> 2. `clang -gdwarf-5 -fdebug-macro /ABSOLUTE_PATH/test.c` // macro visible inside GDB
> 
> Reproducible:
> 1. `clang -gdwarf-5 -fdebug-macro test.c` //log shown above.

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"

The result is:

    $ llvm-dwarfdump -F -color --debug-line testsuite/outputs/gdb.base/template/template
    include_directories[  0] = "/home/simark/build/binutils-gdb/gdb/testsuite"
    file_names[  0]:
	       name: "outputs/gdb.base/template/template.c"
	  dir_index: 0
       md5_checksum: 9c74688f43ba0d35a7a52e4717afd397

And then without your patch:

    $ ./gdb -q -nx --data-directory=data-directory ./testsuite/outputs/gdb.base/template/template -ex start -ex "p VAL" -batch
    Temporary breakpoint 1 at 0x112b: file outputs/gdb.base/template/template.c, line 23.

    Temporary breakpoint 1, main () at outputs/gdb.base/template/template.c:23
    23        int i = VAL;
    No symbol "VAL" in current context.

And with your patch:


    $ ./gdb -q -nx --data-directory=data-directory ./testsuite/outputs/gdb.base/template/template -ex start -ex "p VAL" -batch
    Temporary breakpoint 1 at 0x112b: file outputs/gdb.base/template/template.c, line 23.

    Temporary breakpoint 1, main () at outputs/gdb.base/template/template.c:23
    23        int i = VAL;
    $1 = 33

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.

In addition, the ideal would be to add a test that uses the DWARF
assembler, so that we don't rely on the specific behavior of a specific
compiler version to test what we want to test.

> gdb/ChangeLog
> 2021-03-26  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.
> 
> Thank You,
> Sourabh.
> 

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

I've copied your patch below and added some comments.

> From c80a3e56e9f943844478fb34351a8c3decba413e Mon Sep 17 00:00:00 2001
> From: Sourabh Singh Tomar <SourabhSingh.Tomar@amd.com>
> Date: Wed, 24 Mar 2021 16:23:33 +0530
> 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.
> 
> 2021-03-26  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/ChangeLog            |  6 ++++++
>  gdb/dwarf2/line-header.c | 30 +++++++++++++++++++++++++++---
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 173b08bf04b..1580690a2d7 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-03-26  Sourabh Singh Tomar  <SourabhSingh.Tomar@amd.com>
> +
> +	* dwarf2/line-header.c: Updated the comment and extended

Use present tense (e.g. Update, not Updated).

> +	check in function `line_header::file_file_name` for
> +	DWARFv5 line tables.
> +
>  2021-03-25  Pedro Alves  <pedro@palves.net>
>  
>  	* gdb.server/bkpt-other-inferior.exp: Only enable remote output
> diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c
> index 7575297f966..dad44550531 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 .  */

Watch the indentation.

>    if (is_valid_file_index (file))
>      {
>        const file_entry *fe = file_name_at (file);
> @@ -72,7 +73,29 @@ line_header::file_file_name (int file) const
>        if (!IS_ABSOLUTE_PATH (fe->name))
>  	{
>  	  const char *dir = fe->include_dir (this);
> -	  if (dir != NULL)
> +     /* clang  compiler emits DWARFv5 compliant line tables i.e
> +	 For any C file compiled as:
> +	clang -gdwarf-5 -fdebug-macro test.c
> +
> +	```
> +	line table:
> +	include_directories[  0] = "/home/"
> +	file_names[  0]:
> +	name: = "test.c"
> +	dir_index: 0
> +
> +	macro table:
> +	Referring to file index 0.
> +	```
> +	Now the 0 dir index will never be NULL for DWARFv5.
> +	Do Not append the path to file name since this will override
> +	the macro filename which will cause macro info lookup failure
> +	due to mismatch in `main_file->filename` and
> +	`sal.symtab->filename`.
> +	Note that previously this check will always be `true` for
> +	DWARFv5, since index 0 will never be NULL*/

You can shorten this comment to:

/* The file at index 0 (valid starting with DWARF 5) is already relative
   to the compilation directory.  */

> +
> +	  if (dir != NULL && file)

Use `file != 0`.

Simon