From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 7048A385840D for ; Fri, 19 Nov 2021 18:55:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7048A385840D Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-214-Mi9487H2Ma6peFAUfV5Qew-1; Fri, 19 Nov 2021 13:55:50 -0500 X-MC-Unique: Mi9487H2Ma6peFAUfV5Qew-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C7FF11923761; Fri, 19 Nov 2021 18:55:49 +0000 (UTC) Received: from [10.3.112.120] (ovpn-112-120.phx2.redhat.com [10.3.112.120]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 711E860C82; Fri, 19 Nov 2021 18:55:49 +0000 (UTC) Message-ID: Date: Fri, 19 Nov 2021 10:55:48 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH] Fix for the gdb.base/macscp.exp testcase failure with split dwarf. To: "Achra, Nitika" , "gdb-patches@sourceware.org" Cc: "George, Jini Susan" References: From: Keith Seitz In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Nov 2021 18:55:55 -0000 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://sourceware.org/gdb/wiki/ContributionChecklist .] Have you tested this patch against any other compilers? This patch includes a patch I posted an RFC on several months ago: https://sourceware.org/pipermail/gdb-patches/2021-June/179698.html 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 > 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 (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