linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] building libtraceevent with clang
@ 2017-02-10 17:03 Arnaldo Carvalho de Melo
  2017-02-10 19:14 ` Steven Rostedt
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-02-10 17:03 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

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.


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 */
 	default:
 		if (len)
 			length = len * 4;

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-10 17:03 [PATCH] building libtraceevent with clang Arnaldo Carvalho de Melo
@ 2017-02-10 19:14 ` Steven Rostedt
  2017-02-13 16:24   ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2017-02-10 19:14 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

On Fri, 10 Feb 2017 14:03:17 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> 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;

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-10 19:14 ` Steven Rostedt
@ 2017-02-13 16:24   ` Arnaldo Carvalho de Melo
  2017-02-13 16:26     ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-02-13 16:24 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

Em Fri, Feb 10, 2017 at 02:14:08PM -0500, Steven Rostedt escreveu:
> On Fri, 10 Feb 2017 14:03:17 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > +	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?

I have this now in my tree, and since the patch is basically yours, I
just made myself the reporter and you the author, please ack.


commit 1fb82b6cba967afde11957099a9ef440cf8c2248
Author: Steven Rostedt <rostedt@goodmis.org>
Date:   Mon Feb 13 13:18:29 2017 -0300

    tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP
    
    A undefined value was being used for the OLD_RING_BUFFER_TYPE_TIME_STAMP
    case entry, as the 'lenght' variable was not being initialized, fix it.
    
    Caught by the reporter when building tools/perf/ using clang, which emmitted
    this warning:
    
      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
    
    Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Wang Nan <wangnan0@huawei.com>
    Link: http://lkml.kernel.org/r/20170210141408.5eeb6e91@gandalf.local.home
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
index 65984f1c2974..2009cb7d9675 100644
--- a/tools/lib/traceevent/kbuffer-parse.c
+++ b/tools/lib/traceevent/kbuffer-parse.c
@@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
 		extend += delta;
 		delta = extend;
 		ptr += 4;
