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:

quoted

On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa [off-list ref] wrote:

quoted

On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote:

quoted

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

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

On Tue, Sep 29, 2015 at 11:06:17AM +0530, Naveen N. Rao wrote:

On 2015/09/24 10:15PM, Naveen N Rao wrote:

quoted

On 2015/09/24 08:32AM, Stephane Eranian wrote:

quoted

On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa [off-list ref] wrote:

quoted

On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote:

quoted

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

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
 

On 2015/09/29 08:53AM, Jiri Olsa wrote:

On Tue, Sep 29, 2015 at 11:06:17AM +0530, Naveen N. Rao wrote:

quoted

On 2015/09/24 10:15PM, Naveen N Rao wrote:

quoted

On 2015/09/24 08:32AM, Stephane Eranian wrote:

quoted

On Thu, Sep 24, 2015 at 5:57 AM, Jiri Olsa [off-list ref] wrote:

quoted

On Thu, Sep 24, 2015 at 05:41:58PM +0530, Naveen N. Rao wrote:

quoted

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

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.

quoted

[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

On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote:

SNIP

quoted

quoted

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

quoted

quoted

[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

On 2015/09/29 12:47PM, Jiri Olsa wrote:

On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote:

SNIP

quoted

quoted

quoted

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

On Tue, Sep 29, 2015 at 10:01:36PM +0530, Naveen N. Rao wrote:

On 2015/09/29 12:47PM, Jiri Olsa wrote:

quoted

On Tue, Sep 29, 2015 at 01:30:10PM +0530, Naveen N. Rao wrote:

SNIP

quoted

quoted

quoted

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

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

On Tue, Sep 29, 2015 at 11:10:02AM -0700, Sukadev Bhattiprolu wrote:

SNIP

would the linux/compiler.h include do instead?

otherwise I'd be ok with this

thanks,
jirka