Re: [PATCH 1/2] Possible uninitialized 'last' variable in usdt_copyin_data()

[Kris Van Hees <kris.van.hees@oracle.com>]

On Fri, Aug 15, 2025 at 01:12:53PM -0400, Eugene Loh wrote:
> On 8/15/25 11:25, Kris Van Hees wrote:
> 
> > On Tue, Aug 12, 2025 at 06:46:05PM -0400, eugene.loh@oracle.com wrote:
> > > From: Eugene Loh <eugene.loh@oracle.com>
> > > 
> > > Some compilers warn:
> > > 
> > > libcommon/usdt_parser.c: In function ???usdt_copyin_data???:
> > > libcommon/usdt_parser.c:191:15: warning:
> > >     ???last??? may be used uninitialized in this function [-Wmaybe-uninitialized]
> > >      last->next = blk;
> > >      ~~~~~~~~~~~^~~~~
> > > 
> > > Change the "if" check to make it easier for compilers to recognize
> > > that "last" will be initialized (and non-NULL even!).
> > I disagree...  What compiler version reported this as a warning?  The warning
> > shows a limitation of the compiler to see that last can actually never be
> > used uninitialized.
> 
> OL8 with "yum update" then "make".  Looks like
> gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-26.0.1)
> Should I be using a different recipe?

8.5.0 is definitely an old compiler.  So that is not entirely unexpected.

> At least in my opinion, the new code with this patch is simply cleaner.

I guess that will always be subjective...  I really prefer the original code,
because it captures IMHO better that last depends on first, i.e. there is no
concept of a last block until there is at least a first block, since last is
the most recently added block (which cannot exist until there has been a
first).  With the new code, while it avoids a warning on an older compiler,
you make the conditional operate on last, which seems counter-intuitive to me
for a construct where you have a 'first block' and 'more recently added block'.

YMMV

> > I don't think we should make changes like these to accomodate compielrs that
> > are less advanced.  We generally expect systems to be updated to the most
> > recent version of packages so that would include the compiler.
> > 
> > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > > ---
> > >   libcommon/usdt_parser.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libcommon/usdt_parser.c b/libcommon/usdt_parser.c
> > > index 864198098..d8cb9b7ba 100644
> > > --- a/libcommon/usdt_parser.c
> > > +++ b/libcommon/usdt_parser.c
> > > @@ -163,7 +163,7 @@ usdt_destroy_data(usdt_data_t *data)
> > >   usdt_data_t *
> > >   usdt_copyin_data(int in, int out, int *ok)
> > >   {
> > > -	usdt_data_t *first = NULL, *last;
> > > +	usdt_data_t *first = NULL, *last = NULL;
> > >   	size_t cnt;
> > >   	*ok = 1;
> > > @@ -185,7 +185,7 @@ usdt_copyin_data(int in, int out, int *ok)
> > >   		if ((blk = usdt_copyin_block(in, out, ok)) == NULL)
> > >   			goto err;
> > > -		if (first == NULL)
> > > +		if (last == NULL)
> > >   			first = last = blk;
> > >   		else {
> > >   			last->next = blk;
> > > -- 
> > > 2.47.3
> > >