linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
@ 2020-05-13 23:47 Gustavo A. R. Silva
  2020-05-14  0:56 ` Ian Rogers
  2020-05-14 13:10 ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-13 23:47 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
	Ian Rogers
  Cc: linux-kernel, Gustavo A. R. Silva

Fix the following build failure generated with command 
$ make CC=clang HOSTCC=clang -C tools/ perf:

util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
                        struct branch_stack br_stack;
                                            ^
1 error generated.

Fix this by reordering the members of struct br.

Clang version 11.0.0 was used.

Fixes: f283f293a60d ("perf tools: Replace zero-length array with flexible-array")
Reported-by: Ian Rogers <irogers@google.com>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Here to fix what I break. :)

 tools/perf/util/intel-pt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index f17b1e769ae4..b34179e3926f 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -1799,8 +1799,8 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
 
 	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
 		struct {
-			struct branch_stack br_stack;
 			struct branch_entry entries[LBRS_MAX];
+			struct branch_stack br_stack;
 		} br;
 
 		if (items->mask[INTEL_PT_LBR_0_POS] ||
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-13 23:47 [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample Gustavo A. R. Silva
@ 2020-05-14  0:56 ` Ian Rogers
  2020-05-14 13:10 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 16+ messages in thread
From: Ian Rogers @ 2020-05-14  0:56 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva, clang-built-linux

