Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section

From: Pedro Alves <palves@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 <palves@redhat.com>
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  <palves@redhat.com>

	* 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(-)

Silence GCC "uninitialized" warning on minsyms.c:lookup_minimal_symbol_by_pc_section

Commit Message

Comments

Patch