Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On 2015/09/29 08:53AM, Jiri Olsa wrote: > On Tue, Sep 29, 2015 at 11:06:17AM +0530, Naveen N. Rao wrote: > > On 2015/09/24 10:15PM, Naveen N Rao wrote: > > > On 2015/09/24 08:32AM, Stephane Eranian wrote: > > > > On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsawrote: > > > > > > > > > > On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote: > > > > > > perf build currently fails on powerpc: > > > > > > > > > > > > LINK perf > > > > > > libperf.a(libperf-in.o):(.toc+0x120): undefined reference to > > > > > > `sample_reg_masks' > > > > > > libperf.a(libperf-in.o):(.toc+0x130): undefined reference to > > > > > > `sample_reg_masks' > > > > > > collect2: error: ld returned 1 exit status > > > > > > make[1]: *** [perf] Error 1 > > > > > > make: *** [all] Error 2 > > > > > > > > > > > > This is due to parse-regs-options.c using sample_reg_masks, which is > > > > > > defined only with CONFIG_PERF_REGS. > > > > > > > > > > > > In addition, perf record -I is only useful if the arch supports > > > > > > PERF_REGS. Hence, let's expose -I conditionally. > > > > > > > > > > > > Signed-off-by: Naveen N. Rao > > > > > > > > > > hum, I wonder why we have sample_reg_masks defined as weak in > > > > > util/perf_regs.c > > > > > which is also built only via CONFIG_PERF_REGS > > > > > > > > > > I wonder we could get rid of the weak definition via attached patch, > > > > > Stephane? > > > > > > > > > But the whole point of having it weak is to avoid this error scenario > > > > on any arch without support > > > > and avoid ugly #ifdef HAVE_ in generic files. > > > > > > > > if perf_regs.c is compiled on PPC, then why do we get the undefined? > > > > > > As Jiri Olsa pointed out, powerpc and many other architectures don't > > > (yet) have support for perf regs. > > > > > > But, the larger reason to introduce #ifdef is so the user doesn't see > > > options (s)he can't use on a specific architecture, along the same lines > > > as builtin-probe.c > > > > Stephane, Arnaldo, > > Suka has also posted a fix for this with a different approach [1]. Can > > you please ack/pull one of these versions? Building perf is broken on > > v4.3-rc due to this. > > I did not get any answer for additional comments I made to the patch > (couldnt get marc.info working, sending the patch again) Hi Jiri, I concur with the changes you proposed to my patch here (getting rid of the weak variant): http://article.gmane.org/gmane.linux.kernel/2046108 I am aware of the other approach you posted (and the one attached below). When I said "please ack/pull one of these versions", I meant one of: your version, Suka's and mine. > > > > > [1] http://article.gmane.org/gmane.linux.kernel/2046370 > > I dont have this last version, which seems to have other changes > and patch in above link looks mangled, could you please repost it? Can you please check the raw version: http://article.gmane.org/gmane.linux.kernel/2046370/raw Thanks, Naveen ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On Tue, Sep 29, 2015 at 11:06:17AM +0530, Naveen N. Rao wrote: > On 2015/09/24 10:15PM, Naveen N Rao wrote: > > On 2015/09/24 08:32AM, Stephane Eranian wrote: > > > On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsawrote: > > > > > > > > On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote: > > > > > perf build currently fails on powerpc: > > > > > > > > > > LINK perf > > > > > libperf.a(libperf-in.o):(.toc+0x120): undefined reference to > > > > > `sample_reg_masks' > > > > > libperf.a(libperf-in.o):(.toc+0x130): undefined reference to > > > > > `sample_reg_masks' > > > > > collect2: error: ld returned 1 exit status > > > > > make[1]: *** [perf] Error 1 > > > > > make: *** [all] Error 2 > > > > > > > > > > This is due to parse-regs-options.c using sample_reg_masks, which is > > > > > defined only with CONFIG_PERF_REGS. > > > > > > > > > > In addition, perf record -I is only useful if the arch supports > > > > > PERF_REGS. Hence, let's expose -I conditionally. > > > > > > > > > > Signed-off-by: Naveen N. Rao > > > > > > > > hum, I wonder why we have sample_reg_masks defined as weak in > > > > util/perf_regs.c > > > > which is also built only via CONFIG_PERF_REGS > > > > > > > > I wonder we could get rid of the weak definition via attached patch, > > > > Stephane? > > > > > > > But the whole point of having it weak is to avoid this error scenario > > > on any arch without support > > > and avoid ugly #ifdef HAVE_ in generic files. > > > > > > if perf_regs.c is compiled on PPC, then why do we get the undefined? > > > > As Jiri Olsa pointed out, powerpc and many other architectures don't > > (yet) have support for perf regs. > > > > But, the larger reason to introduce #ifdef is so the user doesn't see > > options (s)he can't use on a specific architecture, along the same lines > > as builtin-probe.c > > Stephane, Arnaldo, > Suka has also posted a fix for this with a different approach [1]. Can > you please ack/pull one of these versions? Building perf is broken on > v4.3-rc due to this. I did not get any answer for additional comments I made to the patch (couldnt get marc.info working, sending the patch again) > > [1] http://article.gmane.org/gmane.linux.kernel/2046370 I dont have this last version, which seems to have other changes and patch in above link looks mangled, could you please repost it? thanks, jirka --- diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 142eeb341b29..19c8fd22fbe3 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -1082,9 +1082,11 @@ struct option __record_options[] = { "sample transaction flags (special events only)"), OPT_BOOLEAN(0, "per-thread", _thread, "use per-thread mmaps"), +#ifdef HAVE_PERF_REGS_SUPPORT OPT_CALLBACK_OPTARG('I', "intr-regs", _intr_regs, NULL, "any register", "sample selected machine registers on interrupt," " use -I ? to list register names", parse_regs), +#endif OPT_BOOLEAN(0, "running-time", _time, "Record running/enabled time of read (:S) events"), OPT_CALLBACK('k', "clockid", , diff --git a/tools/perf/util/Build b/tools/perf/util/Build index 4bc7a9ab45b1..93c6371405a3 100644 --- a/tools/perf/util/Build +++ b/tools/perf/util/Build @@ -104,7 +104,7 @@ libperf-$(CONFIG_LIBBABELTRACE) += data-convert-bt.o libperf-y += scripting-engines/ -libperf-$(CONFIG_PERF_REGS) += perf_regs.o +libperf-y += perf_regs.o libperf-$(CONFIG_ZLIB) += zlib.o libperf-$(CONFIG_LZMA) += lzma.o ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote: SNIP > > > Suka has also posted a fix for this with a different approach [1]. Can > > > you please ack/pull one of these versions? Building perf is broken on > > > v4.3-rc due to this. > > > > I did not get any answer for additional comments I made to the patch > > (couldnt get marc.info working, sending the patch again) > > Hi Jiri, > I concur with the changes you proposed to my patch here (getting rid of > the weak variant): > http://article.gmane.org/gmane.linux.kernel/2046108 > > I am aware of the other approach you posted (and the one attached > below). When I said "please ack/pull one of these versions", I meant one > of: your version, Suka's and mine. I was hoping somebody could test it on ppc ;-) I think the last version (in my last email) that keeps the weak variable is correct, let's wait for Arnaldo to sort this out > > > > > > > > > [1] http://article.gmane.org/gmane.linux.kernel/2046370 > > > > I dont have this last version, which seems to have other changes > > and patch in above link looks mangled, could you please repost it? > > Can you please check the raw version: > http://article.gmane.org/gmane.linux.kernel/2046370/raw we have __maybe_unused definition in tools/include/linux/compiler.h why to redeclare it? jirka ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On Tue, Sep 29, 2015 at 11:10:02AM -0700, Sukadev Bhattiprolu wrote: SNIP > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index 885e8ac..6b8eb13 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -6,6 +6,7 @@ const struct sample_reg __weak sample_reg_masks[] = { > SMPL_REG_END > }; > > +#ifdef HAVE_PERF_REGS_SUPPORT > int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > { > int i, idx = 0; > @@ -29,3 +30,4 @@ out: > *valp = regs->cache_regs[id]; > return 0; > } > +#endif > diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h > index 2984dcc..8dbdfeb 100644 > --- a/tools/perf/util/perf_regs.h > +++ b/tools/perf/util/perf_regs.h > @@ -3,6 +3,10 @@ > > #include > > +#ifndef __maybe_unused > +#define __maybe_unused __attribute__((unused)) > +#endif > + would the linux/compiler.h include do instead? otherwise I'd be ok with this thanks, jirka ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On Tue, Sep 29, 2015 at 10:01:36PM +0530, Naveen N. Rao wrote: > On 2015/09/29 12:47PM, Jiri Olsa wrote: > > On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote: > > > > SNIP > > > > > > > Suka has also posted a fix for this with a different approach [1]. > > > > > Can > > > > > you please ack/pull one of these versions? Building perf is broken on > > > > > v4.3-rc due to this. > > > > > > > > I did not get any answer for additional comments I made to the patch > > > > (couldnt get marc.info working, sending the patch again) > > > > > > Hi Jiri, > > > I concur with the changes you proposed to my patch here (getting rid of > > > the weak variant): > > > http://article.gmane.org/gmane.linux.kernel/2046108 > > > > > > I am aware of the other approach you posted (and the one attached > > > below). When I said "please ack/pull one of these versions", I meant one > > > of: your version, Suka's and mine. > > > > I was hoping somebody could test it on ppc ;-) > > > > I think the last version (in my last email) that keeps the weak > > variable is correct, let's wait for Arnaldo to sort this out > > I just tried it, but it fails. As Suka points out in his patch: > "Adding perf_regs.o to util/Build unconditionally, exposes a > redefinition error for 'perf_reg_value()' function (due to the static > inline version in util/perf_regs.h). So use #ifdef > HAVE_PERF_REGS_SUPPORT' around that function." could you (or Suka) please reply in here with the patch? thanks, jirka ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On 2015/09/29 12:47PM, Jiri Olsa wrote: > On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote: > > SNIP > > > > > Suka has also posted a fix for this with a different approach [1]. Can > > > > you please ack/pull one of these versions? Building perf is broken on > > > > v4.3-rc due to this. > > > > > > I did not get any answer for additional comments I made to the patch > > > (couldnt get marc.info working, sending the patch again) > > > > Hi Jiri, > > I concur with the changes you proposed to my patch here (getting rid of > > the weak variant): > > http://article.gmane.org/gmane.linux.kernel/2046108 > > > > I am aware of the other approach you posted (and the one attached > > below). When I said "please ack/pull one of these versions", I meant one > > of: your version, Suka's and mine. > > I was hoping somebody could test it on ppc ;-) > > I think the last version (in my last email) that keeps the weak > variable is correct, let's wait for Arnaldo to sort this out I just tried it, but it fails. As Suka points out in his patch: "Adding perf_regs.o to util/Build unconditionally, exposes a redefinition error for 'perf_reg_value()' function (due to the static inline version in util/perf_regs.h). So use #ifdef HAVE_PERF_REGS_SUPPORT' around that function." - Naveen ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
Jiri Olsa [jo...@redhat.com] wrote: | > I just tried it, but it fails. As Suka points out in his patch: | > "Adding perf_regs.o to util/Build unconditionally, exposes a | > redefinition error for 'perf_reg_value()' function (due to the static | > inline version in util/perf_regs.h). So use #ifdef | > HAVE_PERF_REGS_SUPPORT' around that function." | | could you (or Suka) please reply in here with the patch? Jiri, Do you mean this patch? I was planning on pinging Arnaldo again in a couple of days about this patch, since the powerpc build is broken. Sukadev --- From d1171a4c34c6100ec8b663ddb803dd69ef3fb7ce Mon Sep 17 00:00:00 2001 From: Sukadev BhattiproluDate: Thu, 24 Sep 2015 17:53:49 -0400 Subject: [PATCH] perf: Fix build break on powerpc due to sample_reg_masks perf_regs.c does not get built on Powerpc as CONFIG_PERF_REGS is false. So the weak definition for 'sample_regs_masks' doesn't get picked up. Adding perf_regs.o to util/Build unconditionally, exposes a redefinition error for 'perf_reg_value()' function (due to the static inline version in util/perf_regs.h). So use #ifdef HAVE_PERF_REGS_SUPPORT' around that function. Signed-off-by: Sukadev Bhattiprolu --- tools/perf/util/Build | 2 +- tools/perf/util/perf_regs.c | 2 ++ tools/perf/util/perf_regs.h | 4 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/Build b/tools/perf/util/Build index 349bc96..e5f18a2 100644 --- a/tools/perf/util/Build +++ b/tools/perf/util/Build @@ -17,6 +17,7 @@ libperf-y += levenshtein.o libperf-y += llvm-utils.o libperf-y += parse-options.o libperf-y += parse-events.o +libperf-y += perf_regs.o libperf-y += path.o libperf-y += rbtree.o libperf-y += bitmap.o @@ -103,7 +104,6 @@ libperf-$(CONFIG_LIBBABELTRACE) += data-convert-bt.o libperf-y += scripting-engines/ -libperf-$(CONFIG_PERF_REGS) += perf_regs.o libperf-$(CONFIG_ZLIB) += zlib.o libperf-$(CONFIG_LZMA) += lzma.o diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c index 885e8ac..6b8eb13 100644 --- a/tools/perf/util/perf_regs.c +++ b/tools/perf/util/perf_regs.c @@ -6,6 +6,7 @@ const struct sample_reg __weak sample_reg_masks[] = { SMPL_REG_END }; +#ifdef HAVE_PERF_REGS_SUPPORT int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) { int i, idx = 0; @@ -29,3 +30,4 @@ out: *valp = regs->cache_regs[id]; return 0; } +#endif diff --git a/tools/perf/util/perf_regs.h b/tools/perf/util/perf_regs.h index 2984dcc..8dbdfeb 100644 --- a/tools/perf/util/perf_regs.h +++ b/tools/perf/util/perf_regs.h @@ -3,6 +3,10 @@ #include +#ifndef __maybe_unused +#define __maybe_unused __attribute__((unused)) +#endif + struct regs_dump; struct sample_reg { -- 1.8.3.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS
On 2015/09/24 10:15PM, Naveen N Rao wrote: > On 2015/09/24 08:32AM, Stephane Eranian wrote: > > On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsawrote: > > > > > > On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote: > > > > perf build currently fails on powerpc: > > > > > > > > LINK perf > > > > libperf.a(libperf-in.o):(.toc+0x120): undefined reference to > > > > `sample_reg_masks' > > > > libperf.a(libperf-in.o):(.toc+0x130): undefined reference to > > > > `sample_reg_masks' > > > > collect2: error: ld returned 1 exit status > > > > make[1]: *** [perf] Error 1 > > > > make: *** [all] Error 2 > > > > > > > > This is due to parse-regs-options.c using sample_reg_masks, which is > > > > defined only with CONFIG_PERF_REGS. > > > > > > > > In addition, perf record -I is only useful if the arch supports > > > > PERF_REGS. Hence, let's expose -I conditionally. > > > > > > > > Signed-off-by: Naveen N. Rao > > > > > > hum, I wonder why we have sample_reg_masks defined as weak in > > > util/perf_regs.c > > > which is also built only via CONFIG_PERF_REGS > > > > > > I wonder we could get rid of the weak definition via attached patch, > > > Stephane? > > > > > But the whole point of having it weak is to avoid this error scenario > > on any arch without support > > and avoid ugly #ifdef HAVE_ in generic files. > > > > if perf_regs.c is compiled on PPC, then why do we get the undefined? > > As Jiri Olsa pointed out, powerpc and many other architectures don't > (yet) have support for perf regs. > > But, the larger reason to introduce #ifdef is so the user doesn't see > options (s)he can't use on a specific architecture, along the same lines > as builtin-probe.c Stephane, Arnaldo, Suka has also posted a fix for this with a different approach [1]. Can you please ack/pull one of these versions? Building perf is broken on v4.3-rc due to this. [1] http://article.gmane.org/gmane.linux.kernel/2046370 Thanks, Naveen ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev