[PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom de Vries]

The test-case gdb.ada/dgopt.exp uses the -gnatD switch, in combination with
-gnatG.

This causes the source file $src/gdb/testsuite/gdb.ada/dgopt/x.adb to be
expanded into $build/gdb/testsuite/outputs/gdb.ada/dgopt/x.adb.dg, and the
debug information should refer to the x.adb.dg file.

That is the case for the .debug_line part:
...
The Directory Table is empty.

 The File Name Table (offset 0x1c):
  Entry Dir     Time    Size    Name
  1     0       0       0       x.adb.dg
...
but not for the .debug_info part:
...
    <11>   DW_AT_name        : $src/gdb/testsuite/gdb.ada/dgopt/x.adb
    <15>   DW_AT_comp_dir    : $build/gdb/testsuite/outputs/gdb.ada/dgopt
...

Filed as PR gcc/103436.

In C we can generate similar debug information, using a source file that does
not contain any code, but includes another one that does:
...
 $ cat gdb/testsuite/gdb.base/include-main.c
 #include "main.c"
...
such that in the .debug_line part we have:
...
 The Directory Table (offset 0x1c):
  1     /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base

 The File Name Table (offset 0x57):
  Entry Dir     Time    Size    Name
  1     1       0       0       main.c
...
and in the .debug_info part:
...
    <11>   DW_AT_name        : $src/gdb/testsuite/gdb.base/include-main.c
    <15>   DW_AT_comp_dir    : $build/gdb/testsuite
...

Add a C test-case that mimics gdb.ada/dgopt.exp, that is:
- generate debug info as described above,
- issue a list of a line in include-main.c, while the corresponding
  CU is not expanded yet.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.base/include-main.c   | 18 ++++++++++
 gdb/testsuite/gdb.base/include-main.exp | 47 +++++++++++++++++++++++++
 gdb/testsuite/gdb.base/main.c           | 22 ++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/include-main.c
 create mode 100644 gdb/testsuite/gdb.base/include-main.exp
 create mode 100644 gdb/testsuite/gdb.base/main.c

diff --git a/gdb/testsuite/gdb.base/include-main.c b/gdb/testsuite/gdb.base/include-main.c
new file mode 100644
index 00000000000..ef5ac0a9f3e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/include-main.c
@@ -0,0 +1,18 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "main.c"
diff --git a/gdb/testsuite/gdb.base/include-main.exp b/gdb/testsuite/gdb.base/include-main.exp
new file mode 100644
index 00000000000..5e5e9495198
--- /dev/null
+++ b/gdb/testsuite/gdb.base/include-main.exp
@@ -0,0 +1,47 @@
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# C test-case that mimics gdb.ada/dgopt.exp.
+
+standard_testfile .c main.c
+
+if { [build_executable "failed to prepare" $testfile $srcfile {debug}] } {
+    return -1
+}
+
+clean_restart
+
+# Set language explicitly, to avoid expanding the include-main.c CU for the
+# language lookup (this is currently not required, but may be after
+# integration of the no-more-psym branch).
+# This to make sure that the source file lookup we do later triggers the
+# symtab expansion.
+gdb_test_no_output "set language c"
+
+gdb_load $binfile
+
+# Verify that no CU was expanded.
+gdb_test_no_output "maint info symtab"
+
+# List a line in include-main.c.  The tricky bit is that there's no code in
+# include-main.c, so the file should not occur in the .debug_line info.
+# GDB needs to find the file based on the CU's DW_AT_name/DW_AT_comp_dir.
+set line [gdb_get_line_number "include" $srcfile]
+gdb_test "list $srcfile:$line" "$line\[ \t\]*#include \"main.c\""
+
+# For completeness, also try to list a line in the file that does contain
+# code.
+set line [gdb_get_line_number "main" $srcfile2]
+gdb_test "list $srcfile2:$line" "$line\[ \t\]*main \\(void\\)\r\n.*"
diff --git a/gdb/testsuite/gdb.base/main.c b/gdb/testsuite/gdb.base/main.c
new file mode 100644
index 00000000000..bfe52c018d4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/main.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  return 0;
+}

base-commit: 4780e5e4933a2497a5aecc4ceabbbb8e82aaf822
-- 
2.31.1

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Simon Marchi]