On Wed, May 13, 2020 at 4:43 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> Fix the following build failure generated with command
> $ make CC=clang HOSTCC=clang -C tools/ perf:
>
> util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>                         struct branch_stack br_stack;
>                                             ^
> 1 error generated.
>
> Fix this by reordering the members of struct br.
>
> Clang version 11.0.0 was used.
>
> Fixes: f283f293a60d ("perf tools: Replace zero-length array with flexible-array")
> Reported-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Tested-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> ---
> Here to fix what I break. :)
>
>  tools/perf/util/intel-pt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index f17b1e769ae4..b34179e3926f 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -1799,8 +1799,8 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
>
>         if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
>                 struct {
> -                       struct branch_stack br_stack;
>                         struct branch_entry entries[LBRS_MAX];
> +                       struct branch_stack br_stack;
>                 } br;
>
>                 if (items->mask[INTEL_PT_LBR_0_POS] ||
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-13 23:47 [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample Gustavo A. R. Silva
  2020-05-14  0:56 ` Ian Rogers
@ 2020-05-14 13:10 ` Arnaldo Carvalho de Melo
  2020-05-14 15:06   ` Gustavo A. R. Silva
  1 sibling, 1 reply; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-14 13:10 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Ian Rogers, linux-kernel,
	Gustavo A. R. Silva

Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> Fix the following build failure generated with command 
> $ make CC=clang HOSTCC=clang -C tools/ perf:
> 
> util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>                         struct branch_stack br_stack;
>                                             ^
> 1 error generated.
> 
> Fix this by reordering the members of struct br.

Yeah, I noticed that as far back as with ubuntu 16.04's clang:

clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)

util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
      extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
                        struct branch_stack br_stack;
                                            ^
1 error generated.


Will fold this with the bug introducing the problem to avoid bisection
problems.

 
> Clang version 11.0.0 was used.
> 
> Fixes: f283f293a60d ("perf tools: Replace zero-length array with flexible-array")
> Reported-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Here to fix what I break. :)
> 
>  tools/perf/util/intel-pt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index f17b1e769ae4..b34179e3926f 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -1799,8 +1799,8 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
>  
>  	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
>  		struct {
> -			struct branch_stack br_stack;
>  			struct branch_entry entries[LBRS_MAX];
> +			struct branch_stack br_stack;
>  		} br;
>  
>  		if (items->mask[INTEL_PT_LBR_0_POS] ||
> -- 
> 2.26.2
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-14 13:10 ` Arnaldo Carvalho de Melo
@ 2020-05-14 15:06   ` Gustavo A. R. Silva
  2020-05-14 19:06     ` Ian Rogers
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-14 15:06 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Ian Rogers, linux-kernel,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > Fix the following build failure generated with command 
> > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > 
> > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> >                         struct branch_stack br_stack;
> >                                             ^
> > 1 error generated.
> > 
> > Fix this by reordering the members of struct br.
> 
> Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> 
> clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> 
> util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
>       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
>                         struct branch_stack br_stack;
>                                             ^
> 1 error generated.
> 
> 
> Will fold this with the bug introducing the problem to avoid bisection
> problems.
> 

I agree. Also, the commit hash of the "Fixes" tag only applies to the
perf/core branch and, I guess that might create confusion.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-14 15:06   ` Gustavo A. R. Silva
@ 2020-05-14 19:06     ` Ian Rogers
  2020-05-14 22:09       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2020-05-14 19:06 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > Fix the following build failure generated with command
> > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > >
> > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > >                         struct branch_stack br_stack;
> > >                                             ^
> > > 1 error generated.
> > >
> > > Fix this by reordering the members of struct br.
> >
> > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> >
> > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> >
> > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> >                         struct branch_stack br_stack;
> >                                             ^
> > 1 error generated.
> >
> >
> > Will fold this with the bug introducing the problem to avoid bisection
> > problems.
> >
>
> I agree. Also, the commit hash of the "Fixes" tag only applies to the
> perf/core branch and, I guess that might create confusion.


So while this fixes the warning I believe it breaks the intent of the code.

tools/perf/util/branch.h:
struct branch_stack {
       u64                     nr;
       u64                     hw_idx;
       struct branch_entry     entries[];
};

tools/perf/util/intel-pt.c:
               struct {
                       struct branch_stack br_stack;
                       struct branch_entry entries[LBRS_MAX];
               } br;

The array in br is trying to extend branch_stack's entries array. You
might have to do something like:

alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
sizeof(branch_entry) * LBRS_MAX];
struct branch_stack *br = &storage;

malloc/free may be nicer on the eyeballs.

Thanks,
Ian

> Thanks
> --
> Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-14 19:06     ` Ian Rogers
@ 2020-05-14 22:09       ` Gustavo A. R. Silva
  2020-05-14 22:46         ` Ian Rogers
  0 siblings, 1 reply; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-14 22:09 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 12:06:48PM -0700, Ian Rogers wrote:
> On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > > Fix the following build failure generated with command
> > > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > > >
> > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > >                         struct branch_stack br_stack;
> > > >                                             ^
> > > > 1 error generated.
> > > >
> > > > Fix this by reordering the members of struct br.
> > >
> > > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> > >
> > > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> > >
> > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> > >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > >                         struct branch_stack br_stack;
> > >                                             ^
> > > 1 error generated.
> > >
> > >
> > > Will fold this with the bug introducing the problem to avoid bisection
> > > problems.
> > >
> >
> > I agree. Also, the commit hash of the "Fixes" tag only applies to the
> > perf/core branch and, I guess that might create confusion.
> 
> 
> So while this fixes the warning I believe it breaks the intent of the code.
> 
> tools/perf/util/branch.h:
> struct branch_stack {
>        u64                     nr;
>        u64                     hw_idx;
>        struct branch_entry     entries[];
> };
> 
> tools/perf/util/intel-pt.c:
>                struct {
>                        struct branch_stack br_stack;
>                        struct branch_entry entries[LBRS_MAX];
>                } br;
> 
> The array in br is trying to extend branch_stack's entries array. You
> might have to do something like:
> 
> alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
> sizeof(branch_entry) * LBRS_MAX];
> struct branch_stack *br = &storage;
> 
> malloc/free may be nicer on the eyeballs.
> 

Yep, I'd go for zalloc/free. There are a couple of places where dynamic
memory is being allocated for struct branch_stack:

tools/perf/util/cs-etm.c-256-   if (etm->synth_opts.last_branch) {
tools/perf/util/cs-etm.c:257:           size_t sz = sizeof(struct branch_stack);
tools/perf/util/cs-etm.c-258-
tools/perf/util/cs-etm.c-259-           sz += etm->synth_opts.last_branch_sz *
tools/perf/util/cs-etm.c-260-                 sizeof(struct branch_entry);
tools/perf/util/cs-etm.c-261-           tidq->last_branch = zalloc(sz);

tools/perf/util/thread-stack.c-148-     if (br_stack_sz) {
tools/perf/util/thread-stack.c:149:             size_t sz = sizeof(struct branch_stack);
tools/perf/util/thread-stack.c-150-
tools/perf/util/thread-stack.c-151-             sz += br_stack_sz * sizeof(struct branch_entry);
tools/perf/util/thread-stack.c-152-             ts->br_stack_rb = zalloc(sz);

there is even function intel_pt_alloc_br_stack().

Just out of curiosity, why the need of such a hack in this case (the
on-stack extension of branch_stack's entries array)?

Thanks
--
Gustavo


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-14 22:09       ` Gustavo A. R. Silva
@ 2020-05-14 22:46         ` Ian Rogers
  2020-05-15  0:10           ` Gustavo A. R. Silva
  0 siblings, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2020-05-14 22:46 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> On Thu, May 14, 2020 at 12:06:48PM -0700, Ian Rogers wrote:
> > On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
> > <gustavoars@kernel.org> wrote:
> > >
> > > On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > > > Fix the following build failure generated with command
> > > > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > > > >
> > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > >                         struct branch_stack br_stack;
> > > > >                                             ^
> > > > > 1 error generated.
> > > > >
> > > > > Fix this by reordering the members of struct br.
> > > >
> > > > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> > > >
> > > > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> > > >
> > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> > > >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > >                         struct branch_stack br_stack;
> > > >                                             ^
> > > > 1 error generated.
> > > >
> > > >
> > > > Will fold this with the bug introducing the problem to avoid bisection
> > > > problems.
> > > >
> > >
> > > I agree. Also, the commit hash of the "Fixes" tag only applies to the
> > > perf/core branch and, I guess that might create confusion.
> >
> >
> > So while this fixes the warning I believe it breaks the intent of the code.
> >
> > tools/perf/util/branch.h:
> > struct branch_stack {
> >        u64                     nr;
> >        u64                     hw_idx;
> >        struct branch_entry     entries[];
> > };
> >
> > tools/perf/util/intel-pt.c:
> >                struct {
> >                        struct branch_stack br_stack;
> >                        struct branch_entry entries[LBRS_MAX];
> >                } br;
> >
> > The array in br is trying to extend branch_stack's entries array. You
> > might have to do something like:
> >
> > alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
> > sizeof(branch_entry) * LBRS_MAX];
> > struct branch_stack *br = &storage;
> >
> > malloc/free may be nicer on the eyeballs.
> >
>
> Yep, I'd go for zalloc/free. There are a couple of places where dynamic
> memory is being allocated for struct branch_stack:
>
> tools/perf/util/cs-etm.c-256-   if (etm->synth_opts.last_branch) {
> tools/perf/util/cs-etm.c:257:           size_t sz = sizeof(struct branch_stack);
> tools/perf/util/cs-etm.c-258-
> tools/perf/util/cs-etm.c-259-           sz += etm->synth_opts.last_branch_sz *
> tools/perf/util/cs-etm.c-260-                 sizeof(struct branch_entry);
> tools/perf/util/cs-etm.c-261-           tidq->last_branch = zalloc(sz);
>
> tools/perf/util/thread-stack.c-148-     if (br_stack_sz) {
> tools/perf/util/thread-stack.c:149:             size_t sz = sizeof(struct branch_stack);
> tools/perf/util/thread-stack.c-150-
> tools/perf/util/thread-stack.c-151-             sz += br_stack_sz * sizeof(struct branch_entry);
> tools/perf/util/thread-stack.c-152-             ts->br_stack_rb = zalloc(sz);
>
> there is even function intel_pt_alloc_br_stack().
>
> Just out of curiosity, why the need of such a hack in this case (the
> on-stack extension of branch_stack's entries array)?

My guess would be that the lbr size is an architectural constant and
so avoiding malloc/free in what could be a hot loop was desirable.
As this is part of a larger patch set, is this the only place this
problem has been encountered? Perhaps a macro could perform the
complicated stack allocation I suggested. It may be nice to save
cycles if code this pattern is widespread and the code hot.

Thanks,
Ian

> Thanks
> --
> Gustavo
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-15  0:10           ` Gustavo A. R. Silva
@ 2020-05-15  0:09             ` Ian Rogers
  2020-05-15  0:29               ` Gustavo A. R. Silva
  2020-05-15 16:41             ` Arnaldo Carvalho de Melo
  1 sibling, 1 reply; 16+ messages in thread
From: Ian Rogers @ 2020-05-15  0:09 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 5:05 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> On Thu, May 14, 2020 at 03:46:05PM -0700, Ian Rogers wrote:
> > On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
> > <gustavoars@kernel.org> wrote:
> > >
> > > On Thu, May 14, 2020 at 12:06:48PM -0700, Ian Rogers wrote:
> > > > On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
> > > > <gustavoars@kernel.org> wrote:
> > > > >
> > > > > On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > > > > > Fix the following build failure generated with command
> > > > > > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > > > > > >
> > > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > > >                         struct branch_stack br_stack;
> > > > > > >                                             ^
> > > > > > > 1 error generated.
> > > > > > >
> > > > > > > Fix this by reordering the members of struct br.
> > > > > >
> > > > > > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> > > > > >
> > > > > > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> > > > > >
> > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> > > > > >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > >                         struct branch_stack br_stack;
> > > > > >                                             ^
> > > > > > 1 error generated.
> > > > > >
> > > > > >
> > > > > > Will fold this with the bug introducing the problem to avoid bisection
> > > > > > problems.
> > > > > >
> > > > >
> > > > > I agree. Also, the commit hash of the "Fixes" tag only applies to the
> > > > > perf/core branch and, I guess that might create confusion.
> > > >
> > > >
> > > > So while this fixes the warning I believe it breaks the intent of the code.
> > > >
> > > > tools/perf/util/branch.h:
> > > > struct branch_stack {
> > > >        u64                     nr;
> > > >        u64                     hw_idx;
> > > >        struct branch_entry     entries[];
> > > > };
> > > >
> > > > tools/perf/util/intel-pt.c:
> > > >                struct {
> > > >                        struct branch_stack br_stack;
> > > >                        struct branch_entry entries[LBRS_MAX];
> > > >                } br;
> > > >
> > > > The array in br is trying to extend branch_stack's entries array. You
> > > > might have to do something like:
> > > >
> > > > alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
> > > > sizeof(branch_entry) * LBRS_MAX];
> > > > struct branch_stack *br = &storage;
> > > >
> > > > malloc/free may be nicer on the eyeballs.
> > > >
> > >
> > > Yep, I'd go for zalloc/free. There are a couple of places where dynamic
> > > memory is being allocated for struct branch_stack:
> > >
> > > tools/perf/util/cs-etm.c-256-   if (etm->synth_opts.last_branch) {
> > > tools/perf/util/cs-etm.c:257:           size_t sz = sizeof(struct branch_stack);
> > > tools/perf/util/cs-etm.c-258-
> > > tools/perf/util/cs-etm.c-259-           sz += etm->synth_opts.last_branch_sz *
> > > tools/perf/util/cs-etm.c-260-                 sizeof(struct branch_entry);
> > > tools/perf/util/cs-etm.c-261-           tidq->last_branch = zalloc(sz);
> > >
> > > tools/perf/util/thread-stack.c-148-     if (br_stack_sz) {
> > > tools/perf/util/thread-stack.c:149:             size_t sz = sizeof(struct branch_stack);
> > > tools/perf/util/thread-stack.c-150-
> > > tools/perf/util/thread-stack.c-151-             sz += br_stack_sz * sizeof(struct branch_entry);
> > > tools/perf/util/thread-stack.c-152-             ts->br_stack_rb = zalloc(sz);
> > >
> > > there is even function intel_pt_alloc_br_stack().
> > >
> > > Just out of curiosity, why the need of such a hack in this case (the
> > > on-stack extension of branch_stack's entries array)?
> >
> > My guess would be that the lbr size is an architectural constant and
> > so avoiding malloc/free in what could be a hot loop was desirable.
> > As this is part of a larger patch set, is this the only place this
> > problem has been encountered? Perhaps a macro could perform the
>
> Yep. I just built linux-next --which contains all the flexible-array
> conversions-- with Clang --GCC doesn't catch this issue, not even GCC
> 10-- and I don't see any other issue like this.
>
> I mean, I have run into these other two:
>
> https://lore.kernel.org/lkml/20200505235205.GA18539@embeddedor/
> https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/
>
> but those are due to the erroneous application of the sizeof operator
> to zero-length arrays.
>
> > complicated stack allocation I suggested. It may be nice to save
> > cycles if code this pattern is widespread and the code hot.
> >
>
> Apparently, this is the only instace of this sort of issue in the whole
> codebase.

Thanks for checking, I'd convert it to malloc/free but Intel really
owns this code.
Ian

> Thanks
> --
> Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-14 22:46         ` Ian Rogers
@ 2020-05-15  0:10           ` Gustavo A. R. Silva
  2020-05-15  0:09             ` Ian Rogers
  2020-05-15 16:41             ` Arnaldo Carvalho de Melo
  0 siblings, 2 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-15  0:10 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 03:46:05PM -0700, Ian Rogers wrote:
> On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > On Thu, May 14, 2020 at 12:06:48PM -0700, Ian Rogers wrote:
> > > On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
> > > <gustavoars@kernel.org> wrote:
> > > >
> > > > On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > > > > Fix the following build failure generated with command
> > > > > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > > > > >
> > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > >                         struct branch_stack br_stack;
> > > > > >                                             ^
> > > > > > 1 error generated.
> > > > > >
> > > > > > Fix this by reordering the members of struct br.
> > > > >
> > > > > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> > > > >
> > > > > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> > > > >
> > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> > > > >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > >                         struct branch_stack br_stack;
> > > > >                                             ^
> > > > > 1 error generated.
> > > > >
> > > > >
> > > > > Will fold this with the bug introducing the problem to avoid bisection
> > > > > problems.
> > > > >
> > > >
> > > > I agree. Also, the commit hash of the "Fixes" tag only applies to the
> > > > perf/core branch and, I guess that might create confusion.
> > >
> > >
> > > So while this fixes the warning I believe it breaks the intent of the code.
> > >
> > > tools/perf/util/branch.h:
> > > struct branch_stack {
> > >        u64                     nr;
> > >        u64                     hw_idx;
> > >        struct branch_entry     entries[];
> > > };
> > >
> > > tools/perf/util/intel-pt.c:
> > >                struct {
> > >                        struct branch_stack br_stack;
> > >                        struct branch_entry entries[LBRS_MAX];
> > >                } br;
> > >
> > > The array in br is trying to extend branch_stack's entries array. You
> > > might have to do something like:
> > >
> > > alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
> > > sizeof(branch_entry) * LBRS_MAX];
> > > struct branch_stack *br = &storage;
> > >
> > > malloc/free may be nicer on the eyeballs.
> > >
> >
> > Yep, I'd go for zalloc/free. There are a couple of places where dynamic
> > memory is being allocated for struct branch_stack:
> >
> > tools/perf/util/cs-etm.c-256-   if (etm->synth_opts.last_branch) {
> > tools/perf/util/cs-etm.c:257:           size_t sz = sizeof(struct branch_stack);
> > tools/perf/util/cs-etm.c-258-
> > tools/perf/util/cs-etm.c-259-           sz += etm->synth_opts.last_branch_sz *
> > tools/perf/util/cs-etm.c-260-                 sizeof(struct branch_entry);
> > tools/perf/util/cs-etm.c-261-           tidq->last_branch = zalloc(sz);
> >
> > tools/perf/util/thread-stack.c-148-     if (br_stack_sz) {
> > tools/perf/util/thread-stack.c:149:             size_t sz = sizeof(struct branch_stack);
> > tools/perf/util/thread-stack.c-150-
> > tools/perf/util/thread-stack.c-151-             sz += br_stack_sz * sizeof(struct branch_entry);
> > tools/perf/util/thread-stack.c-152-             ts->br_stack_rb = zalloc(sz);
> >
> > there is even function intel_pt_alloc_br_stack().
> >
> > Just out of curiosity, why the need of such a hack in this case (the
> > on-stack extension of branch_stack's entries array)?
> 
> My guess would be that the lbr size is an architectural constant and
> so avoiding malloc/free in what could be a hot loop was desirable.
> As this is part of a larger patch set, is this the only place this
> problem has been encountered? Perhaps a macro could perform the

Yep. I just built linux-next --which contains all the flexible-array
conversions-- with Clang --GCC doesn't catch this issue, not even GCC
10-- and I don't see any other issue like this.

I mean, I have run into these other two:

https://lore.kernel.org/lkml/20200505235205.GA18539@embeddedor/
https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/

but those are due to the erroneous application of the sizeof operator
to zero-length arrays.

> complicated stack allocation I suggested. It may be nice to save
> cycles if code this pattern is widespread and the code hot.
> 

Apparently, this is the only instace of this sort of issue in the whole
codebase.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-15  0:09             ` Ian Rogers
@ 2020-05-15  0:29               ` Gustavo A. R. Silva
  0 siblings, 0 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-15  0:29 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Thu, May 14, 2020 at 05:09:23PM -0700, Ian Rogers wrote:
> On Thu, May 14, 2020 at 5:05 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > On Thu, May 14, 2020 at 03:46:05PM -0700, Ian Rogers wrote:
> > > On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
> > > <gustavoars@kernel.org> wrote:
> > > >
> > > > On Thu, May 14, 2020 at 12:06:48PM -0700, Ian Rogers wrote:
> > > > > On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
> > > > > <gustavoars@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > > > > > > Fix the following build failure generated with command
> > > > > > > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > > > > > > >
> > > > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > > > >                         struct branch_stack br_stack;
> > > > > > > >                                             ^
> > > > > > > > 1 error generated.
> > > > > > > >
> > > > > > > > Fix this by reordering the members of struct br.
> > > > > > >
> > > > > > > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> > > > > > >
> > > > > > > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> > > > > > >
> > > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> > > > > > >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > > >                         struct branch_stack br_stack;
> > > > > > >                                             ^
> > > > > > > 1 error generated.
> > > > > > >
> > > > > > >
> > > > > > > Will fold this with the bug introducing the problem to avoid bisection
> > > > > > > problems.
> > > > > > >
> > > > > >
> > > > > > I agree. Also, the commit hash of the "Fixes" tag only applies to the
> > > > > > perf/core branch and, I guess that might create confusion.
> > > > >
> > > > >
> > > > > So while this fixes the warning I believe it breaks the intent of the code.
> > > > >
> > > > > tools/perf/util/branch.h:
> > > > > struct branch_stack {
> > > > >        u64                     nr;
> > > > >        u64                     hw_idx;
> > > > >        struct branch_entry     entries[];
> > > > > };
> > > > >
> > > > > tools/perf/util/intel-pt.c:
> > > > >                struct {
> > > > >                        struct branch_stack br_stack;
> > > > >                        struct branch_entry entries[LBRS_MAX];
> > > > >                } br;
> > > > >
> > > > > The array in br is trying to extend branch_stack's entries array. You
> > > > > might have to do something like:
> > > > >
> > > > > alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
> > > > > sizeof(branch_entry) * LBRS_MAX];
> > > > > struct branch_stack *br = &storage;
> > > > >
> > > > > malloc/free may be nicer on the eyeballs.
> > > > >
> > > >
> > > > Yep, I'd go for zalloc/free. There are a couple of places where dynamic
> > > > memory is being allocated for struct branch_stack:
> > > >
> > > > tools/perf/util/cs-etm.c-256-   if (etm->synth_opts.last_branch) {
> > > > tools/perf/util/cs-etm.c:257:           size_t sz = sizeof(struct branch_stack);
> > > > tools/perf/util/cs-etm.c-258-
> > > > tools/perf/util/cs-etm.c-259-           sz += etm->synth_opts.last_branch_sz *
> > > > tools/perf/util/cs-etm.c-260-                 sizeof(struct branch_entry);
> > > > tools/perf/util/cs-etm.c-261-           tidq->last_branch = zalloc(sz);
> > > >
> > > > tools/perf/util/thread-stack.c-148-     if (br_stack_sz) {
> > > > tools/perf/util/thread-stack.c:149:             size_t sz = sizeof(struct branch_stack);
> > > > tools/perf/util/thread-stack.c-150-
> > > > tools/perf/util/thread-stack.c-151-             sz += br_stack_sz * sizeof(struct branch_entry);
> > > > tools/perf/util/thread-stack.c-152-             ts->br_stack_rb = zalloc(sz);
> > > >
> > > > there is even function intel_pt_alloc_br_stack().
> > > >
> > > > Just out of curiosity, why the need of such a hack in this case (the
> > > > on-stack extension of branch_stack's entries array)?
> > >
> > > My guess would be that the lbr size is an architectural constant and
> > > so avoiding malloc/free in what could be a hot loop was desirable.
> > > As this is part of a larger patch set, is this the only place this
> > > problem has been encountered? Perhaps a macro could perform the
> >
> > Yep. I just built linux-next --which contains all the flexible-array
> > conversions-- with Clang --GCC doesn't catch this issue, not even GCC
> > 10-- and I don't see any other issue like this.
> >
> > I mean, I have run into these other two:
> >
> > https://lore.kernel.org/lkml/20200505235205.GA18539@embeddedor/
> > https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/
> >
> > but those are due to the erroneous application of the sizeof operator
> > to zero-length arrays.
> >
> > > complicated stack allocation I suggested. It may be nice to save
> > > cycles if code this pattern is widespread and the code hot.
> > >
> >
> > Apparently, this is the only instace of this sort of issue in the whole
> > codebase.
> 
> Thanks for checking, I'd convert it to malloc/free but Intel really
> owns this code.
> 

Go ahead and I can add that part to my patch and include a Co-developed-by
tag. :)

Thanks
--
Gustavo


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-15  0:10           ` Gustavo A. R. Silva
  2020-05-15  0:09             ` Ian Rogers
@ 2020-05-15 16:41             ` Arnaldo Carvalho de Melo
  2020-05-15 16:43               ` Arnaldo Carvalho de Melo
  2020-05-16 12:35               ` Adrian Hunter
  1 sibling, 2 replies; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-15 16:41 UTC (permalink / raw)
  To: Adrian Hunter, Gustavo A. R. Silva
  Cc: Ian Rogers, Arnaldo Carvalho de Melo, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, LKML, Gustavo A. R. Silva

Sorry for the top post: Adrian, can you take a look at this?

Em Thu, May 14, 2020 at 07:10:25PM -0500, Gustavo A. R. Silva escreveu:
> On Thu, May 14, 2020 at 03:46:05PM -0700, Ian Rogers wrote:
> > On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
> > <gustavoars@kernel.org> wrote:
> > >
> > > On Thu, May 14, 2020 at 12:06:48PM -0700, Ian Rogers wrote:
> > > > On Thu, May 14, 2020 at 8:01 AM Gustavo A. R. Silva
> > > > <gustavoars@kernel.org> wrote:
> > > > >
> > > > > On Thu, May 14, 2020 at 10:10:30AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > > Em Wed, May 13, 2020 at 06:47:38PM -0500, Gustavo A. R. Silva escreveu:
> > > > > > > Fix the following build failure generated with command
> > > > > > > $ make CC=clang HOSTCC=clang -C tools/ perf:
> > > > > > >
> > > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > > >                         struct branch_stack br_stack;
> > > > > > >                                             ^
> > > > > > > 1 error generated.
> > > > > > >
> > > > > > > Fix this by reordering the members of struct br.
> > > > > >
> > > > > > Yeah, I noticed that as far back as with ubuntu 16.04's clang:
> > > > > >
> > > > > > clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
> > > > > >
> > > > > > util/intel-pt.c:1802:24: error: field 'br_stack' with variable sized type 'struct branch_stack' not at the end of a struct or class is a GNU
> > > > > >       extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
> > > > > >                         struct branch_stack br_stack;
> > > > > >                                             ^
> > > > > > 1 error generated.
> > > > > >
> > > > > >
> > > > > > Will fold this with the bug introducing the problem to avoid bisection
> > > > > > problems.
> > > > > >
> > > > >
> > > > > I agree. Also, the commit hash of the "Fixes" tag only applies to the
> > > > > perf/core branch and, I guess that might create confusion.
> > > >
> > > >
> > > > So while this fixes the warning I believe it breaks the intent of the code.
> > > >
> > > > tools/perf/util/branch.h:
> > > > struct branch_stack {
> > > >        u64                     nr;
> > > >        u64                     hw_idx;
> > > >        struct branch_entry     entries[];
> > > > };
> > > >
> > > > tools/perf/util/intel-pt.c:
> > > >                struct {
> > > >                        struct branch_stack br_stack;
> > > >                        struct branch_entry entries[LBRS_MAX];
> > > >                } br;
> > > >
> > > > The array in br is trying to extend branch_stack's entries array. You
> > > > might have to do something like:
> > > >
> > > > alignas(alignof(branch_stack)) char storage[sizeof(branch_stack) +
> > > > sizeof(branch_entry) * LBRS_MAX];
> > > > struct branch_stack *br = &storage;
> > > >
> > > > malloc/free may be nicer on the eyeballs.
> > > >
> > >
> > > Yep, I'd go for zalloc/free. There are a couple of places where dynamic
> > > memory is being allocated for struct branch_stack:
> > >
> > > tools/perf/util/cs-etm.c-256-   if (etm->synth_opts.last_branch) {
> > > tools/perf/util/cs-etm.c:257:           size_t sz = sizeof(struct branch_stack);
> > > tools/perf/util/cs-etm.c-258-
> > > tools/perf/util/cs-etm.c-259-           sz += etm->synth_opts.last_branch_sz *
> > > tools/perf/util/cs-etm.c-260-                 sizeof(struct branch_entry);
> > > tools/perf/util/cs-etm.c-261-           tidq->last_branch = zalloc(sz);
> > >
> > > tools/perf/util/thread-stack.c-148-     if (br_stack_sz) {
> > > tools/perf/util/thread-stack.c:149:             size_t sz = sizeof(struct branch_stack);
> > > tools/perf/util/thread-stack.c-150-
> > > tools/perf/util/thread-stack.c-151-             sz += br_stack_sz * sizeof(struct branch_entry);
> > > tools/perf/util/thread-stack.c-152-             ts->br_stack_rb = zalloc(sz);
> > >
> > > there is even function intel_pt_alloc_br_stack().
> > >
> > > Just out of curiosity, why the need of such a hack in this case (the
> > > on-stack extension of branch_stack's entries array)?
> > 
> > My guess would be that the lbr size is an architectural constant and
> > so avoiding malloc/free in what could be a hot loop was desirable.
> > As this is part of a larger patch set, is this the only place this
> > problem has been encountered? Perhaps a macro could perform the
> 
> Yep. I just built linux-next --which contains all the flexible-array
> conversions-- with Clang --GCC doesn't catch this issue, not even GCC
> 10-- and I don't see any other issue like this.
> 
> I mean, I have run into these other two:
> 
> https://lore.kernel.org/lkml/20200505235205.GA18539@embeddedor/
> https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/
> 
> but those are due to the erroneous application of the sizeof operator
> to zero-length arrays.
> 
> > complicated stack allocation I suggested. It may be nice to save
> > cycles if code this pattern is widespread and the code hot.
> > 
> 
> Apparently, this is the only instace of this sort of issue in the whole
> codebase.

Adrian Hunter was not CCed, Adrian?

- Arnaldo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-15 16:41             ` Arnaldo Carvalho de Melo
@ 2020-05-15 16:43               ` Arnaldo Carvalho de Melo
  2020-05-15 17:09                 ` Gustavo A. R. Silva
  2020-05-16 12:35               ` Adrian Hunter
  1 sibling, 1 reply; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-15 16:43 UTC (permalink / raw)
  To: Adrian Hunter, Gustavo A. R. Silva
  Cc: Ian Rogers, Arnaldo Carvalho de Melo, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, LKML, Gustavo A. R. Silva

Em Fri, May 15, 2020 at 01:41:53PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, May 14, 2020 at 07:10:25PM -0500, Gustavo A. R. Silva escreveu:
> > On Thu, May 14, 2020 at 03:46:05PM -0700, Ian Rogers wrote:
> > > On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
> > > <gustavoars@kernel.org> wrote:
> > 
> > Yep. I just built linux-next --which contains all the flexible-array
> > conversions-- with Clang --GCC doesn't catch this issue, not even GCC
> > 10-- and I don't see any other issue like this.
> > 
> > I mean, I have run into these other two:
> > 
> > https://lore.kernel.org/lkml/20200505235205.GA18539@embeddedor/
> > https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/
> > 
> > but those are due to the erroneous application of the sizeof operator
> > to zero-length arrays.
> > 
> > > complicated stack allocation I suggested. It may be nice to save
> > > cycles if code this pattern is widespread and the code hot.
> > > 
> > 
> > Apparently, this is the only instace of this sort of issue in the whole
> > codebase.
> 
> Adrian Hunter was not CCed, Adrian?

Gustavo, I've removed this from my tree from now till this gets
resolved.

- Arnaldo

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-15 16:43               ` Arnaldo Carvalho de Melo
@ 2020-05-15 17:09                 ` Gustavo A. R. Silva
  0 siblings, 0 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-15 17:09 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Adrian Hunter, Ian Rogers, Peter Zijlstra, Ingo Molnar,
	Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On Fri, May 15, 2020 at 01:43:31PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, May 15, 2020 at 01:41:53PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, May 14, 2020 at 07:10:25PM -0500, Gustavo A. R. Silva escreveu:
> > > On Thu, May 14, 2020 at 03:46:05PM -0700, Ian Rogers wrote:
> > > > On Thu, May 14, 2020 at 3:04 PM Gustavo A. R. Silva
> > > > <gustavoars@kernel.org> wrote:
> > > 
> > > Yep. I just built linux-next --which contains all the flexible-array
> > > conversions-- with Clang --GCC doesn't catch this issue, not even GCC
> > > 10-- and I don't see any other issue like this.
> > > 
> > > I mean, I have run into these other two:
> > > 
> > > https://lore.kernel.org/lkml/20200505235205.GA18539@embeddedor/
> > > https://lore.kernel.org/lkml/20200508163826.GA768@embeddedor/
> > > 
> > > but those are due to the erroneous application of the sizeof operator
> > > to zero-length arrays.
> > > 
> > > > complicated stack allocation I suggested. It may be nice to save
> > > > cycles if code this pattern is widespread and the code hot.
> > > > 
> > > 
> > > Apparently, this is the only instace of this sort of issue in the whole
> > > codebase.
> > 
> > Adrian Hunter was not CCed, Adrian?
> 
> Gustavo, I've removed this from my tree from now till this gets
> resolved.
> 

Yep, sure.  In the meantime, I'll send a patch without the changes
to struct branch_stack.

Thanks
--
Gustavo


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-15 16:41             ` Arnaldo Carvalho de Melo
  2020-05-15 16:43               ` Arnaldo Carvalho de Melo
@ 2020-05-16 12:35               ` Adrian Hunter
  2020-05-19 19:12                 ` Gustavo A. R. Silva
  2020-05-20  1:42                 ` Arnaldo Carvalho de Melo
  1 sibling, 2 replies; 16+ messages in thread
From: Adrian Hunter @ 2020-05-16 12:35 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Gustavo A. R. Silva
  Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
	Alexander Shishkin, Jiri Olsa, Namhyung Kim, LKML,
	Gustavo A. R. Silva

On 15/05/20 7:41 pm, Arnaldo Carvalho de Melo wrote:
> Sorry for the top post: Adrian, can you take a look at this?
> 
> Adrian Hunter was not CCed, Adrian?

From: Adrian Hunter <adrian.hunter@intel.com>
Date: Sat, 16 May 2020 15:12:28 +0300
Subject: [PATCH] perf intel-pt: Use allocated branch stack for PEBS sample

To avoid having struct branch_stack as a non-last structure member,
use allocated branch stack for PEBS sample.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/intel-pt.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
index f17b1e769ae4..e4dd8bf610ce 100644
--- a/tools/perf/util/intel-pt.c
+++ b/tools/perf/util/intel-pt.c
@@ -913,11 +913,11 @@ static void intel_pt_add_callchain(struct intel_pt *pt,
 	sample->callchain = pt->chain;
 }
 
-static struct branch_stack *intel_pt_alloc_br_stack(struct intel_pt *pt)
+static struct branch_stack *intel_pt_alloc_br_stack(unsigned int entry_cnt)
 {
 	size_t sz = sizeof(struct branch_stack);
 
-	sz += pt->br_stack_sz * sizeof(struct branch_entry);
+	sz += entry_cnt * sizeof(struct branch_entry);
 	return zalloc(sz);
 }
 
@@ -930,7 +930,7 @@ static int intel_pt_br_stack_init(struct intel_pt *pt)
 			evsel->synth_sample_type |= PERF_SAMPLE_BRANCH_STACK;
 	}
 
-	pt->br_stack = intel_pt_alloc_br_stack(pt);
+	pt->br_stack = intel_pt_alloc_br_stack(pt->br_stack_sz);
 	if (!pt->br_stack)
 		return -ENOMEM;
 
@@ -951,6 +951,9 @@ static void intel_pt_add_br_stack(struct intel_pt *pt,
 	sample->branch_stack = pt->br_stack;
 }
 
+/* INTEL_PT_LBR_0, INTEL_PT_LBR_1 and INTEL_PT_LBR_2 */
+#define LBRS_MAX (INTEL_PT_BLK_ITEM_ID_CNT * 3U)
+
 static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
 						   unsigned int queue_nr)
 {
@@ -968,8 +971,10 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
 			goto out_free;
 	}
 
-	if (pt->synth_opts.last_branch) {
-		ptq->last_branch = intel_pt_alloc_br_stack(pt);
+	if (pt->synth_opts.last_branch || pt->synth_opts.other_events) {
+		unsigned int entry_cnt = max(LBRS_MAX, pt->br_stack_sz);
+
+		ptq->last_branch = intel_pt_alloc_br_stack(entry_cnt);
 		if (!ptq->last_branch)
 			goto out_free;
 	}
@@ -1720,9 +1725,6 @@ static void intel_pt_add_lbrs(struct branch_stack *br_stack,
 	}
 }
 
-/* INTEL_PT_LBR_0, INTEL_PT_LBR_1 and INTEL_PT_LBR_2 */
-#define LBRS_MAX (INTEL_PT_BLK_ITEM_ID_CNT * 3)
-
 static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
 {
 	const struct intel_pt_blk_items *items = &ptq->state->items;
@@ -1798,25 +1800,18 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
 	}
 
 	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
-		struct {
-			struct branch_stack br_stack;
-			struct branch_entry entries[LBRS_MAX];
-		} br;
-
 		if (items->mask[INTEL_PT_LBR_0_POS] ||
 		    items->mask[INTEL_PT_LBR_1_POS] ||
 		    items->mask[INTEL_PT_LBR_2_POS]) {
-			intel_pt_add_lbrs(&br.br_stack, items);
-			sample.branch_stack = &br.br_stack;
+			intel_pt_add_lbrs(ptq->last_branch, items);
 		} else if (pt->synth_opts.last_branch) {
 			thread_stack__br_sample(ptq->thread, ptq->cpu,
 						ptq->last_branch,
 						pt->br_stack_sz);
-			sample.branch_stack = ptq->last_branch;
 		} else {
-			br.br_stack.nr = 0;
-			sample.branch_stack = &br.br_stack;
+			ptq->last_branch->nr = 0;
 		}
+		sample.branch_stack = ptq->last_branch;
 	}
 
 	if (sample_type & PERF_SAMPLE_ADDR && items->has_mem_access_address)
-- 
2.17.1




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-16 12:35               ` Adrian Hunter
@ 2020-05-19 19:12                 ` Gustavo A. R. Silva
  2020-05-20  1:42                 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 16+ messages in thread
From: Gustavo A. R. Silva @ 2020-05-19 19:12 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Arnaldo Carvalho de Melo, Ian Rogers, Peter Zijlstra,
	Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa,
	Namhyung Kim, LKML, Gustavo A. R. Silva

On Sat, May 16, 2020 at 03:35:48PM +0300, Adrian Hunter wrote:
> On 15/05/20 7:41 pm, Arnaldo Carvalho de Melo wrote:
> > Sorry for the top post: Adrian, can you take a look at this?
> > 
> > Adrian Hunter was not CCed, Adrian?
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Sat, 16 May 2020 15:12:28 +0300
> Subject: [PATCH] perf intel-pt: Use allocated branch stack for PEBS sample
> 
> To avoid having struct branch_stack as a non-last structure member,
> use allocated branch stack for PEBS sample.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>

Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>>

Thanks, Adrian.
--
Gustavo

> ---
>  tools/perf/util/intel-pt.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index f17b1e769ae4..e4dd8bf610ce 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -913,11 +913,11 @@ static void intel_pt_add_callchain(struct intel_pt *pt,
>  	sample->callchain = pt->chain;
>  }
>  
> -static struct branch_stack *intel_pt_alloc_br_stack(struct intel_pt *pt)
> +static struct branch_stack *intel_pt_alloc_br_stack(unsigned int entry_cnt)
>  {
>  	size_t sz = sizeof(struct branch_stack);
>  
> -	sz += pt->br_stack_sz * sizeof(struct branch_entry);
> +	sz += entry_cnt * sizeof(struct branch_entry);
>  	return zalloc(sz);
>  }
>  
> @@ -930,7 +930,7 @@ static int intel_pt_br_stack_init(struct intel_pt *pt)
>  			evsel->synth_sample_type |= PERF_SAMPLE_BRANCH_STACK;
>  	}
>  
> -	pt->br_stack = intel_pt_alloc_br_stack(pt);
> +	pt->br_stack = intel_pt_alloc_br_stack(pt->br_stack_sz);
>  	if (!pt->br_stack)
>  		return -ENOMEM;
>  
> @@ -951,6 +951,9 @@ static void intel_pt_add_br_stack(struct intel_pt *pt,
>  	sample->branch_stack = pt->br_stack;
>  }
>  
> +/* INTEL_PT_LBR_0, INTEL_PT_LBR_1 and INTEL_PT_LBR_2 */
> +#define LBRS_MAX (INTEL_PT_BLK_ITEM_ID_CNT * 3U)
> +
>  static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>  						   unsigned int queue_nr)
>  {
> @@ -968,8 +971,10 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>  			goto out_free;
>  	}
>  
> -	if (pt->synth_opts.last_branch) {
> -		ptq->last_branch = intel_pt_alloc_br_stack(pt);
> +	if (pt->synth_opts.last_branch || pt->synth_opts.other_events) {
> +		unsigned int entry_cnt = max(LBRS_MAX, pt->br_stack_sz);
> +
> +		ptq->last_branch = intel_pt_alloc_br_stack(entry_cnt);
>  		if (!ptq->last_branch)
>  			goto out_free;
>  	}
> @@ -1720,9 +1725,6 @@ static void intel_pt_add_lbrs(struct branch_stack *br_stack,
>  	}
>  }
>  
> -/* INTEL_PT_LBR_0, INTEL_PT_LBR_1 and INTEL_PT_LBR_2 */
> -#define LBRS_MAX (INTEL_PT_BLK_ITEM_ID_CNT * 3)
> -
>  static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
>  {
>  	const struct intel_pt_blk_items *items = &ptq->state->items;
> @@ -1798,25 +1800,18 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
>  	}
>  
>  	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
> -		struct {
> -			struct branch_stack br_stack;
> -			struct branch_entry entries[LBRS_MAX];
> -		} br;
> -
>  		if (items->mask[INTEL_PT_LBR_0_POS] ||
>  		    items->mask[INTEL_PT_LBR_1_POS] ||
>  		    items->mask[INTEL_PT_LBR_2_POS]) {
> -			intel_pt_add_lbrs(&br.br_stack, items);
> -			sample.branch_stack = &br.br_stack;
> +			intel_pt_add_lbrs(ptq->last_branch, items);
>  		} else if (pt->synth_opts.last_branch) {
>  			thread_stack__br_sample(ptq->thread, ptq->cpu,
>  						ptq->last_branch,
>  						pt->br_stack_sz);
> -			sample.branch_stack = ptq->last_branch;
>  		} else {
> -			br.br_stack.nr = 0;
> -			sample.branch_stack = &br.br_stack;
> +			ptq->last_branch->nr = 0;
>  		}
> +		sample.branch_stack = ptq->last_branch;
>  	}
>  
>  	if (sample_type & PERF_SAMPLE_ADDR && items->has_mem_access_address)
> -- 
> 2.17.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample
  2020-05-16 12:35               ` Adrian Hunter
  2020-05-19 19:12                 ` Gustavo A. R. Silva
@ 2020-05-20  1:42                 ` Arnaldo Carvalho de Melo
  1 sibling, 0 replies; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-05-20  1:42 UTC (permalink / raw)
  To: Adrian Hunter
  Cc: Arnaldo Carvalho de Melo, Gustavo A. R. Silva, Ian Rogers,
	Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, LKML, Gustavo A. R. Silva

Em Sat, May 16, 2020 at 03:35:48PM +0300, Adrian Hunter escreveu:
> On 15/05/20 7:41 pm, Arnaldo Carvalho de Melo wrote:
> > Sorry for the top post: Adrian, can you take a look at this?
> > 
> > Adrian Hunter was not CCed, Adrian?
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Sat, 16 May 2020 15:12:28 +0300
> Subject: [PATCH] perf intel-pt: Use allocated branch stack for PEBS sample
> 
> To avoid having struct branch_stack as a non-last structure member,
> use allocated branch stack for PEBS sample.

Thanks, applied.

- Arnaldo
 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/intel-pt.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/perf/util/intel-pt.c b/tools/perf/util/intel-pt.c
> index f17b1e769ae4..e4dd8bf610ce 100644
> --- a/tools/perf/util/intel-pt.c
> +++ b/tools/perf/util/intel-pt.c
> @@ -913,11 +913,11 @@ static void intel_pt_add_callchain(struct intel_pt *pt,
>  	sample->callchain = pt->chain;
>  }
>  
> -static struct branch_stack *intel_pt_alloc_br_stack(struct intel_pt *pt)
> +static struct branch_stack *intel_pt_alloc_br_stack(unsigned int entry_cnt)
>  {
>  	size_t sz = sizeof(struct branch_stack);
>  
> -	sz += pt->br_stack_sz * sizeof(struct branch_entry);
> +	sz += entry_cnt * sizeof(struct branch_entry);
>  	return zalloc(sz);
>  }
>  
> @@ -930,7 +930,7 @@ static int intel_pt_br_stack_init(struct intel_pt *pt)
>  			evsel->synth_sample_type |= PERF_SAMPLE_BRANCH_STACK;
>  	}
>  
> -	pt->br_stack = intel_pt_alloc_br_stack(pt);
> +	pt->br_stack = intel_pt_alloc_br_stack(pt->br_stack_sz);
>  	if (!pt->br_stack)
>  		return -ENOMEM;
>  
> @@ -951,6 +951,9 @@ static void intel_pt_add_br_stack(struct intel_pt *pt,
>  	sample->branch_stack = pt->br_stack;
>  }
>  
> +/* INTEL_PT_LBR_0, INTEL_PT_LBR_1 and INTEL_PT_LBR_2 */
> +#define LBRS_MAX (INTEL_PT_BLK_ITEM_ID_CNT * 3U)
> +
>  static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>  						   unsigned int queue_nr)
>  {
> @@ -968,8 +971,10 @@ static struct intel_pt_queue *intel_pt_alloc_queue(struct intel_pt *pt,
>  			goto out_free;
>  	}
>  
> -	if (pt->synth_opts.last_branch) {
> -		ptq->last_branch = intel_pt_alloc_br_stack(pt);
> +	if (pt->synth_opts.last_branch || pt->synth_opts.other_events) {
> +		unsigned int entry_cnt = max(LBRS_MAX, pt->br_stack_sz);
> +
> +		ptq->last_branch = intel_pt_alloc_br_stack(entry_cnt);
>  		if (!ptq->last_branch)
>  			goto out_free;
>  	}
> @@ -1720,9 +1725,6 @@ static void intel_pt_add_lbrs(struct branch_stack *br_stack,
>  	}
>  }
>  
> -/* INTEL_PT_LBR_0, INTEL_PT_LBR_1 and INTEL_PT_LBR_2 */
> -#define LBRS_MAX (INTEL_PT_BLK_ITEM_ID_CNT * 3)
> -
>  static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
>  {
>  	const struct intel_pt_blk_items *items = &ptq->state->items;
> @@ -1798,25 +1800,18 @@ static int intel_pt_synth_pebs_sample(struct intel_pt_queue *ptq)
>  	}
>  
>  	if (sample_type & PERF_SAMPLE_BRANCH_STACK) {
> -		struct {
> -			struct branch_stack br_stack;
> -			struct branch_entry entries[LBRS_MAX];
> -		} br;
> -
>  		if (items->mask[INTEL_PT_LBR_0_POS] ||
>  		    items->mask[INTEL_PT_LBR_1_POS] ||
>  		    items->mask[INTEL_PT_LBR_2_POS]) {
> -			intel_pt_add_lbrs(&br.br_stack, items);
> -			sample.branch_stack = &br.br_stack;
> +			intel_pt_add_lbrs(ptq->last_branch, items);
>  		} else if (pt->synth_opts.last_branch) {
>  			thread_stack__br_sample(ptq->thread, ptq->cpu,
>  						ptq->last_branch,
>  						pt->br_stack_sz);
> -			sample.branch_stack = ptq->last_branch;
>  		} else {
> -			br.br_stack.nr = 0;
> -			sample.branch_stack = &br.br_stack;
> +			ptq->last_branch->nr = 0;
>  		}
> +		sample.branch_stack = ptq->last_branch;
>  	}
>  
>  	if (sample_type & PERF_SAMPLE_ADDR && items->has_mem_access_address)
> -- 
> 2.17.1
> 
> 
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-05-20  1:42 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 23:47 [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample Gustavo A. R. Silva
2020-05-14  0:56 ` Ian Rogers
2020-05-14 13:10 ` Arnaldo Carvalho de Melo
2020-05-14 15:06   ` Gustavo A. R. Silva
2020-05-14 19:06     ` Ian Rogers
2020-05-14 22:09       ` Gustavo A. R. Silva
2020-05-14 22:46         ` Ian Rogers
2020-05-15  0:10           ` Gustavo A. R. Silva
2020-05-15  0:09             ` Ian Rogers
2020-05-15  0:29               ` Gustavo A. R. Silva
2020-05-15 16:41             ` Arnaldo Carvalho de Melo
2020-05-15 16:43               ` Arnaldo Carvalho de Melo
2020-05-15 17:09                 ` Gustavo A. R. Silva
2020-05-16 12:35               ` Adrian Hunter
2020-05-19 19:12                 ` Gustavo A. R. Silva
2020-05-20  1:42                 ` Arnaldo Carvalho de Melo

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