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 890853836431 for ; Wed, 11 May 2022 14:53:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 890853836431 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-171-zgTIsZ8LPgi1DYX2NCkc2Q-1; Wed, 11 May 2022 10:53:39 -0400 X-MC-Unique: zgTIsZ8LPgi1DYX2NCkc2Q-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 331371801387; Wed, 11 May 2022 14:53:38 +0000 (UTC) Received: from [10.97.116.38] (ovpn-116-38.gru2.redhat.com [10.97.116.38]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 70EAD2026D6A; Wed, 11 May 2022 14:53:37 +0000 (UTC) Message-ID: Date: Wed, 11 May 2022 11:53:35 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH] gdb: Fix issue with Clang CLI macros To: Simon Marchi , gdb-patches@sourceware.org References: <20220420174111.220211-1-blarsen@redhat.com> <279a5be0-36a4-26d2-00f6-6f58e71d7187@simark.ca> From: Bruno Larsen In-Reply-To: <279a5be0-36a4-26d2-00f6-6f58e71d7187@simark.ca> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 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=-12.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, 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: Wed, 11 May 2022 14:53:44 -0000 On 5/11/22 09:59, Simon Marchi wrote: > > > On 2022-04-20 13:41, Bruno Larsen via Gdb-patches wrote: >> Clang up to the current version adds macros that were defined in the >> command line or by "other means", according to the Dwarf specification, >> after the last DW_MACRO_end_file, instead of before the first >> DW_MACRO_start_file, as the specification dictates. This has been >> submitted as a bug to Clang developers, but seeing as there is no >> expected date for it to be fixed, a workaround was added for all current >> versions of Clang. >> >> The workaround detects when the main file would be closed and if the >> producer is clang, and turns that operation into a noop, so we keep a >> reference to the current_file as those macros are read. >> >> This patch fixes PR macros/29034, and can be tested by running >> gdb.base/macscp.exp using clang, the test printing FROM_COMMANDLINE >> should be fixed. > > Hi Bruno, Hi Simon! Thanks for the review, I've fixed all minor comments locally and will send a v2 shortly. > > Can we also add a DWARF-assembly-based test case, so that this is tested > regardless of CC_FOR_TARGET? I can do that, but since I have a producer check and I want to specifically fix this clang bug, the test would want to see nothing happening on regular testing, and the macro showing up for clang. Is this what you had in mind? > > My series here adds some code to generate DWARF5 .debug_macro sections: > > https://sourceware.org/pipermail/gdb-patches/2022-April/188478.html > >> --- >> gdb/dwarf2/cu.h | 1 + >> gdb/dwarf2/macro.c | 17 +++++++++++++---- >> gdb/dwarf2/macro.h | 2 +- >> gdb/dwarf2/read.c | 15 ++++++++++++++- >> gdb/dwarf2/read.h | 2 ++ >> gdb/producer.c | 26 ++++++++++++++++++++++++++ >> gdb/producer.h | 4 ++++ >> 7 files changed, 61 insertions(+), 6 deletions(-) >> >> diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h >> index 6b72ec234bf..3b7f92be406 100644 >> --- a/gdb/dwarf2/cu.h >> +++ b/gdb/dwarf2/cu.h >> @@ -257,6 +257,7 @@ struct dwarf2_cu >> bool producer_is_icc : 1; >> bool producer_is_icc_lt_14 : 1; >> bool producer_is_codewarrior : 1; >> + bool producer_is_clang_le_14 : 1; >> >> /* When true, the file that we're processing is known to have >> debugging info for C++ namespaces. GCC 3.3.x did not produce >> diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c >> index 99c3653a2c3..305af1a7f47 100644 >> --- a/gdb/dwarf2/macro.c >> +++ b/gdb/dwarf2/macro.c >> @@ -431,7 +431,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile, >> struct dwarf2_section_info *str_section, >> struct dwarf2_section_info *str_offsets_section, >> gdb::optional str_offsets_base, >> - htab_t include_hash) >> + htab_t include_hash, struct dwarf2_cu *cu) > > I think having both a dwarf2_per_objfile parameter and a dwarf2_cu parameter is > redundant, as you can get the per_objfile from the cu. So perhaps just replace > the existing per_objfile parameter with the cu one (at the same position). > >> { >> struct objfile *objfile = per_objfile->objfile; >> enum dwarf_macro_record_type macinfo_type; >> @@ -658,6 +658,15 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile, >> if (! current_file) >> complaint (_("macro debug info has an unmatched " >> "`close_file' directive")); >> + else if (current_file->included_by == nullptr >> + && producer_is_clang_le_14 (cu)) > > Given that we don't really expect this to be fixed soon (nobody has expressed the > willingness to fix it), shouldn't we check for "producer_is_clang", without a > version check? Otherwise this will need to be updated whenever a new Clang version > is released, if the bug hasn't been fixed. Sure! I just didn't want us to forget this was supposed to be a quick bug fix in a few months, so I went with the overzealous route. I do still think this should be updated to producer_is_clang_le_XX once they fix it, to guarantee that they don't regress. > >> + { >> + /* Clang, until the current version, misplaces macro definitions, > > It's not clear what "current version" means, as the definition of that changes with > time. I would say "Clang (at the time of writing) misplaces...". I would even put > a link to the Clang bug, so that people can easily go check if that is still the case > or if that has been fixed. > > You should say "command-line macro definitions (e.g. -DFOO=1)", because it's not > all macro definitions.> > > >> + putting them after the last DW_MACRO_end_file instead of >> + before the first DW_MACRO_start_file. This condition should be >> + removed once we can reasonably expect most clang users to have >> + updated to a fixed version. */ >> + } >> else >> { >> current_file = current_file->included_by; >> @@ -737,7 +746,7 @@ dwarf_decode_macro_bytes (dwarf2_per_objfile *per_objfile, >> current_file, lh, section, >> section_is_gnu, is_dwz, offset_size, >> str_section, str_offsets_section, >> - str_offsets_base, include_hash); >> + str_offsets_base, include_hash, cu); >> >> htab_remove_elt (include_hash, (void *) new_mac_ptr); >> } >> @@ -781,7 +790,7 @@ dwarf_decode_macros (dwarf2_per_objfile *per_objfile, >> unsigned int offset, struct dwarf2_section_info *str_section, >> struct dwarf2_section_info *str_offsets_section, >> gdb::optional str_offsets_base, >> - int section_is_gnu) >> + int section_is_gnu, struct dwarf2_cu *cu) >> { >> bfd *abfd; >> const gdb_byte *mac_ptr, *mac_end; >> @@ -942,5 +951,5 @@ dwarf_decode_macros (dwarf2_per_objfile *per_objfile, >> dwarf_decode_macro_bytes (per_objfile, builder, abfd, mac_ptr, mac_end, >> current_file, lh, section, section_is_gnu, 0, >> offset_size, str_section, str_offsets_section, >> - str_offsets_base, include_hash.get ()); >> + str_offsets_base, include_hash.get (), cu); >> } >> diff --git a/gdb/dwarf2/macro.h b/gdb/dwarf2/macro.h >> index e8c33a34a8d..02753ef377a 100644 >> --- a/gdb/dwarf2/macro.h >> +++ b/gdb/dwarf2/macro.h >> @@ -31,6 +31,6 @@ extern void dwarf_decode_macros (dwarf2_per_objfile *per_objfile, >> dwarf2_section_info *str_section, >> dwarf2_section_info *str_offsets_section, >> gdb::optional str_offsets_base, >> - int section_is_gnu); >> + int section_is_gnu, struct dwarf2_cu *cu); >> >> #endif /* GDB_DWARF2_MACRO_H */ >> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c >> index 698720276a9..1fc5d5d3dd8 100644 >> --- a/gdb/dwarf2/read.c >> +++ b/gdb/dwarf2/read.c >> @@ -9454,6 +9454,15 @@ producer_is_gcc_lt_4_3 (struct dwarf2_cu *cu) >> return cu->producer_is_gcc_lt_4_3; >> } >> >> +bool >> +producer_is_clang_le_14 (struct dwarf2_cu *cu) >> +{ >> + if (!cu->checked_producer) >> + check_producer (cu); >> + >> + return cu->producer_is_clang_le_14; >> +} >> + >> static file_and_directory & >> find_file_and_directory (struct die_info *die, struct dwarf2_cu *cu) >> { >> @@ -13295,6 +13304,10 @@ check_producer (struct dwarf2_cu *cu) >> } >> else if (startswith (cu->producer, "CodeWarrior S12/L-ISA")) >> cu->producer_is_codewarrior = true; >> + else if (producer_is_clang (cu->producer, &major, &minor)) >> + { >> + cu->producer_is_clang_le_14 = major <= 14; >> + } > > Remove braces. > >> else >> { >> /* For other non-GCC compilers, expect their behavior is DWARF version >> @@ -23218,7 +23231,7 @@ dwarf_decode_macros (struct dwarf2_cu *cu, unsigned int offset, >> >> dwarf_decode_macros (per_objfile, builder, section, lh, >> offset_size, offset, str_section, str_offsets_section, >> - str_offsets_base, section_is_gnu); >> + str_offsets_base, section_is_gnu, cu); >> } >> >> /* Return the .debug_loc section to use for CU. >> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h >> index be3b9d19601..23f040a6ca8 100644 >> --- a/gdb/dwarf2/read.h >> +++ b/gdb/dwarf2/read.h >> @@ -677,4 +677,6 @@ extern void dwarf2_get_section_info (struct objfile *, >> asection **, const gdb_byte **, >> bfd_size_type *); >> >> +extern bool producer_is_clang_le_14(struct dwarf2_cu *cu); >> + >> #endif /* DWARF2READ_H */ >> diff --git a/gdb/producer.c b/gdb/producer.c >> index ef1dd93afbc..a9fb7dd8245 100644 >> --- a/gdb/producer.c >> +++ b/gdb/producer.c >> @@ -127,6 +127,32 @@ producer_is_llvm (const char *producer) >> || startswith (producer, " F90 Flang "))); > > Just noting that users of these functions will need to be careful, as there's > an overlap between producer_is_llvm and producer_is_clang. > >> } >> >> +/* see producer.h */ > > Capital S, period + two spaces at the end. > >> + >> +bool >> +producer_is_clang (const char* producer, int *major, int* minor) > > Space before * (twice). > >> +{ >> + const char *cs; > > Declare in the inner scope (at the point of use). > >> + >> + if (producer != nullptr && startswith (producer, "clang ")) >> + { >> + int maj, min; >> + if (major == nullptr) >> + major = &maj; >> + if (minor == nullptr) >> + minor = &min; >> + >> + /* The full producer sting will look something like > > sting -> string > >> + "clang version XX.X.X ..." >> + So we can safely ignore all characters before the first digit. */ >> + cs = producer + strlen("clang version "); > > Space after strlen. > >> + >> + if (sscanf (cs, "%d.%d", major, minor) == 2) >> + return true; > > Remove one indentation level here. > >> + } >> + return false; >> +} > > Can you add some tests for this in producer_parsing_tests?> > Simon > Cheers! Bruno Larsen