Re: libctf warnings

[Alan Modra <amodra@gmail.com>]

On Tue, Apr 09, 2024 at 08:07:57PM +0100, Nick Alcock wrote:
> On 9 Apr 2024, Alan Modra said:
> 
> > Seen with every compiler I have:
> > home/alan/src/binutils-gdb/libctf/ctf-create.c: In function ‘ctf_add_encoded’:
> > /home/alan/src/binutils-gdb/libctf/ctf-create.c:555:3: warning: ‘encoding’ may be used uninitialized [-Wmaybe-uninitialized]
> >   555 |   memcpy (dtd->dtd_vlen, &encoding, sizeof (encoding));
> 
> Never seen this one. Does it apply to prehistoric compilers only?

No, but I neglected to mention this was bulding with -O1 -fno-inlines.
To be honest, I'd forgotten that was the way I was building..
(-O1 -fno-inlines can help quite a bit when debugging.)

> *waves objection flag* though honestly the only bit that's really

Heh.

> objectionable is the bit that's actually broken, below (the rest is just
> me wondering why on earth anyone would care about warnings seen only
> with decade-old compilers).

Because people build binutils on older systems with decades-old
compilers.

> 
> > diff --git a/libctf/ctf-create.c b/libctf/ctf-create.c
> > index d0255e5ba7f..99ff4882fd9 100644
> > --- a/libctf/ctf-create.c
> > +++ b/libctf/ctf-create.c
> > @@ -551,6 +551,10 @@ ctf_add_encoded (ctf_dict_t *fp, uint32_t flag,
> >      case CTF_K_FLOAT:
> >        encoding = CTF_FP_DATA (ep->cte_format, ep->cte_offset, ep->cte_bits);
> >        break;
> > +    default:
> > +      /* ctf_assert is opaque to compilers.  This dead code avoids a
> > +	 warning about "encoding" being used uninitialized.  */
> 
> Hmm. Do you know of any way to make it *less* opaque? :/

Um, my comment is wrong.  ctf_assert is only opague with -fno-inlines.
I'd forgotten I was using that option.  Sorry.

> > diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
> > index d5433b9d9bd..360bc1a0e63 100644
> > --- a/libctf/ctf-link.c
> > +++ b/libctf/ctf-link.c
> > @@ -762,7 +762,7 @@ ctf_link_deduplicating_open_inputs (ctf_dict_t *fp, ctf_dynhash_t *cu_names,
> >        ctf_link_input_t *one_input;
> >        ctf_dict_t *one_fp;
> >        ctf_dict_t *parent_fp = NULL;
> > -      uint32_t parent_i;
> > +      uint32_t parent_i = 0;
> >        ctf_next_t *j = NULL;
> >        /* If we are processing CU names, get the real input.  All the inputs
> 
> Again, if this is only seen with GCC 4.5, I'm tempted to just ignore
> warnings from a compiler this prehistoric.

It also might be seen with newer compilers and some optimisation
options other than plain -O2.

> > diff --git a/libctf/ctf-serialize.c b/libctf/ctf-serialize.c
> > index 8645f32ab20..11cbe75601e 100644
> > --- a/libctf/ctf-serialize.c
> > +++ b/libctf/ctf-serialize.c
> > @@ -202,17 +202,15 @@ symtypetab_density (ctf_dict_t *fp, ctf_dict_t *symfp, ctf_dynhash_t *symhash,
> >  	    }
> >  
> >  	  ctf_dynhash_remove (linker_known, name);
> > -	}
> > -      *unpadsize += sizeof (uint32_t);
> > -      (*count)++;
> >  
> > -      if (!(flags & CTF_SYMTYPETAB_FORCE_INDEXED))
> > -	{
> >  	  if (*max < sym->st_symidx)
> >  	    *max = sym->st_symidx;
> >  	}
> >        else
> >  	(*max)++;
> > +
> > +      *unpadsize += sizeof (uint32_t);
> > +      (*count)++;
> 
> I think this is a change in semantics. (That (*max++) branch is no
> longer executed when sym->st_type != STT_FUNC, whether or not (flags &
> CTF_SYMTYPETAB_EMIT_FUNCTION) is turned on.

I think you may be reading the patch wrong.  The "else" now belongs to
the earlier "if (!(flags & CTF_SYMTYPETAB_FORCE_INDEXED))".  The only
semantic change is that *max now changes before *unpadsize and *count.

> 
> I think just initializing sym to NULL to quiet the warning would be
> safer.
> 
> > diff --git a/libctf/swap.h b/libctf/swap.h
> > index d004dc14348..2dd20a27390 100644
> > --- a/libctf/swap.h
> > +++ b/libctf/swap.h
> > @@ -67,6 +67,7 @@ bswap_64 (uint64_t v)
> >  /* < C11? define away static assertions.  */
> >  
> >  #if !defined (__STDC_VERSION__) || __STDC_VERSION__ < 201112L
> > +#undef _Static_assert
> >  #define _Static_assert(cond, err)
> >  #endif
> 
> I do wish there was some way to say 'use the compiler's _Static_assert'
> if available', but there's no requirement it's a macro...

Right, but in the case I hit, it *is* a macro.  Would you prefer
#ifndef instead?

-- 
Alan Modra
Australia Development Lab, IBM