[PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Ian Rogers]

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
>

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Arnaldo Carvalho de Melo]

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Ian Rogers]

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Ian Rogers]

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
>

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Ian Rogers]

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Arnaldo Carvalho de Melo]

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Arnaldo Carvalho de Melo]

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Adrian Hunter]

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

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

Re: [PATCH perf/core] perf intel-pt: Fix clang build failure in intel_pt_synth_pebs_sample

[Arnaldo Carvalho de Melo]

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