From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Fz+LEN+44mLnBxwAWB0awg (envelope-from ) for ; Thu, 28 Jul 2022 12:27:11 -0400 Received: by simark.ca (Postfix, from userid 112) id 339291E9ED; Thu, 28 Jul 2022 12:27:11 -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=x6pRpV4h; 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 8E3481E5EA for ; Thu, 28 Jul 2022 12:27:10 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 09DFB3857C5F for ; Thu, 28 Jul 2022 16:27:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 09DFB3857C5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1659025630; bh=isQKFulArt4yT10MMYbrMfaTzP6CAY7QQhASY3FHk7k=; 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=x6pRpV4huSJQzR0GG33MxzV/qOTAkhnt7PI6eauosCtRMgAEdpMg4/2MmOETuGZ9P 8WXTvJbrHOs13qpSQ8ah6DCLGU2P4UaFzyfSn5cfCgUSt9mUE4jgONv8BVuzLYWY/3 TALQu4isgmVMEDMUVflSpKSoKa62REKQamDJ7AKU= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 253483858C51 for ; Thu, 28 Jul 2022 16:26:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 253483858C51 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 26SGQhBH002156 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Jul 2022 12:26:48 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 26SGQhBH002156 Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 1316F1E5EA; Thu, 28 Jul 2022 12:26:43 -0400 (EDT) Message-ID: <6d95a4a8-7651-1ca2-aef8-875dd1911e2c@polymtl.ca> Date: Thu, 28 Jul 2022 12:26:42 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v3 4/7] gdb/dwarf: pass a file_entry to line_header::file_file_name Content-Language: fr To: Bruno Larsen , gdb-patches@sourceware.org References: <20220428033542.1636284-1-simon.marchi@polymtl.ca> <20220428033542.1636284-5-simon.marchi@polymtl.ca> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 28 Jul 2022 16:26:43 +0000 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: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 5/3/22 16:12, Bruno Larsen wrote: > 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 Thanks for the review. Sorry for the delay, I'm just coming back to this patch series. > 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? I don't know, I don't immediately see how that could be done, and it depends what you mean by "handle names and paths". >> 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; Hmm, matter of style I guess. I'm a big fan of early returns, I see it as filtering out cases, and I find it lightens the cognitive load a bit. When I see: if (IS_ABSOLUTE_PATH (fe.name)) return fe.name; I understand immediately that if fe.name is absolute, we return it as is, and the rest of the code only deals with relative paths. So my brain can stop thinking about absolute paths for the rest of the function. Perhaps it would make more sense to swap the last two cases though, like this: if (IS_ABSOLUTE_PATH (fe.name)) return fe.name; const char *dir = fe.include_dir (this); if (dir == nullptr) return fe.name; return path_join (dir, fe.name); Since the "dir == nullptr" condition filters out the unusual / error case of not having a directory. I'll do this change. Simon