[PATCH] Fix for the gdb.base/macscp.exp testcase failure with split dwarf.
Achra, Nitika
Nitika.Achra@amd.com
Mon Mar 14 14:32:00 GMT 2022
[AMD Official Use Only]
Hi Keith,
Thanks for the review and sorry for the delay in the reply. I have fixed the regressions mentioned by you. Also, I have removed the change you have posted for the RFC. I have attached the updated patch. So, this patch along with your fix will result in the following for gdb.base/macscp.exp testcase with -gsplit-dwarf:
Command Used:
make check RUNTESTFLAGS='CC_FOR_TARGET="clang -gdwarf-5 -gsplit-dwarf -fdebug-macro"'
TESTS="gdb.base/macscp.exp"
Compiler used: clang 12.0.0
Before the patch:
Number of expected passes 234
Number of unexpected failures 65
Number of known failures 40
After applying the attached patch and including your change also:
Number of expected passes 319
Number of unexpected failures 1
Number of known failures 19
Compiler used: clang 12.0.0
Root cause and the fix for these failures:
Clang is emitting both .debug_line and .debug_line.dwo sections and .debug_macro.dwo is pointing to the .debug_line.dwo section(file index in the .debug_macro.dwo is the index in the file_names array of
.debug_line.dwo). However, GDB is reading only .debug_line section. In this patch, I have added the support
for reading .debug_line.dwo section and added m_dwo_include_dirs and m_dwo_file_names vectors in struct line_header to store the directories and files respectively and reading the file and directory names from
these vectors whenever required.
Could you please take another look? Thanks in advance.
Regards,
Nitika
Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows
From: Keith Seitz<mailto:keiths@redhat.com>
Sent: Saturday, November 20, 2021 12:25 AM
To: Achra, Nitika<mailto:Nitika.Achra@amd.com>; gdb-patches@sourceware.org<mailto:gdb-patches@sourceware.org>
Cc: George, Jini Susan<mailto:JiniSusan.George@amd.com>
Subject: Re: [PATCH] Fix for the gdb.base/macscp.exp testcase failure with split dwarf.
[CAUTION: External Email]
On 11/18/21 22:56, Achra, Nitika via Gdb-patches wrote:
> [AMD Official Use Only]
>
> Requesting to please review the attached patch.
Hi, thank you for submitting your patch for review!
I've given the patch a very quick look, and in general,
I think this is on the right path. Based on a preliminary
review, however, I have several questions (and a request or two).
> Command used: make check RUNTESTFLAGS='CC_FOR_TARGET="clang -gdwarf-5 -gsplit-dwarf -fdebug-macro"'
> TESTS="gdb.base/macscp.exp">
> Before the patch:
> Number of expected passes 234
> Number of unexpected failures 65
> Number of known failures 40
>
> After the patch:
> Number of expected passes 315
> Number of unexpected failures 1
> Number of known failures 23
>
> Compiler used: clang 12.0.0
While you've described the ultimate outcome of this patch, I am
missing a description of how you fixed it. Yes, I could study the patch,
but a written preface would help set up what we are reviewing.
[See advice in the official Contribution Checklist,
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsourceware.org%2Fgdb%2Fwiki%2FContributionChecklist&data=04%7C01%7Cnitika.achra%40amd.com%7Ca3fd2143112f429956c508d9ab8e36cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637729449561032249%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=AFlGKgdVVamKqpddCxazSqqty7b5uvgcCHcvHEPwR1M%3D&reserved=0 .]
Have you tested this patch against any other compilers? This patch includes
a patch I posted an RFC on several months ago:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsourceware.org%2Fpipermail%2Fgdb-patches%2F2021-June%2F179698.html&data=04%7C01%7Cnitika.achra%40amd.com%7Ca3fd2143112f429956c508d9ab8e36cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637729449561032249%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=zQ5bO%2FwGGpRJYB4VXslE%2BDwCDTxCDxhnh04feoMobCA%3D&reserved=0
In that thread, Simon discusses testing this line_header::file_file_name
change against several compilers, including gcc and clang. More on this
below.
While the goal of this patch is to improve the -gsplit-dwarf experience
(which I gather is mainly used by clang users), I would certainly like to know
how this patch may impact users not using this compiler configuration, including
gcc users.
To understand the impact here, I ran this patch using gcc-11 on
gdb.base/macscp.exp. [Results are same -gdwarf-4 or -gdwarf-5]
unpatched; default flags
# of expected passes 320
# of known failures 19
w/this patch; default flags
# of expected passes 317
# of known failures 22
unpatched; with -gsplit-dwarf
# of expected passes 1
# of unsupported tests 1
w/this patch; with -gsplit-dwarf
# of expected passes 179
# of unexpected failures 121
# of known failures 40
These discrepancies need at the least an explanation.
I also ran the entirety of the test suite against gcc-11 with/without this
patch applied (no -gsplit-dwarf). Other than the above gdb.base/macscp.exp,
this patch would introduce regressions in gdb.dwarf2/fission-base.exp:
PASS: gdb.dwarf2/fission-base.exp: ptype main
PASS: gdb.dwarf2/fission-base.exp: ptype func
-PASS: gdb.dwarf2/fission-base.exp: frame in main
-PASS: gdb.dwarf2/fission-base.exp: break func
-PASS: gdb.dwarf2/fission-base.exp: continue to func
-PASS: gdb.dwarf2/fission-base.exp: frame in func
+FAIL: gdb.dwarf2/fission-base.exp: frame in main
+FAIL: gdb.dwarf2/fission-base.exp: break func
+FAIL: gdb.dwarf2/fission-base.exp: continue to func
+FAIL: gdb.dwarf2/fission-base.exp: frame in func
I also ran the full testsuite using clang-12. That shows the
above regressions plus a few more related to fission.
[gdb.dwarf2/fission-reread.exp and gdb.dwarf2/fission-multi-cu.exp]
> gdb/ChangeLog:
>
> * dwarf2/line-header.c (add_include_dir): Handle .debug_line.dwo
> include directories table entries.
> * (add_file_name): Handle .debug_line.dwo file names entries.
> * (file_file_name): Read the file name from .debug_line.dwo or
> .debug_line sections.
We no longer use/require ChangeLog entries. :-)
Until the above issues are documented and/or resolved, I will forgo a
more in-depth look at the patch, but I do want to mention this change:
> 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. */
> - if (is_valid_file_index (file))
> + table? First check in the .debug_line.dwo file table and then in
> + .debug_line table. Remember that file numbers start with one in
> + DWARFv4 and with zero in DWARFv5. */
> + if (is_valid_dwo_file_index (file))
> {
> - 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);
> + const file_entry *fe = file_name_at (file, true);
> + return make_unique_xstrdup (fe->symtab->filename);
> + }
> + else if (is_valid_file_index (file))
> + {
> + const file_entry *fe = file_name_at (file, false);
> + return make_unique_xstrdup (fe->symtab->filename);
> }
> else
> {
This is the change that Simon and I were discussing several months ago.
Coincidentally, I have been revisiting this patch and testing it this
week, and I believe this bit (using the symtab's filename) is a significant
enough change that it should be submitted separately to discuss.
Keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-for-the-gdb.base-macscp.exp-testcase-failure-wit.patch
Type: application/octet-stream
Size: 31563 bytes
Desc: 0001-Fix-for-the-gdb.base-macscp.exp-testcase-failure-wit.patch
URL: <https://sourceware.org/pipermail/gdb-patches/attachments/20220314/241eb27e/attachment-0001.obj>
More information about the Gdb-patches
mailing list