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.129.124]) by sourceware.org (Postfix) with ESMTPS id 23CEE3888825 for ; Fri, 18 Mar 2022 20:15:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 23CEE3888825 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-365-QxeNuGjMO7mdh2q-UI86Ng-1; Fri, 18 Mar 2022 16:15:08 -0400 X-MC-Unique: QxeNuGjMO7mdh2q-UI86Ng-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id A9379185A7A4; Fri, 18 Mar 2022 20:15:07 +0000 (UTC) Received: from [10.2.17.71] (unknown [10.2.17.71]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 26D22403373; Fri, 18 Mar 2022 20:15:07 +0000 (UTC) Message-ID: <845ed59c-e989-6ba0-933b-6dd508b8cbd7@redhat.com> Date: Fri, 18 Mar 2022 13:15:05 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.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" , "Natarajan, Kavitha" References: From: Keith Seitz In-Reply-To: X-Scanned-By: MIMEDefang 2.85 on 10.11.54.10 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: 8bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE 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, 18 Mar 2022 20:15:12 -0000 On 3/14/22 07:32, Achra, Nitika wrote: > Thanks for the review and sorry for the delay in the reply. No problem; it happens. [At least to me!] I'm happy to see you return to the problem/patch. > 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" Just a cautionary note: When running C++ tests (gdb.cp), don't forget to set CXX_FOR_TARGET, too. > Compiler used: clang 12.0.0 I tested w/GCC and Clang 12 (Fedora 12.0.1-1.fc34) GCC results show no regressions. > *Before the patch:* > > Number of expected passes            234 > Number of unexpected failures        65 > Number of known failures             40 I also see those results with origin/master. > *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 After applying both our patches, I have an equal number of PASSes, but 4 unexpected failures. [which could mean just about anything] > 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. I think there is still something missing. Regression testing the entire testsuite (with both our patches), I see the following three issues (w/clang; gcc is fine): 1. gdb.base/included.exp (gdb) list integer -18 int integer; [PASS] +18 #include "included.h" [FAIL] 2. gdb.cp/m-static.exp (gdb) info variable everywhere -File ../../../src/gdb/testsuite/gdb.cp/m-static.h: [PASS] +File ../../../src/gdb/testsuite/gdb.cp/m-static.cc: [FAIL] 3. gdb.threads/siginfo-threads.exp (gdb) p $_siginfo.si_signo -$4 = 3182372 [PASS] +There is no member named _sifields. [FAIL] (gdb) p $_siginfo._sifields._kill.si_pid -$7 = 3182372 [PASS] +There is no member named _sifields. [FAIL] I haven't pursued these further to understand the problem. See other discussion below. Keith > diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h > index 8fb44be56b2..48cf9a5b742 100644 > --- a/gdb/dwarf2/line-header.h > +++ b/gdb/dwarf2/line-header.h > @@ -44,9 +44,12 @@ struct file_entry > length (length_) > {} > > - /* Return the include directory at D_INDEX stored in LH. Returns > - NULL if D_INDEX is out of bounds. */ > - const char *include_dir (const line_header *lh) const; > + /* Return the include directory at D_INDEX stored in m_include_dirs vector > + in LH or in m_dwo_include_dirs vector in LH if IS_DWO is true. m_dwo_ > + include_dirs contains the include_directories entries of .debug_line.dwo > + section whereas m_include_dirs contains the include_directories array > + entries of .debug_line section. Returns NULL if D_INDEX is out of bounds. */ > + const char *include_dir (const line_header *lh, bool is_dwo) const; Regarding passing a boolean everywhere (like above) and special-casing on this, like: > - /* Return the include dir at INDEX (0-based in DWARF 5 and 1-based before). > + /* Return the include dir from m_include_dirs or from the m_dwo_include_dirs > + if IS_DWO is true at INDEX (0-based in DWARF 5 and 1-based before). > Returns NULL if INDEX is out of bounds. */ > - const char *include_dir_at (dir_index index) const > + const char *include_dir_at (dir_index index, bool is_dwo) const > { > int vec_index; > - if (version >= 5) > + > + if (is_dwo && m_dwo_include_dirs.empty()) > + is_dwo = false; > + > + if ((is_dwo && dwo_version >= 5) || (!is_dwo && version >= 5)) > vec_index = index; > else > vec_index = index - 1; > - if (vec_index < 0 || vec_index >= m_include_dirs.size ()) > + int include_dirs_size = is_dwo ? m_dwo_include_dirs.size () : > + m_include_dirs.size (); > + if (vec_index < 0 || vec_index >= include_dirs_size) > return NULL; > - return m_include_dirs[vec_index]; > + return is_dwo ? m_dwo_include_dirs[vec_index] : > + m_include_dirs[vec_index]; > } I've seen this implementation pattern elsewhere (mostly older code), and I just can't help but ask... It seems to unnecessarily clutter the code compared to creating a new struct/class, inheriting from the existing line_header. [In other words, it seems C-ish to me.] Was that approach considered? Keith