Re: [PATCH] building libtraceevent with clang

[Steven Rostedt]

On Fri, 10 Feb 2017 14:03:17 -0300
Arnaldo Carvalho de Melo <acme@xxxxxxxxxx> wrote:

> Hi Steven,
> 
> 	I tried building perf (and thus libtraceevent) with clang and
> got this one:
> 
> kbuffer-parse.c:312:7: warning: variable 'length' is used uninitialized whenever switch case is taken [-Wsometimes-uninitialized]
>         case OLD_RINGBUF_TYPE_TIME_EXTEND:
>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> kbuffer-parse.c:339:29: note: uninitialized use occurs here
>         kbuf->next = kbuf->index + length;
>                                    ^~~~~~
> kbuffer-parse.c:297:21: note: initialize the variable 'length' to silence this warning
>         unsigned int length;
>                            ^
>                             = 0
> 1 warning generated.

Hmm, I'm surprised gcc never complained.

> 
> 
> Please take a look if the following is what should be done:
> 
> diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> index 65984f1c2974..3f717294cb82 100644
> --- a/tools/lib/traceevent/kbuffer-parse.c
> +++ b/tools/lib/traceevent/kbuffer-parse.c
> @@ -309,20 +309,20 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
>  		kbuf->next = kbuf->size;
>  		return 0;
>  
> -	case OLD_RINGBUF_TYPE_TIME_EXTEND:
> -		extend = read_4(kbuf, ptr);
> -		extend <<= TS_SHIFT;
> -		extend += delta;
> -		delta = extend;
> -		ptr += 4;
> -		break;
> -
>  	case OLD_RINGBUF_TYPE_TIME_STAMP:
>  		/* should never happen! */
>  		kbuf->curr = kbuf->size;
>  		kbuf->next = kbuf->size;
>  		kbuf->index = kbuf->size;
>  		return -1;
> +
> +	case OLD_RINGBUF_TYPE_TIME_EXTEND:
> +		extend = read_4(kbuf, ptr);
> +		extend <<= TS_SHIFT;
> +		extend += delta;
> +		delta = extend;
> +		ptr += 4;
> +		/* Fall through */

No, actually, the length should be zeroed here.

	length = 0;

As the ptr is placed passed the metadata. At the end of this case
statement, the ptr will be after the metadata. The length is
to represent the length of the data that is part of the event. The
index is the ptr - start_data + length. As there's no data to this type
of event, it should be zero.

When I add length = 0, I get more events from my old 2.6.30 trace.dat
file :-/  I'll have to update my tests.

Care to send another patch?

-- Steve


>  	default:
>  		if (len)
>  			length = len * 4;