+		lenght = 0;
 		break;
 
 	case OLD_RINGBUF_TYPE_TIME_STAMP:

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-13 16:24   ` Arnaldo Carvalho de Melo
@ 2017-02-13 16:26     ` Arnaldo Carvalho de Melo
  2017-02-13 17:14       ` Steven Rostedt
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-02-13 16:26 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

Em Mon, Feb 13, 2017 at 01:24:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Feb 10, 2017 at 02:14:08PM -0500, Steven Rostedt escreveu:
> > On Fri, 10 Feb 2017 14:03:17 -0300
> > Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > +	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?
> 
> I have this now in my tree, and since the patch is basically yours, I
> just made myself the reporter and you the author, please ack.
> 
> 
> commit 1fb82b6cba967afde11957099a9ef440cf8c2248
> Author: Steven Rostedt <rostedt@goodmis.org>
> Date:   Mon Feb 13 13:18:29 2017 -0300
> 
>     tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP
>     
>     A undefined value was being used for the OLD_RING_BUFFER_TYPE_TIME_STAMP
>     case entry, as the 'lenght' variable was not being initialized, fix it.
>     
>     Caught by the reporter when building tools/perf/ using clang, which emmitted
>     this warning:
>     
>       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
>     
>     Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: David Ahern <dsahern@gmail.com>
>     Cc: Jiri Olsa <jolsa@kernel.org>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Wang Nan <wangnan0@huawei.com>
>     Link: http://lkml.kernel.org/r/20170210141408.5eeb6e91@gandalf.local.home
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> index 65984f1c2974..2009cb7d9675 100644
> --- a/tools/lib/traceevent/kbuffer-parse.c
> +++ b/tools/lib/traceevent/kbuffer-parse.c
> @@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
>  		extend += delta;
>  		delta = extend;
>  		ptr += 4;
> +		lenght = 0;

ouch, 'length' :-)

clang provides a really nice error message:

kbuffer-parse.c:318:3: error: use of undeclared identifier 'lenght'; did you mean 'length'?
                lenght = 0;
                ^~~~~~
                length
kbuffer-parse.c:297:15: note: 'length' declared here
        unsigned int length;
                     ^
1 error generated.

I only had to test compile it :-)

>  		break;
>  
>  	case OLD_RINGBUF_TYPE_TIME_STAMP:
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-13 16:26     ` Arnaldo Carvalho de Melo
@ 2017-02-13 17:14       ` Steven Rostedt
  2017-02-13 17:20         ` Arnaldo Carvalho de Melo
  2017-02-14  6:40         ` [tip:perf/core] tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP tip-bot for Steven Rostedt (VMware)
  0 siblings, 2 replies; 10+ messages in thread
From: Steven Rostedt @ 2017-02-13 17:14 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

On Mon, 13 Feb 2017 13:26:22 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:


> > diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> > index 65984f1c2974..2009cb7d9675 100644
> > --- a/tools/lib/traceevent/kbuffer-parse.c
> > +++ b/tools/lib/traceevent/kbuffer-parse.c
> > @@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
> >  		extend += delta;
> >  		delta = extend;
> >  		ptr += 4;
> > +		lenght = 0;  
> 
> ouch, 'length' :-)
> 
> clang provides a really nice error message:
> 
> kbuffer-parse.c:318:3: error: use of undeclared identifier 'lenght'; did you mean 'length'?
>                 lenght = 0;
>                 ^~~~~~
>                 length
> kbuffer-parse.c:297:15: note: 'length' declared here
>         unsigned int length;
>                      ^
> 1 error generated.
> 
> I only had to test compile it :-)
> 
> >  		break;
> >  
> >  	case OLD_RINGBUF_TYPE_TIME_STAMP:
> >   

OK, I pulled the patch into git, and made it official ;-)
I also fixed the "lenght" in the change log too.

Could you use this instead.

-- Steve


>From 4f060a0577156acfdc0524a634f4afcc1657c929 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Mon, 13 Feb 2017 12:11:44 -0500
Subject: [PATCH] tools lib traceevent: Initialize lenght on
 OLD_RING_BUFFER_TYPE_TIME_STAMP

A undefined value was being used for the OLD_RING_BUFFER_TYPE_TIME_STAMP
case entry, as the 'length' variable was not being initialized, fix it.

Caught by the reporter when building tools/perf/ using clang, which emmitted
this warning:

  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

Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170210141408.5eeb6e91@gandalf.local.home
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
index 65984f1..c94e364 100644
--- a/tools/lib/traceevent/kbuffer-parse.c
+++ b/tools/lib/traceevent/kbuffer-parse.c
@@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
 		extend += delta;
 		delta = extend;
 		ptr += 4;
+		length = 0;
 		break;
 
 	case OLD_RINGBUF_TYPE_TIME_STAMP:
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-13 17:14       ` Steven Rostedt
@ 2017-02-13 17:20         ` Arnaldo Carvalho de Melo
  2017-02-13 17:46           ` Steven Rostedt
  2017-02-17  2:00           ` Steven Rostedt
  2017-02-14  6:40         ` [tip:perf/core] tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP tip-bot for Steven Rostedt (VMware)
  1 sibling, 2 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-02-13 17:20 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

Em Mon, Feb 13, 2017 at 12:14:18PM -0500, Steven Rostedt escreveu:
> On Mon, 13 Feb 2017 13:26:22 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> 
> 
> > > diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> > > index 65984f1c2974..2009cb7d9675 100644
> > > --- a/tools/lib/traceevent/kbuffer-parse.c
> > > +++ b/tools/lib/traceevent/kbuffer-parse.c
> > > @@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
> > >  		extend += delta;
> > >  		delta = extend;
> > >  		ptr += 4;
> > > +		lenght = 0;  
> > 
> > ouch, 'length' :-)
> > 
> > clang provides a really nice error message:
> > 
> > kbuffer-parse.c:318:3: error: use of undeclared identifier 'lenght'; did you mean 'length'?
> >                 lenght = 0;
> >                 ^~~~~~
> >                 length
> > kbuffer-parse.c:297:15: note: 'length' declared here
> >         unsigned int length;
> >                      ^
> > 1 error generated.
> > 
> > I only had to test compile it :-)
> > 
> > >  		break;
> > >  
> > >  	case OLD_RINGBUF_TYPE_TIME_STAMP:
> > >   
> 
> OK, I pulled the patch into git, and made it official ;-)
> I also fixed the "lenght" in the change log too.
> 
> Could you use this instead.

