libctf warnings

[Nick Alcock]

On 10 Apr 2024, Alan Modra spake thusly:

> 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.)

Oh, maybe I should add that to my test vector :) if this happens on
non-prehistoric compilers, by all means fix it!

>> 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.

Yes, but do we care about *warnings* there, particularly when they're
fps, as opposed to actual problems? As I see it the purpose of a warning
is to highlight real problems: if the warning is false, and only happens
on ancient compilers, then quiescing it is only worthwhile if it doesn't
increase the risk of missing real problems in future. (But if it happens
on newer compilers too, that's different.)

>> > 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.

Hm. Is there some way to *detect* -fno-inlines? Because I'm quite happy
to define away ctf_assert() to nothing when that flag is in use, if that
would help. (Alas, I can't immediately see a useful predefined
preprocessor flag.)

>> > 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.

That's OK then, initialize away.

>> > 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.

If the only difference is ordering, go for it (on re-examination it
looks like you're right). Tab-based indentation makes this very hard to
read :(

>> >  /* < 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?

That sounds like a good idea.

-- 
NULL && (void)