From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id BlsrHMnL4mJpERwAWB0awg (envelope-from ) for ; Thu, 28 Jul 2022 13:47:53 -0400 Received: by simark.ca (Postfix, from userid 112) id 65F861EA03; Thu, 28 Jul 2022 13:47:53 -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=s00DXbWj; 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 E02AF1E9EB for ; Thu, 28 Jul 2022 13:47:52 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 595F53856257 for ; Thu, 28 Jul 2022 17:47:52 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 595F53856257 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1659030472; bh=nSZSHRtCE2yUYd+SmVgVFymXQeDq0vNvt1pPcPYeyxw=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=s00DXbWjEivga/MfWG2kXT89dmHiH10KqHgtkilm0INyQBK8tZUtORWvZwMY7nUi9 flKX5AJFzWvk5llJurtPrDZRdOgwIVpswvw+G0WF9Ymgjt2p/JcPf7QBhptynTKZb6 CSTvCtzRZ9PxiK+Gv3SHlxVS0hHO+HjaVlk5Lqug= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id CD6A73857B9F for ; Thu, 28 Jul 2022 17:47:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CD6A73857B9F 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 26SHkOfv013757 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Jul 2022 13:46:29 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 26SHkOfv013757 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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 606AC1E87E; Thu, 28 Jul 2022 13:46:24 -0400 (EDT) Message-ID: <9c6b5e6e-c164-a01f-13d3-4081dcdabb95@polymtl.ca> Date: Thu, 28 Jul 2022 13:46:23 -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 5/7] gdb: add "id" fields to identify symtabs and subfiles Content-Language: fr To: Lancelot SIX References: <20220428033542.1636284-1-simon.marchi@polymtl.ca> <20220428033542.1636284-6-simon.marchi@polymtl.ca> <20220428235314.elsyab5pk5bufivr@ubuntu.lan> In-Reply-To: <20220428235314.elsyab5pk5bufivr@ubuntu.lan> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 28 Jul 2022 17:46:24 +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 Cc: gdb-patches@sourceware.org Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 4/28/22 19:53, Lancelot SIX wrote: > Hi, > > Just some nits below. > > When applying the patch I have: > > Applying: gdb: add "id" fields to identify symtabs and subfiles > .git/rebase-apply/patch:153: trailing whitespace. > It is used to look up existing subfiles in calls to start_subfile. */ > warning: 1 line adds whitespace errors. Fixed. >> Another this to consider is that while the main symtab's name (or > > s/this/thing/ ? Fixed. >> Changes in the core symtab files are: >> >> - Add "name_for_id" and "filename_for_id" fields to "struct subfile" >> and "struct symta"b, next to existing "name" and "filename" fields. > ^ > Two chars are swapped here. Fixed. >> - Make buildsym_compunit::buildsym_compunit and >> buildsym_compunit::start_subfile accept a "name_for_id" parameter >> next to the existing "name" ones. >> - Make buildsym_compunit::start_subfile used "name_for_id" for looking >> up existing subfiles. This is the key thing for making calls >> to start_subfile for the main source file look up the existing >> subfile successfully, and avoid relying on >> watch_main_source_file_lossage. >> - Make sal_macro_scope pass "filename_for_id", rather than "filename", >> to macro_lookup_inclusion. This is the key thing to making the >> lookup work and macro printing work. >> >> Changes in the DWARF files are: >> >> - Make line_header::file_file_name return the "most complete possible" >> name. The only pre-existing user of this method is the macro code, >> to give the macro_source_file objects their name. And we now want >> them to have this "most complete possible" name, which will match the >> corresponding symtab's "filename_for_id". >> - Make dwarf2_cu::start_compunit_symtab pass the "most complete >> possible" name for the main symtab's "filename_for_id". In this >> context, where the info comes from the compilation unit's DW_AT_name >> / DW_AT_comp_dir, it means prepending DW_AT_comp_dir to DW_AT_name if >> DW_AT_name is not already absolute. >> - Change dwarf2_start_subfile to build a name_for_id for the subfile >> being started. The simplest way is to re-use >> line_header::file_file_name, since the callers always have a >> file_entry handy. This ensures that it will get the exact same path >> representation as the macro code does, for the same file (since it >> also uses line_header::file_file_name). >> - Update calls to allocate_symtab to pass the "name_for_id" from the >> subfile. >> >> The rest of the changes are to update the other symtab users (jit, >> ctfread, mdebugread, xcoffread). For those, the same value is passed >> for the "id" as the for the filename, so they should keep the same >> behavior they have today. > > I guess this is just personal preference, but I would have chosen to > add overloads so those files do not have to change / bother about an > argument which is meaningless for them. > > Something like: > > // gdb/buildsym.h > struct buildsym_compunit > { > buildsym_compunit (struct objfile *objfile_, const char *name, > const char *comp_dir_, enum language language_, > CORE_ADDR last_addr) > : buildsym_compunit (objfile_, name, comp_dir_, name, > language_, last_addr); > buildsym_compunit (struct objfile *objfile_, const char *name, > const char *comp_dir_, const char *name_for_id, > language language_, CORE_ADDR last_addr); > > ... > > void start_subfile (const char *name, const char *name_for_id); > > void start_subfile (const char *name) > { start_subfile (name, name); } > }; > > // gdb/symfile.h > extern struct symtab *allocate_symtab > (struct compunit_symtab *cust, const char *filename, const char *id)) > ATTRIBUTE_NONNULL (1); > > static inline struct symtab * > allocate_symtab (struct compunit_symtab *cust, const char *filename) > { > return allocate_sumtab (cust, filename, filename); > } > > In the end it does not make much difference, your approach work also > perfectly fine so feel free to keep it the way it is. I did that. > > Otherwise, FWIW I find the approach to keep one string representation of > the filename for display and one for some sort of canonicalization > perfectly appropriate. Ok, thanks for the review. >> Similary, with DWARF 5: > > s/Similary/Similarly/ Fixed. Simon