Sure, now take a look at this another one:

commit 6401e4361df183bd9953dce56f7c51d8ef28b11e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon Feb 13 13:33:57 2017 -0300

    tools lib traceevent plugin function: Initialize 'index' variable
    
    Detected with clang:
    
        CC       /tmp/build/perf/plugin_function.o
      plugin_function.c:145:6: warning: variable 'index' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
              if (parent && ftrace_indent->set)
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      plugin_function.c:148:29: note: uninitialized use occurs here
              trace_seq_printf(s, "%*s", index*3, "");
                                         ^~~~~
      plugin_function.c:145:2: note: remove the 'if' if its condition is always true
              if (parent && ftrace_indent->set)
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      plugin_function.c:145:6: warning: variable 'index' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
              if (parent && ftrace_indent->set)
                  ^~~~~~
      plugin_function.c:148:29: note: uninitialized use occurs here
              trace_seq_printf(s, "%*s", index*3, "");
                                         ^~~~~
      plugin_function.c:145:6: note: remove the '&&' if its condition is always true
              if (parent && ftrace_indent->set)
                  ^~~~~~~~~
      plugin_function.c:133:11: note: initialize the variable 'index' to silence this warning
              int index;
                       ^
                        = 0
      2 warnings generated.
    
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: David Ahern <dsahern@gmail.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Wang Nan <wangnan0@huawei.com>
    Link: http://lkml.kernel.org/n/tip-b5wyjocel55gorl2jq2cbxrr@git.kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