On 2021-11-26 4:48 a.m., Tom de Vries via Gdb-patches wrote:
> The test-case gdb.ada/dgopt.exp uses the -gnatD switch, in combination with
> -gnatG.
>
> This causes the source file $src/gdb/testsuite/gdb.ada/dgopt/x.adb to be
> expanded into $build/gdb/testsuite/outputs/gdb.ada/dgopt/x.adb.dg, and the
> debug information should refer to the x.adb.dg file.
>
> That is the case for the .debug_line part:
> ...
> The Directory Table is empty.
>
>  The File Name Table (offset 0x1c):
>   Entry Dir     Time    Size    Name
>   1     0       0       0       x.adb.dg
> ...
> but not for the .debug_info part:
> ...
>     <11>   DW_AT_name        : $src/gdb/testsuite/gdb.ada/dgopt/x.adb
>     <15>   DW_AT_comp_dir    : $build/gdb/testsuite/outputs/gdb.ada/dgopt
> ...
>
> Filed as PR gcc/103436.
>
> In C we can generate similar debug information, using a source file that does
> not contain any code, but includes another one that does:
> ...
>  $ cat gdb/testsuite/gdb.base/include-main.c
>  #include "main.c"
> ...
> such that in the .debug_line part we have:
> ...
>  The Directory Table (offset 0x1c):
>   1     /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base
>
>  The File Name Table (offset 0x57):
>   Entry Dir     Time    Size    Name
>   1     1       0       0       main.c
> ...
> and in the .debug_info part:
> ...
>     <11>   DW_AT_name        : $src/gdb/testsuite/gdb.base/include-main.c
>     <15>   DW_AT_comp_dir    : $build/gdb/testsuite
> ...
>
> Add a C test-case that mimics gdb.ada/dgopt.exp, that is:
> - generate debug info as described above,
> - issue a list of a line in include-main.c, while the corresponding
>   CU is not expanded yet.
>
> Tested on x86_64-linux.
> ---
>  gdb/testsuite/gdb.base/include-main.c   | 18 ++++++++++
>  gdb/testsuite/gdb.base/include-main.exp | 47 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/main.c           | 22 ++++++++++++

I'm not a fan of having a file with the generic name "main.c" there.
Could we name it something like "include-main-includee.c", without
affecting the intent of the test?

Simon

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom de Vries]

On 11/26/21 5:20 PM, Simon Marchi wrote:
> On 2021-11-26 4:48 a.m., Tom de Vries via Gdb-patches wrote:
>> The test-case gdb.ada/dgopt.exp uses the -gnatD switch, in combination with
>> -gnatG.
>>
>> This causes the source file $src/gdb/testsuite/gdb.ada/dgopt/x.adb to be
>> expanded into $build/gdb/testsuite/outputs/gdb.ada/dgopt/x.adb.dg, and the
>> debug information should refer to the x.adb.dg file.
>>
>> That is the case for the .debug_line part:
>> ...
>> The Directory Table is empty.
>>
>>  The File Name Table (offset 0x1c):
>>   Entry Dir     Time    Size    Name
>>   1     0       0       0       x.adb.dg
>> ...
>> but not for the .debug_info part:
>> ...
>>     <11>   DW_AT_name        : $src/gdb/testsuite/gdb.ada/dgopt/x.adb
>>     <15>   DW_AT_comp_dir    : $build/gdb/testsuite/outputs/gdb.ada/dgopt
>> ...
>>
>> Filed as PR gcc/103436.
>>
>> In C we can generate similar debug information, using a source file that does
>> not contain any code, but includes another one that does:
>> ...
>>  $ cat gdb/testsuite/gdb.base/include-main.c
>>  #include "main.c"
>> ...
>> such that in the .debug_line part we have:
>> ...
>>  The Directory Table (offset 0x1c):
>>   1     /home/vries/gdb_versions/devel/src/gdb/testsuite/gdb.base
>>
>>  The File Name Table (offset 0x57):
>>   Entry Dir     Time    Size    Name
>>   1     1       0       0       main.c
>> ...
>> and in the .debug_info part:
>> ...
>>     <11>   DW_AT_name        : $src/gdb/testsuite/gdb.base/include-main.c
>>     <15>   DW_AT_comp_dir    : $build/gdb/testsuite
>> ...
>>
>> Add a C test-case that mimics gdb.ada/dgopt.exp, that is:
>> - generate debug info as described above,
>> - issue a list of a line in include-main.c, while the corresponding
>>   CU is not expanded yet.
>>
>> Tested on x86_64-linux.
>> ---
>>  gdb/testsuite/gdb.base/include-main.c   | 18 ++++++++++
>>  gdb/testsuite/gdb.base/include-main.exp | 47 +++++++++++++++++++++++++
>>  gdb/testsuite/gdb.base/main.c           | 22 ++++++++++++
> 
> I'm not a fan of having a file with the generic name "main.c" there.

Could you explain why not?

I don't understand why we'd need to duplicate a file that can easily be
reused.  It even helps one to recognize quickly (once you known the
contents of the file) that the test-case that is being investigated does
not require a specific executable, just based on the fact it uses "main.c".

Thanks,
- Tom

> Could we name it something like "include-main-includee.c", without
> affecting the intent of the test?
> 
> Simon
> 

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Simon Marchi]

