From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id sHQUGA6NcWJxFwQAWB0awg (envelope-from ) for ; Tue, 03 May 2022 16:14:06 -0400 Received: by simark.ca (Postfix, from userid 112) id 534A01E048; Tue, 3 May 2022 16:14:06 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=TBFZId4y; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_DYNAMIC, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id A8F901E00E for ; Tue, 3 May 2022 16:14:05 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 46C4F3858D3C for ; Tue, 3 May 2022 20:14:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 46C4F3858D3C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1651608845; bh=bk6i7z/B6WhJIAgoz0zOHDwjBz3D/QY7vYG4NT4/zJk=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=TBFZId4yY73fxM1sn8Vju2Vpww+BEcK/3v/W0+0huxl7gCEWQ4o3uTgut05bAF2B+ 7+fx35oroTne/ugTeLY18DTrs7Eidn1rQ57e5aImWv6soywniCG0IHjMVxPMkY1cZq bng36fDaGMF9VuBAjGDLqCIa/OW3ntK22kY8XUuM= 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 BA0993858D3C for ; Tue, 3 May 2022 20:13:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org BA0993858D3C 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-564-bPZsMo34NdykwuYtQYL0_Q-1; Tue, 03 May 2022 16:13:03 -0400 X-MC-Unique: bPZsMo34NdykwuYtQYL0_Q-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E3DE8A6C625; Tue, 3 May 2022 20:13:02 +0000 (UTC) Received: from [10.97.116.19] (ovpn-116-19.gru2.redhat.com [10.97.116.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 18389400F736; Tue, 3 May 2022 20:13:01 +0000 (UTC) Message-ID: Date: Tue, 3 May 2022 17:12:59 -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 v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name To: Simon Marchi , gdb-patches@sourceware.org References: <20220428033542.1636284-1-simon.marchi@polymtl.ca> <20220428033542.1636284-5-simon.marchi@polymtl.ca> In-Reply-To: <20220428033542.1636284-5-simon.marchi@polymtl.ca> X-Scanned-By: MIMEDefang 2.84 on 10.11.54.1 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-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: , From: Bruno Larsen via Gdb-patches Reply-To: Bruno Larsen Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Hi Simon! On 4/28/22 00:35, Simon Marchi via Gdb-patches wrote: > In the following patch, there will be some callers of file_file_name > that will already have access to the file_entry object for which they > want the file name. It would be inefficient to have them pass an index, > only for line_header::file_file_name to re-lookup the same file_entry > object. Change line_header::file_file_name to accept a file_entry > object reference, instead of an index to look up. > > I think this change makes sense in any case. Callers that have an index > can first obtain a file_entry using line_header::file_name_at or > line_header::file_names. > > When passing a file_entry object, we can assume that the file_entry's > index is valid, unlike when passing an index. So, push the special case > about an invalid index to the sole current caller of file_file_name, > macro_start_file. I think that error belongs there anyway, since it > specifically talks about "bad file number in macro information". > > This requires recording the file index in the file_entry structure, so > add that. > Thanks for looking at this! We definitely need some movement in the file side of things. The general direction looks good, but I have some questions Related to this patch, do you feel like it could be worth trying to centralize files into a single class? Joining file_entry, file_and_directory, and the information contained in symtabs into a single class (maybe file_info?) that will always handle names and paths the same way? > Change-Id: Ic6e44c407539d92b7863d7ba82405ade17f384ad > --- > gdb/dwarf2/line-header.c | 47 ++++++++++++---------------------------- > gdb/dwarf2/line-header.h | 10 ++++++--- > gdb/dwarf2/macro.c | 16 +++++++++++++- > 3 files changed, 36 insertions(+), 37 deletions(-) > > diff --git a/gdb/dwarf2/line-header.c b/gdb/dwarf2/line-header.c > index 13379851b9b6..33af77d3ecf3 100644 > --- a/gdb/dwarf2/line-header.c > +++ b/gdb/dwarf2/line-header.c > @@ -48,47 +48,28 @@ line_header::add_file_name (const char *name, > unsigned int mod_time, > unsigned int length) > { > + file_name_index index > + = version >= 5 ? file_names_size (): file_names_size () + 1; > + > if (dwarf_line_debug >= 2) > - { > - size_t new_size; > - if (version >= 5) > - new_size = file_names_size (); > - else > - new_size = file_names_size () + 1; > - gdb_printf (gdb_stdlog, "Adding file %zu: %s\n", > - new_size, name); > - } > - m_file_names.emplace_back (name, d_index, mod_time, length); > + gdb_printf (gdb_stdlog, "Adding file %d: %s\n", index, name); > + > + m_file_names.emplace_back (name, index, d_index, mod_time, length); > } > > std::string > -line_header::file_file_name (int file) const > +line_header::file_file_name (const file_entry &fe) 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)) > - { > - const file_entry *fe = file_name_at (file); > + gdb_assert (is_valid_file_index (fe.index)); > > - if (!IS_ABSOLUTE_PATH (fe->name)) > - { > - const char *dir = fe->include_dir (this); > - if (dir != NULL) > - return path_join (dir, fe->name); > - } > + if (IS_ABSOLUTE_PATH (fe.name)) > + return fe.name; > > - return fe->name; > - } > - else > - { > - /* The compiler produced a bogus file number. We can at least > - record the macro definitions made in the file, even if we > - won't be able to find the file by name. */ > - complaint (_("bad file number in macro information (%d)"), > - file); > + const char *dir = fe.include_dir (this); > + if (dir != nullptr) > + return path_join (dir, fe.name); > > - return string_printf ("", file); > - } > + return fe.name; I'm a bit confused about this last return. Why have a shortcut for absolute paths, then attempting to get the include dir, then returning only the name if that fails, instead of simplifying to something like: if (!IS_ABSOLUTE_PATH (fe.name) { const char *dir = fe.include_dir (this); if (dir != nullptr) return path_join (dir, fe.name); } return fe.name; > } > > static void > diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h > index 53db3a1d5aa8..0fe539b0c427 100644 > --- a/gdb/dwarf2/line-header.h > +++ b/gdb/dwarf2/line-header.h > @@ -36,9 +36,10 @@ struct file_entry > { > file_entry () = default; > > - file_entry (const char *name_, dir_index d_index_, > + file_entry (const char *name_, file_name_index index_, dir_index d_index_, > unsigned int mod_time_, unsigned int length_) > : name (name_), > + index (index_), > d_index (d_index_), > mod_time (mod_time_), > length (length_) > @@ -52,6 +53,9 @@ struct file_entry > owned by debug_line_buffer. */ > const char *name {}; > > + /* The index of this file in the file table. */ > + file_name_index index {}; > + > /* The directory index (1-based). */ > dir_index d_index {}; > > @@ -168,8 +172,8 @@ struct line_header > const gdb_byte *statement_program_start {}, *statement_program_end {}; > > /* Return file name relative to the compilation directory of file > - number FILE in this object's file name table. */ > - std::string file_file_name (int file) const; > + FE in this object's file name table. */ > + std::string file_file_name (const file_entry &fe) const; > > /* Return the compilation directory of the compilation unit in the context of > which this line header is read. Return nullptr if non applicable. */ > diff --git a/gdb/dwarf2/macro.c b/gdb/dwarf2/macro.c > index 99c3653a2c3a..38c0fdfec738 100644 > --- a/gdb/dwarf2/macro.c > +++ b/gdb/dwarf2/macro.c > @@ -52,7 +52,21 @@ macro_start_file (buildsym_compunit *builder, > const struct line_header *lh) > { > /* File name relative to the compilation directory of this source file. */ > - std::string file_name = lh->file_file_name (file); > + const file_entry *fe = lh->file_name_at (file); > + std::string file_name; > + > + if (fe != nullptr) > + file_name = lh->file_file_name (*fe); > + else > + { > + /* The compiler produced a bogus file number. We can at least > + record the macro definitions made in the file, even if we > + won't be able to find the file by name. */ > + complaint (_("bad file number in macro information (%d)"), > + file); > + > + file_name = string_printf ("", file); > + } > > if (! current_file) > { Cheers! Bruno Larsen