From patchwork Thu Nov 7 14:27:11 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Simon Marchi (Code Review)" X-Patchwork-Id: 35725 Received: (qmail 81686 invoked by alias); 7 Nov 2019 14:27:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 81672 invoked by uid 89); 7 Nov 2019 14:27:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN autolearn=ham version=3.3.1 spammy= X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 07 Nov 2019 14:27:17 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id E57EE2058E; Thu, 7 Nov 2019 09:27:14 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id E8DE9200EF; Thu, 7 Nov 2019 09:27:11 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id C284925B28; Thu, 7 Nov 2019 09:27:11 -0500 (EST) X-Gerrit-PatchSet: 3 Date: Thu, 7 Nov 2019 09:27:11 -0500 From: "Tom de Vries (Code Review)" To: gdb-patches@sourceware.org Cc: Joel Brobecker , Tom Tromey , Luis Machado , Andrew Burgess Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review v3] [gdb/symtab] Prefer var def over decl X-Gerrit-Change-Id: Id92326cb8ef9903b121ef9e320658eb565d0f5a9 X-Gerrit-Change-Number: 29 X-Gerrit-ChangeURL: X-Gerrit-Commit: cede0513116b126cb1ef7506cd1ff25eb86cf878 In-Reply-To: References: X-Gerrit-Comment-Date: Thu, 7 Nov 2019 09:27:11 -0500 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Message-Id: <20191107142711.C284925B28@gnutoolchain-gerrit.osci.io> Tom de Vries has posted comments on this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29 ...................................................................... Patch Set 3: > Patch Set 3: > > Thank you for doing this. > > The patch itself seems perfectly fine to me. I appreciated the comment > and the test case -- those help a lot. > > However, I wonder why gdb is even making a symbol for a declaration in the > first place. For some other kinds of declarations, gdb doesn't bother, so > I was wondering if it's possible to simply ignore them in general. In general, there's a use of adding variable declarations to the symbol table. Say we have a declaration of a variable in one file, and the definition in another. If the file with the definition is build without debug info, but the file with the declaration with debug info, we can combine the debuginfo of the declaration and the minimal symbol info of the definition to print the value of the variable using the proper type. But in the case of this PR, the size of the declaration is zero, and that currently means that this information composition doesn't work. That could be fixed by using the size of the symbol from the minimal symbol info (that's PR24989). As for the question whether we can ignore the declaration, I've tried this approach (let's call it ignore.patch). Ignore.patch: ... ... It passes regression testing, and fixes the original test-case for this PR (PR24971). Ignore.patch does not fix the test-case added in the current patch set, but that one would have to be adapted for this fix (this fix works when type size is zero, and that's not the case for that test-case). Also, ignore.patch fixes the test-case of the spinoff PR24985. OTOH, if we go with ignore.patch, it may have to be reverted again in order to trigger the (currently non-existing) fix for PR24989. So AFAIU: - if we fix this PR using ignore.patch and then revert it in order to fix PR24972, the test-case for this PR is handled by looking up the symbol size in the minimal symbol info - if we fix this PR using the currently proposed patch set, and then fix PR24972, the test-case for this PR is still handled by finding the definition instead of the declaration. diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 0a7a0334202..381bd45e805 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -21830,6 +21830,10 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu, else if (attr2 && (DW_UNSND (attr2) != 0) && dwarf2_attr (die, DW_AT_type, cu) != NULL) { + if (die->tag == DW_TAG_variable && die_is_declaration (die, cu) + && TYPE_LENGTH (SYMBOL_TYPE (sym)) == 0) + suppress_add = 1; + /* A variable with DW_AT_external is never static, but it may be block-scoped. */ list_to_add