>> I'm not a fan of having a file with the generic name "main.c" there.
>
> Could you explain why not?
>
> I don't understand why we'd need to duplicate a file that can easily be
> reused.  It even helps one to recognize quickly (once you known the
> contents of the file) that the test-case that is being investigated does
> not require a specific executable, just based on the fact it uses "main.c".

I usually don't like multiple test cases re-using the same source file,
that makes it more complicated to modify it (must make sure to not
change the behavior of the the other tests).  But the case of an empty
program is simple enough, and your reasoning makes sense, so I'm ok with
that.

Simon

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Simon Marchi]

On 2021-11-26 13:47, Simon Marchi wrote:
>>> I'm not a fan of having a file with the generic name "main.c" there.
>>
>> Could you explain why not?
>>
>> I don't understand why we'd need to duplicate a file that can easily be
>> reused.  It even helps one to recognize quickly (once you known the
>> contents of the file) that the test-case that is being investigated does
>> not require a specific executable, just based on the fact it uses "main.c".
> 
> I usually don't like multiple test cases re-using the same source file,
> that makes it more complicated to modify it (must make sure to not
> change the behavior of the the other tests).  But the case of an empty
> program is simple enough, and your reasoning makes sense, so I'm ok with
> that.
> 
> Simon
> 

I'm seeing these 2 failures starting from this commit:

FAIL: gdb.base/list-missing-source.exp: list
FAIL: gdb.base/list-missing-source.exp: info source

Simon

[committed][gdb/testsuite] Fix FAIL in gdb.base/list-missing-source.exp

[Tom de Vries]

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

[ was: Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp ]
On 11/27/21 1:59 AM, Simon Marchi wrote:
> On 2021-11-26 13:47, Simon Marchi wrote:
>>>> I'm not a fan of having a file with the generic name "main.c" there.
>>>
>>> Could you explain why not?
>>>
>>> I don't understand why we'd need to duplicate a file that can easily be
>>> reused.  It even helps one to recognize quickly (once you known the
>>> contents of the file) that the test-case that is being investigated does
>>> not require a specific executable, just based on the fact it uses "main.c".
>>
>> I usually don't like multiple test cases re-using the same source file,
>> that makes it more complicated to modify it (must make sure to not
>> change the behavior of the the other tests).  But the case of an empty
>> program is simple enough, and your reasoning makes sense, so I'm ok with
>> that.
>>
>> Simon
>>
> 
> I'm seeing these 2 failures starting from this commit:
> 
> FAIL: gdb.base/list-missing-source.exp: list
> FAIL: gdb.base/list-missing-source.exp: info source

Thanks for letting me know, fixed in commit below.

- Tom



[-- Attachment #2: 0001-gdb-testsuite-Fix-FAIL-in-gdb.base-list-missing-source.exp.patch --]
[-- Type: text/x-patch, Size: 1093 bytes --]

[gdb/testsuite] Fix FAIL in gdb.base/list-missing-source.exp

In commit f8080fb7a44 "[gdb/testsuite] Add gdb.base/include-main.exp" a
file gdb.base/main.c was added, which caused the following regression:
...
(gdb) list^M
<gdb.base/main.c>
(gdb) FAIL: gdb.base/list-missing-source.exp: list
...

The problem is that the test-case does not expect to find a file main.c, but
now it finds gdb.base/main.c.

Fix this by using the more specific file name list-missing-source.c.

Tested on x86_64-linux.

---
 gdb/testsuite/gdb.base/list-missing-source.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/list-missing-source.exp b/gdb/testsuite/gdb.base/list-missing-source.exp
index d6677fc0a5e..260ba4dcb7d 100644
--- a/gdb/testsuite/gdb.base/list-missing-source.exp
+++ b/gdb/testsuite/gdb.base/list-missing-source.exp
@@ -19,7 +19,7 @@
 standard_testfile
 
 # Create a source file in the output directory.
-set srcfile [standard_output_file main.c]
+set srcfile [standard_output_file list-missing-source.c]
 set fd [open "$srcfile" w]
 puts $fd {
 int

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Add a C test-case that mimics gdb.ada/dgopt.exp, that is:
Tom> - generate debug info as described above,
Tom> - issue a list of a line in include-main.c, while the corresponding
Tom>   CU is not expanded yet.

I noted this in another patch submission, but I thought I'd point it out
here: this test case also fails for me, with git gdb, with the fission
and fission-dwp target boards.  I'm using the Fedora 34 system gcc.

E.g.:

    $ runtest --target_board=fission gdb.base/include-main.exp
    [...]
    Running /home/tromey/gdb/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.base/include-main.exp ...
    FAIL: gdb.base/include-main.exp: list include-main.c:18

Given this, I wonder if we really want to continue supporting this.  I
think it may be better to just declare these as compiler bugs (including
gdb.ada/dgopt.exp) when using fission.

The rationale for this is just that the line table and the CU DIE ought
to agree, and if they don't, GDB can just declare that it respects the
line table and may ignore the CU DIE.

What do you think of this?

Fixing this in the fission case is a pain because the GDB index is
intentionally set up to avoid reading the CU DIE unless the CU is going
to be expanded.

thanks,
Tom

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom Tromey]

Tom> Given this, I wonder if we really want to continue supporting this.  I
Tom> think it may be better to just declare these as compiler bugs (including
Tom> gdb.ada/dgopt.exp) when using fission.

Well, it so happens that my patches fix the dgopt.exp fission cases with
my branch (but not the include-main.exp fission cases).  I was thinking
maybe I'd be able to drop these patches, but they're also needed for
plain include-main.exp on the branch.  So I think we can just carry on
as-is, since I don't think the new reader will regress anything now, and
the new patches aren't really that intrusive.

Tom

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom de Vries]

