[PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Naveen N. Rao]

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 <naveen.n.rao@linux.vnet.ibm.com>
---
 tools/perf/builtin-record.c | 2 ++
 tools/perf/util/Build       | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index a01c8ae..de2aaac 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1095,9 +1095,11 @@ struct option __record_options[] = {
 		    "sample transaction flags (special events only)"),
 	OPT_BOOLEAN(0, "per-thread", &record.opts.target.per_thread,
 		    "use per-thread mmaps"),
+#ifdef HAVE_PERF_REGS_SUPPORT
 	OPT_CALLBACK_OPTARG('I', "intr-regs", &record.opts.sample_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", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
 	OPT_CALLBACK('k', "clockid", &record.opts,
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 4bc7a9a..cc3960c 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -84,7 +84,7 @@ libperf-$(CONFIG_AUXTRACE) += intel-pt-decoder/
 libperf-$(CONFIG_AUXTRACE) += intel-pt.o
 libperf-$(CONFIG_AUXTRACE) += intel-bts.o
 libperf-y += parse-branch-options.o
-libperf-y += parse-regs-options.o
+libperf-$(CONFIG_PERF_REGS) += parse-regs-options.o
 
 libperf-$(CONFIG_LIBELF) += symbol-elf.o
 libperf-$(CONFIG_LIBELF) += probe-file.o
-- 
2.5.3

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

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 <naveen.n.rao@linux.vnet.ibm.com>

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?


anyway this looks ok

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka


---
diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
index ff63649fa9ac..e5627b3d1bb8 100644
--- a/tools/perf/arch/x86/util/Build
+++ b/tools/perf/arch/x86/util/Build
@@ -2,7 +2,7 @@ libperf-y += header.o
 libperf-y += tsc.o
 libperf-y += pmu.o
 libperf-y += kvm-stat.o
-libperf-y += perf_regs.o
+libperf-$(CONFIG_PERF_REGS) += perf_regs.o
 
 libperf-$(CONFIG_DWARF) += dwarf-regs.o
 
diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
index 885e8ac83997..43168fb0d9a2 100644
--- a/tools/perf/util/perf_regs.c
+++ b/tools/perf/util/perf_regs.c
@@ -2,10 +2,6 @@
 #include "perf_regs.h"
 #include "event.h"
 
-const struct sample_reg __weak sample_reg_masks[] = {
-	SMPL_REG_END
-};
-
 int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
 {
 	int i, idx = 0;

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Stephane Eranian]

On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa <jolsa@redhat.com> wrote:
>
> 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 <naveen.n.rao@linux.vnet.ibm.com>
>
> 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?

>
>
> anyway this looks ok
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
> index ff63649fa9ac..e5627b3d1bb8 100644
> --- a/tools/perf/arch/x86/util/Build
> +++ b/tools/perf/arch/x86/util/Build
> @@ -2,7 +2,7 @@ libperf-y += header.o
>  libperf-y += tsc.o
>  libperf-y += pmu.o
>  libperf-y += kvm-stat.o
> -libperf-y += perf_regs.o
> +libperf-$(CONFIG_PERF_REGS) += perf_regs.o
>
>  libperf-$(CONFIG_DWARF) += dwarf-regs.o
>
> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c
> index 885e8ac83997..43168fb0d9a2 100644
> --- a/tools/perf/util/perf_regs.c
> +++ b/tools/perf/util/perf_regs.c
> @@ -2,10 +2,6 @@
>  #include "perf_regs.h"
>  #include "event.h"
>
> -const struct sample_reg __weak sample_reg_masks[] = {
> -       SMPL_REG_END
> -};
> -
>  int perf_reg_value(u64 *valp, struct regs_dump *regs, int id)
>  {
>         int i, idx = 0;

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

On Thu, Sep 24, 2015 at 08:32:12AM -0700, Stephane Eranian wrote:
> On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > 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 <naveen.n.rao@linux.vnet.ibm.com>
> >
> > 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?

sample_reg_masks is touched via parse-regs-options.c as well
and perf_regs.o depends on CONFIG_PERF_REGS

so with keeping the weak deffinition, how about attached change

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", &record.opts.target.per_thread,
 		    "use per-thread mmaps"),
+#ifdef HAVE_PERF_REGS_SUPPORT
 	OPT_CALLBACK_OPTARG('I', "intr-regs", &record.opts.sample_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", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
 	OPT_CALLBACK('k', "clockid", &record.opts,
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
 

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Naveen N. Rao]

On 2015/09/24 08:32AM, Stephane Eranian wrote:
> On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > 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 <naveen.n.rao@linux.vnet.ibm.com>
> >
> > 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

Regards,
Naveen

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Naveen N. Rao]

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 Olsa <jolsa@redhat.com> wrote:
> > >
> > > 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 <naveen.n.rao@linux.vnet.ibm.com>
> > >
> > > 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

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

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 Olsa <jolsa@redhat.com> wrote:
> > > >
> > > > 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 <naveen.n.rao@linux.vnet.ibm.com>
> > > >
> > > > 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", &record.opts.target.per_thread,
 		    "use per-thread mmaps"),
+#ifdef HAVE_PERF_REGS_SUPPORT
 	OPT_CALLBACK_OPTARG('I', "intr-regs", &record.opts.sample_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", &record.opts.running_time,
 		    "Record running/enabled time of read (:S) events"),
 	OPT_CALLBACK('k', "clockid", &record.opts,
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
 

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Naveen N. Rao]

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 Olsa <jolsa@redhat.com> wrote:
> > > > >
> > > > > 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 <naveen.n.rao@linux.vnet.ibm.com>
> > > > >
> > > > > 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

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

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

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Naveen N. Rao]

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

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

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

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Sukadev Bhattiprolu]

Jiri Olsa [jolsa@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 Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: 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 <sukadev@linux.vnet.ibm.com>
---
 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 <linux/types.h>
 
+#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

Re: [PATCH] perf record: Limit --intr-regs to platforms supporting PERF_REGS

[Jiri Olsa]

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 <linux/types.h>
>  
> +#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