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

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

[eugene.loh]

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

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

[PATCH 2/2] Use a consistent type for dtrace_consume()

[eugene.loh]

From: Eugene Loh <eugene.loh@oracle.com>

For a long time, the type of dtrace_consume() has been
- int                 according to libdtrace/dtrace.h
- dtrace_workstatus_t according to libdtrace/dt_consume.c

With some recent compilers, however, this triggers the warning (redacted
here):

    libdtrace/dt_consume.c:3041:1: warning: conflicting types
    for ‘dtrace_consume’ due to enum/integer mismatch;
    have ‘dtrace_workstatus_t(...)’ [-Wenum-int-mismatch]
     3041 | dtrace_consume(...)
          | ^~~~~~~~~~~~~~
    In file included from libdtrace/dt_impl.h:14,
                     from libdtrace/dt_consume.c:16:
    libdtrace/dtrace.h:210:12: note: previous declaration of
    ‘dtrace_consume’ with type ‘int(...)’
      210 | extern int dtrace_consume(...)
          |            ^~~~~~~~~~~~~~

which is a nuisance.

Note that dtrace_consume() is called from only one site, where its
value is compared to DTRACE_WORKSTATUS_ERROR, which is an argument
for the dtrace_workstatus_t type.

On the other hand, dtrace_consume() is defined to return a variety
of values, like 0, dt_set_errno(), and dt_consume_cpu(), all of which
are int, but also DTRACE_WORKSTATUS_OKAY, DTRACE_WORKSTATUS_ERROR,
and rval, all of which are dtrace_workstatus_t.  But then rval itself
is set to dtrace_workstatus_t dt_consume_begin() or int
dt_consume_cpu().  So, there is simply no consistency here.

Having the prototype be dtrace_workstatus_t requires some amount
of code refactoring.

Just change the definition to int and clean up the warning.

Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
---
 libdtrace/dt_consume.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
index 07b19d498..d9be563d9 100644
--- a/libdtrace/dt_consume.c
+++ b/libdtrace/dt_consume.c
@@ -3037,7 +3037,7 @@ dt_consume_fini(dtrace_hdl_t *dtp)
 	dt_htab_destroy(dtp->dt_spec_bufs);
 }
 
-dtrace_workstatus_t
+int
 dtrace_consume(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pf,
 	       dtrace_consume_rec_f *rf, void *arg)
 {
-- 
2.47.3

Re: [DTrace-devel] [PATCH 2/2] Use a consistent type for dtrace_consume()

[Kris Van Hees]

On Tue, Aug 12, 2025 at 06:46:06PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
> 
> For a long time, the type of dtrace_consume() has been
> - int                 according to libdtrace/dtrace.h
> - dtrace_workstatus_t according to libdtrace/dt_consume.c
> 
> With some recent compilers, however, this triggers the warning (redacted
> here):
> 
>     libdtrace/dt_consume.c:3041:1: warning: conflicting types
>     for ???dtrace_consume??? due to enum/integer mismatch;
>     have ???dtrace_workstatus_t(...)??? [-Wenum-int-mismatch]
>      3041 | dtrace_consume(...)
>           | ^~~~~~~~~~~~~~
>     In file included from libdtrace/dt_impl.h:14,
>                      from libdtrace/dt_consume.c:16:
>     libdtrace/dtrace.h:210:12: note: previous declaration of
>     ???dtrace_consume??? with type ???int(...)???
>       210 | extern int dtrace_consume(...)
>           |            ^~~~~~~~~~~~~~
> 
> which is a nuisance.
> 
> Note that dtrace_consume() is called from only one site, where its
> value is compared to DTRACE_WORKSTATUS_ERROR, which is an argument
> for the dtrace_workstatus_t type.
> 
> On the other hand, dtrace_consume() is defined to return a variety
> of values, like 0, dt_set_errno(), and dt_consume_cpu(), all of which
> are int, but also DTRACE_WORKSTATUS_OKAY, DTRACE_WORKSTATUS_ERROR,
> and rval, all of which are dtrace_workstatus_t.  But then rval itself
> is set to dtrace_workstatus_t dt_consume_begin() or int
> dt_consume_cpu().  So, there is simply no consistency here.
> 
> Having the prototype be dtrace_workstatus_t requires some amount
> of code refactoring.
> 
> Just change the definition to int and clean up the warning.
> 
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

... I admit this was my fault.  I tried to refactor the code so that we could
    have dtrace_consume() return dtrace_workstatus_t, but only got this far.
    I agree that for now, changing the return type of dtrace_consume() is the
    best action to get rid of the compiler warning.  Maybe we can revisit the
    actual refactoring work later to clean this up properly.

> ---
>  libdtrace/dt_consume.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> index 07b19d498..d9be563d9 100644
> --- a/libdtrace/dt_consume.c
> +++ b/libdtrace/dt_consume.c
> @@ -3037,7 +3037,7 @@ dt_consume_fini(dtrace_hdl_t *dtp)
>  	dt_htab_destroy(dtp->dt_spec_bufs);
>  }
>  
> -dtrace_workstatus_t
> +int
>  dtrace_consume(dtrace_hdl_t *dtp, FILE *fp, dtrace_consume_probe_f *pf,
>  	       dtrace_consume_rec_f *rf, void *arg)
>  {
> -- 
> 2.47.3
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel

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

[Kris Van Hees]

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.

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
> 

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

[Eugene Loh]

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?

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

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

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

[Kris Van Hees]

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