From patchwork Tue Jun 19 15:21:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 27926 Received: (qmail 122749 invoked by alias); 19 Jun 2018 15:22:05 -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 122182 invoked by uid 89); 19 Jun 2018 15:22:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=itd, it'd, cxx, TEXT X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Jun 2018 15:22:03 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A38EA7DAC3 for ; Tue, 19 Jun 2018 15:22:01 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8459510C1E; Tue, 19 Jun 2018 15:21:59 +0000 (UTC) Subject: Re: [PATCH] Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section To: Sergio Durigan Junior , GDB Patches References: <20180325191943.8246-12-palves@redhat.com> <20180618202634.10452-1-sergiodj@redhat.com> From: Pedro Alves Message-ID: <73d51f47-0f68-178f-3a5d-e842a2fa240b@redhat.com> Date: Tue, 19 Jun 2018 16:21:58 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180618202634.10452-1-sergiodj@redhat.com> On 06/18/2018 09:26 PM, Sergio Durigan Junior wrote: > Commit 20944a6e20324cd897bf6c4c5fd20ef7224dacaa ("Fix stepping past > GNU ifunc resolvers (introduce lookup_msym_prefer)") introduced a new > way to determine the 'want_type' variable on > minsyms.c:lookup_minimal_symbol_by_pc_section. Because the > 'lookup_msym_prefer' has a default value, we know that 'want_type' > will always be initialized. Sorry, but that doesn't follow. It's the 'prefer' parameter that will always be initialized to something with the default value, not the 'want_type' local. struct bound_minimal_symbol lookup_minimal_symbol_by_pc_section (CORE_ADDR, struct obj_section *, lookup_msym_prefer prefer = lookup_msym_prefer::TEXT); vs: enum minimal_symbol_type want_type; But even that is not necessarily true, since the caller may well pass down an explicit argument, which in turn was uninitialized, like: lookup_msym_prefer bogus; lookup_minimal_symbol_by_pc_section (addr, NULL, bogus); > However, GCC is complaining that the > variable can be used uninitialized in the function: > > ../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)': > ../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized] > && MSYMBOL_TYPE (&msymbol[hi]) != want_type > > (This is with gcc-8.1.1-1.fc29.x86_64). > > This patch fixes it by initializing 'want_type' with 'mst_text', which > is the same default value that is passed in the 'lookup_msym_prefer' > variable. But it's not, as explained above. The reason this warning is a false positive is that we know that 'want_type' is always going to be initialized because the switch to converts enum lookup_msym_prefer values to enum minimal_symbol_type values: switch (prefer) { case lookup_msym_prefer::TEXT: want_type = mst_text; break; case lookup_msym_prefer::TRAMPOLINE: want_type = mst_solib_trampoline; break; case lookup_msym_prefer::GNU_IFUNC: want_type = mst_text_gnu_ifunc; break; } has a case for every possible enumerator. The actual problem is that GCC assumes that enum variables may hold values other than the named enumerators, like e.g., "lookup_msym_prefer prefer = (lookup_msym_prefer) 10;". We know that this isn't something we want to support with these enum types, so it's better to assert that we never see a value not covered by the enumerators. The simplest is to add a "default:" case with a gdb_assert, but when I wrote that code, I had not done that on purpose, thinking that I'd prefer it if we enabled "-Wswitch" in gdb, which helps find switch/cases where we need to handle a new enumerator, whenever we add one, like this: CXX minsyms.o src/gdb/minsyms.c: In function ‘bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)’: src/gdb/minsyms.c:695:10: error: enumeration value ‘NEW_KIND’ not handled in switch [-Werror=switch] switch (prefer) ^ I think that it's preferable, if reasonable, to rework the code a bit to make it more explicit to the compiler that a variable is always initialized instead of initializing variables to quiet -Wmaybe-uninitialized, which is documented as spewing false positives. (IMO it'd be reasonable if GCC moved that warning from -Wall to -Wextra.) Thus I'd prefer this instead: From d66e808818ece92d6d79bfdb4e0a421b30662dd3 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 18 Jun 2018 22:36:43 +0100 Subject: [PATCH] Silence -Wmaybe-uninitialized warning in minsyms.c:lookup_minimal_symbol_by_pc_section Compiling with GCC 8.1 shows this warning: ../../gdb/minsyms.c: In function 'bound_minimal_symbol lookup_minimal_symbol_by_pc_section(CORE_ADDR, obj_section*, lookup_msym_prefer)': ../../gdb/minsyms.c:825:40: warning: 'want_type' may be used uninitialized in this function [-Wmaybe-uninitialized] && MSYMBOL_TYPE (&msymbol[hi]) != want_type That warning is a false positive, because the switch that converts enum lookup_msym_prefer values to enum enum minimal_symbol_type values has a case for every lookup_msym_prefer enumerator: switch (prefer) { case lookup_msym_prefer::TEXT: want_type = mst_text; break; case lookup_msym_prefer::TRAMPOLINE: want_type = mst_solib_trampoline; break; case lookup_msym_prefer::GNU_IFUNC: want_type = mst_text_gnu_ifunc; break; } The problem is that GCC assumes that enum variables may hold values other than the named enumerators (like e.g., "lookup_msym_prefer prefer = (lookup_msym_prefer) 10;"). Rework the code a bit, adding a gdb_assert to make it explicit to the compiler that want_type is initialized in all normal-return paths. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * minsyms.c (msym_prefer_to_msym_type): New, factored out from ... (lookup_minimal_symbol_by_pc_section)... here. --- gdb/minsyms.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/gdb/minsyms.c b/gdb/minsyms.c index 4882e58ee4..4409e6f8b3 100644 --- a/gdb/minsyms.c +++ b/gdb/minsyms.c @@ -656,6 +656,27 @@ frob_address (struct objfile *objfile, CORE_ADDR *pc) return 0; } +/* Helper for lookup_minimal_symbol_by_pc_section. Convert a + lookup_msym_prefer to a minimal_symbol_type. */ + +static minimal_symbol_type +msym_prefer_to_msym_type (lookup_msym_prefer prefer) +{ + switch (prefer) + { + case lookup_msym_prefer::TEXT: + return mst_text; + case lookup_msym_prefer::TRAMPOLINE: + return mst_solib_trampoline; + case lookup_msym_prefer::GNU_IFUNC: + return mst_text_gnu_ifunc; + } + + /* Assert here instead of in a default switch case above so that + -Wswitch warns if a new enumerator is added. */ + gdb_assert_not_reached ("unhandled lookup_msym_prefer"); +} + /* Search through the minimal symbol table for each objfile and find the symbol whose address is the largest address that is still less than or equal to PC, and matches SECTION (which is not NULL). @@ -683,7 +704,6 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio struct minimal_symbol *best_symbol = NULL; struct objfile *best_objfile = NULL; struct bound_minimal_symbol result; - enum minimal_symbol_type want_type; if (section == NULL) { @@ -692,18 +712,7 @@ lookup_minimal_symbol_by_pc_section (CORE_ADDR pc_in, struct obj_section *sectio return {}; } - switch (prefer) - { - case lookup_msym_prefer::TEXT: - want_type = mst_text; - break; - case lookup_msym_prefer::TRAMPOLINE: - want_type = mst_solib_trampoline; - break; - case lookup_msym_prefer::GNU_IFUNC: - want_type = mst_text_gnu_ifunc; - break; - } + minimal_symbol_type want_type = msym_prefer_to_msym_type (prefer); /* We can not require the symbol found to be in section, because e.g. IRIX 6.5 mdebug relies on this code returning an absolute