* [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 related [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-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: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-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 related [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).