On 12/1/21 2:10 AM, Tom Tromey wrote:
> Tom> Given this, I wonder if we really want to continue supporting this.  I
> Tom> think it may be better to just declare these as compiler bugs (including
> Tom> gdb.ada/dgopt.exp) when using fission.
> 

I still don't understand what specific compiler behavior you consider a bug.

> Well, it so happens that my patches fix the dgopt.exp fission cases with
> my branch (but not the include-main.exp fission cases).  I was thinking
> maybe I'd be able to drop these patches, but they're also needed for
> plain include-main.exp on the branch.  So I think we can just carry on
> as-is, since I don't think the new reader will regress anything now, and
> the new patches aren't really that intrusive.

OK, that sounds good :)

FYI, I've now debugged the fission failure of include-main.exp on trunk,
and found that it's because dw2_get_file_names uses a cutu reader
constructor that ignores the dwo file.  By using the other constructor,
the test passes:
...
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 57538fc0adf..04c258d1aef 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3084,7 +3084,7 @@ dw2_get_file_names (dwarf2_per_cu_data *this_cu,
   if (this_cu->v.quick->files_read)
     return this_cu->v.quick->file_names;

-  cutu_reader reader (this_cu, per_objfile);
+  cutu_reader reader (this_cu, per_objfile, nullptr, nullptr, false);
   if (!reader.dummy_p)
     dw2_get_file_names_reader (&reader, reader.comp_unit_die);

...

I'm testing this currently.

Thanks,
- Tom

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom Tromey]

> Given this, I wonder if we really want to continue supporting this.  I
> think it may be better to just declare these as compiler bugs (including
> gdb.ada/dgopt.exp) when using fission.

Tom> I still don't understand what specific compiler behavior you consider a bug.

The file name in the CU DIE does not appear in the line table.

Tom> FYI, I've now debugged the fission failure of include-main.exp on trunk,
Tom> and found that it's because dw2_get_file_names uses a cutu reader
Tom> constructor that ignores the dwo file.  By using the other constructor,
Tom> the test passes:

I considered this, but when I looked, the .dwo file did not include the
DW_AT_stmt_list.  So, I thought this would introduce other problems.

If this can work, then I think this is the only use of this constructor
and so it could be removed.

Tom

Re: [PATCH] [gdb/testsuite] Add gdb.base/include-main.exp

[Tom de Vries]

On 12/1/21 6:27 PM, Tom Tromey wrote:
>> Given this, I wonder if we really want to continue supporting this.  I
>> think it may be better to just declare these as compiler bugs (including
>> gdb.ada/dgopt.exp) when using fission.
> 
> Tom> I still don't understand what specific compiler behavior you consider a bug.
> 
> The file name in the CU DIE does not appear in the line table.
> 

OK, it took me a while to understand, but I agree.

I was confused by a comment in the dwarf 5 standard:
...
Prior to DWARF Version 5, the current compilation file name was not
represented in the file_names field.
...
But that seems to be incorrect, there's an issue open to correct that (
https://dwarfstd.org/ShowIssue.php?issue=210713.1 ).

Anyway, with trunk gcc this no longer occurs, I've bisected the
difference to
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96c82a16b2076891a9974d0f0e96a0b85fbc2df4
.

> Tom> FYI, I've now debugged the fission failure of include-main.exp on trunk,
> Tom> and found that it's because dw2_get_file_names uses a cutu reader
> Tom> constructor that ignores the dwo file.  By using the other constructor,
> Tom> the test passes:
> 
> I considered this, but when I looked, the .dwo file did not include the
> DW_AT_stmt_list.  So, I thought this would introduce other problems.
> 
> If this can work, then I think this is the only use of this constructor
> and so it could be removed.

I ran into regressions when testing this, so I filed a PR, mentioned the
tentative patch and a specific regression (
https://sourceware.org/bugzilla/show_bug.cgi?id=28644 ).

I'm parking this for the moment.

Thanks,
- Tom