index a00ec190821a..42dbf73758f3 100644
--- a/tools/lib/traceevent/plugin_function.c
+++ b/tools/lib/traceevent/plugin_function.c
@@ -130,7 +130,7 @@ static int function_handler(struct trace_seq *s, struct pevent_record *record,
 	unsigned long long pfunction;
 	const char *func;
 	const char *parent;
-	int index;
+	int index = 0;
 
 	if (pevent_get_field_val(s, event, "ip", record, &function, 1))
 		return trace_seq_putc(s, '!');
 
> -- Steve
> 
> 
> From 4f060a0577156acfdc0524a634f4afcc1657c929 Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Date: Mon, 13 Feb 2017 12:11:44 -0500
> Subject: [PATCH] tools lib traceevent: Initialize lenght on
>  OLD_RING_BUFFER_TYPE_TIME_STAMP
> 
> A undefined value was being used for the OLD_RING_BUFFER_TYPE_TIME_STAMP
> case entry, as the 'length' variable was not being initialized, fix it.
> 
> Caught by the reporter when building tools/perf/ using clang, which emmitted
> this warning:
> 
>   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
> 
> Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Wang Nan <wangnan0@huawei.com>
> Link: http://lkml.kernel.org/r/20170210141408.5eeb6e91@gandalf.local.home
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  tools/lib/traceevent/kbuffer-parse.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
> index 65984f1..c94e364 100644
> --- a/tools/lib/traceevent/kbuffer-parse.c
> +++ b/tools/lib/traceevent/kbuffer-parse.c
> @@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
>  		extend += delta;
>  		delta = extend;
>  		ptr += 4;
> +		length = 0;
>  		break;
>  
>  	case OLD_RINGBUF_TYPE_TIME_STAMP:
> -- 
> 2.9.3

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-13 17:20         ` Arnaldo Carvalho de Melo
@ 2017-02-13 17:46           ` Steven Rostedt
  2017-02-17  2:00           ` Steven Rostedt
  1 sibling, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2017-02-13 17:46 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

On Mon, 13 Feb 2017 14:20:20 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:


> Sure, now take a look at this another one:
> 
> commit 6401e4361df183bd9953dce56f7c51d8ef28b11e
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> Date:   Mon Feb 13 13:33:57 2017 -0300
> 
>     tools lib traceevent plugin function: Initialize 'index' variable
>     
>     Detected with clang:
>     
>         CC       /tmp/build/perf/plugin_function.o
>       plugin_function.c:145:6: warning: variable 'index' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>               if (parent && ftrace_indent->set)
>                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       plugin_function.c:148:29: note: uninitialized use occurs here
>               trace_seq_printf(s, "%*s", index*3, "");
>                                          ^~~~~
>       plugin_function.c:145:2: note: remove the 'if' if its condition is always true
>               if (parent && ftrace_indent->set)
>               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       plugin_function.c:145:6: warning: variable 'index' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
>               if (parent && ftrace_indent->set)
>                   ^~~~~~
>       plugin_function.c:148:29: note: uninitialized use occurs here
>               trace_seq_printf(s, "%*s", index*3, "");
>                                          ^~~~~
>       plugin_function.c:145:6: note: remove the '&&' if its condition is always true
>               if (parent && ftrace_indent->set)
>                   ^~~~~~~~~
>       plugin_function.c:133:11: note: initialize the variable 'index' to silence this warning
>               int index;
>                        ^
>                         = 0
>       2 warnings generated.

I wonder why gcc isn't catching these. These look pretty obvious to me.

>     
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: David Ahern <dsahern@gmail.com>
>     Cc: Jiri Olsa <jolsa@kernel.org>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Wang Nan <wangnan0@huawei.com>
>     Link: http://lkml.kernel.org/n/tip-b5wyjocel55gorl2jq2cbxrr@git.kernel.org
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
> index a00ec190821a..42dbf73758f3 100644
> --- a/tools/lib/traceevent/plugin_function.c
> +++ b/tools/lib/traceevent/plugin_function.c
> @@ -130,7 +130,7 @@ static int function_handler(struct trace_seq *s, struct pevent_record *record,
>  	unsigned long long pfunction;
>  	const char *func;
>  	const char *parent;
> -	int index;
> +	int index = 0;
>  
>  	if (pevent_get_field_val(s, event, "ip", record, &function, 1))
>  		return trace_seq_putc(s, '!');
>  

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

^ permalink raw reply	[flat|nested] 10+ messages in thread
* [tip:perf/core] tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP
  2017-02-13 17:14       ` Steven Rostedt
  2017-02-13 17:20         ` Arnaldo Carvalho de Melo
@ 2017-02-14  6:40         ` tip-bot for Steven Rostedt (VMware)
  1 sibling, 0 replies; 10+ messages in thread
From: tip-bot for Steven Rostedt (VMware) @ 2017-02-14  6:40 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: namhyung, jolsa, tglx, acme, hpa, wangnan0, mingo, linux-kernel,
	rostedt, dsahern, adrian.hunter

Commit-ID:  14e4d7e0abfdefabea2b8796c5a8b2b9c77b5326
Gitweb:     http://git.kernel.org/tip/14e4d7e0abfdefabea2b8796c5a8b2b9c77b5326
Author:     Steven Rostedt (VMware) <rostedt@goodmis.org>
AuthorDate: Mon, 13 Feb 2017 12:11:44 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 13 Feb 2017 17:22:32 -0300

tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP

A undefined value was being used for the OLD_RING_BUFFER_TYPE_TIME_STAMP
case entry, as the 'length' variable was not being initialized, fix it.

Caught by the reporter when building tools/perf/ using clang, which emmitted
this warning:

  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

Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/20170213121418.47f279e8@gandalf.local.home
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/kbuffer-parse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/traceevent/kbuffer-parse.c b/tools/lib/traceevent/kbuffer-parse.c
index 65984f1..c94e364 100644
--- a/tools/lib/traceevent/kbuffer-parse.c
+++ b/tools/lib/traceevent/kbuffer-parse.c
@@ -315,6 +315,7 @@ static unsigned int old_update_pointers(struct kbuffer *kbuf)
 		extend += delta;
 		delta = extend;
 		ptr += 4;
+		length = 0;
 		break;
 
 	case OLD_RINGBUF_TYPE_TIME_STAMP:

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-13 17:20         ` Arnaldo Carvalho de Melo
  2017-02-13 17:46           ` Steven Rostedt
@ 2017-02-17  2:00           ` Steven Rostedt
  2017-02-17 13:44             ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2017-02-17  2:00 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

On Mon, 13 Feb 2017 14:20:20 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
   
>     Cc: Adrian Hunter <adrian.hunter@intel.com>
>     Cc: David Ahern <dsahern@gmail.com>
>     Cc: Jiri Olsa <jolsa@kernel.org>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Wang Nan <wangnan0@huawei.com>
>     Link: http://lkml.kernel.org/n/tip-b5wyjocel55gorl2jq2cbxrr@git.kernel.org
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
> index a00ec190821a..42dbf73758f3 100644
> --- a/tools/lib/traceevent/plugin_function.c
> +++ b/tools/lib/traceevent/plugin_function.c
> @@ -130,7 +130,7 @@ static int function_handler(struct trace_seq *s, struct pevent_record *record,
>  	unsigned long long pfunction;
>  	const char *func;
>  	const char *parent;
> -	int index;
> +	int index = 0;

I went to apply this to trace-cmd's version, and it failed to apply.
Then I realized that I had this fixed back in 2015, but missed sending
it. That's because I don't strictly follow the 80 column limit for some
of the if statements (I find the break up ugly), and when doing diffs,
this got lost in the diffs of the 80 column limits ones done with the
port to the kernel tree. :-/

-- Steve


>  
>  	if (pevent_get_field_val(s, event, "ip", record, &function, 1))
>  		return trace_seq_putc(s, '!');
>  

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] building libtraceevent with clang
  2017-02-17  2:00           ` Steven Rostedt
@ 2017-02-17 13:44             ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2017-02-17 13:44 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Namhyung Kim, Jiri Olsa, Linux Kernel Mailing List

Em Thu, Feb 16, 2017 at 09:00:23PM -0500, Steven Rostedt escreveu:
> On Mon, 13 Feb 2017 14:20:20 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > +++ b/tools/lib/traceevent/plugin_function.c
> > @@ -130,7 +130,7 @@ static int function_handler(struct trace_seq *s, struct pevent_record *record,
> >  	unsigned long long pfunction;
> >  	const char *func;
> >  	const char *parent;
> > -	int index;
> > +	int index = 0;
 
> I went to apply this to trace-cmd's version, and it failed to apply.  Then I
> realized that I had this fixed back in 2015, but missed sending it.

> That's because I don't strictly follow the 80 column limit for some of the if
> statements (I find the break up ugly), and when doing diffs, this got lost in
> the diffs of the 80 column limits ones done with the port to the kernel tree.
> :-/

I'm not strict about it either:

$ find tools/perf/ -name "*.[ch]" | xargs cat | wc -l
161984
$ find tools/perf/ -name "*.[ch]" | xargs expand | cut -c80- | grep -v ^$ | wc -l
3706
$ 

2.29%

You (well, libtracevent) are a bit more strict:

$ find tools/lib/traceevent/ -name "*.[ch]" | xargs cat | wc -l
13814
$ find tools/lib/traceevent/ -name "*.[ch]" | xargs expand | cut -c80- | grep -v ^$ | wc -l
184
$ 

1.33%

For reference, the whole kernel, modulo tools/

$ find . -name "*.[ch]" | grep -v ^\.\/tools | xargs cat | wc -l
20162880
$ find . -name "*.[ch]" | grep -v ^\.\/tools | xargs expand | cut -c80- | grep -v ^$ | wc -l
439722
$

2.18%

Using functions to avoid too deep if statements usually helps here tho :-)

- Arnaldo
 
> -- Steve
> 
> 
> >  
> >  	if (pevent_get_field_val(s, event, "ip", record, &function, 1))
> >  		return trace_seq_putc(s, '!');
> >  

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-17 13:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-10 17:03 [PATCH] building libtraceevent with clang Arnaldo Carvalho de Melo
2017-02-10 19:14 ` Steven Rostedt
2017-02-13 16:24   ` Arnaldo Carvalho de Melo
2017-02-13 16:26     ` Arnaldo Carvalho de Melo
2017-02-13 17:14       ` Steven Rostedt
2017-02-13 17:20         ` Arnaldo Carvalho de Melo
2017-02-13 17:46           ` Steven Rostedt
2017-02-17  2:00           ` Steven Rostedt
2017-02-17 13:44             ` Arnaldo Carvalho de Melo
2017-02-14  6:40         ` [tip:perf/core] tools lib traceevent: Initialize lenght on OLD_RING_BUFFER_TYPE_TIME_STAMP tip-bot for Steven Rostedt (VMware)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).