Skip to contentSkip to conversation

perf: fix wrong DEBUG configuration

PATCH In review Martin Liška 4 participants 26 messages May 19, 2015 – May 27, 2015 perf events

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 perf: fix wrong DEBUG configuration

Following patch is fix for wrong DEBUG configuration.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/config/Makefile          | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +1 -1
index 59a98c6..b708ae0 100644
@@ -129,7 +129,7 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -Og
 endif
 
 ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 201f6c4c..29792d2 100644
@@ -397,7 +397,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#1 lore raw

Em Tue, May 19, 2015 at 03:41:03PM +0200, Martin Liška escreveu:

Quoted 1 line

Following patch is fix for wrong DEBUG configuration.

Can you please state, in the changelog, what you think is wrong and how your proposed change fixes it, so that after reading your intent I can go and look at the code to see if it matches that?

- Arnaldo

Quoted 57 lines
---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/config/Makefile          | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
tools/perf/config/Makefile +1 -1
index 59a98c6..b708ae0 100644
@@ -129,7 +129,7 @@ ifndef DEBUG
 endif
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -Og
 endif
 ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 201f6c4c..29792d2 100644
@@ -397,7 +397,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#2 lore raw
Quoted 58 lines · Arnaldo Carvalho de Melo

Em Tue, May 19, 2015 at 03:41:03PM +0200, Martin Liška escreveu:

Quoted 1 line

Following patch is fix for wrong DEBUG configuration.

Can you please state, in the changelog, what you think is wrong and how your proposed change fixes it, so that after reading your intent I can go and look at the code to see if it matches that?

- Arnaldo

Quoted 49 lines
---
  tools/perf/arch/common.c            | 2 +-
  tools/perf/config/Makefile          | 2 +-
  tools/perf/util/symbol.c            | 2 +-
  tools/perf/util/trace-event-parse.c | 2 +-
  4 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
  static bool lookup_path(char *name)
  {
  	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
  	char buf[PATH_MAX];
  	char *env = getenv("PATH");
tools/perf/config/Makefile +1 -1
index 59a98c6..b708ae0 100644
@@ -129,7 +129,7 @@ ifndef DEBUG
  endif
  ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -Og
  endif
  ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 201f6c4c..29792d2 100644
@@ -397,7 +397,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
  					    const char *name)
  {
  	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
  	if (symbols == NULL)
  		return NULL;
tools/perf/util/trace-event-parse.c +0 -0
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
  	char *line;
Quoted 8 lines
Quoted 8 lines
tools/perf/util/trace-event-parse.c +1 -2
  	char *next = NULL;
  	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
  	line = strtok_r(file, "\n", &next);
  	while (line) {
--

2.1.4

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.
Assign default value for pointers that are identified by compiler as non-initialized.

Quoted 1 line

From b5f2b42b279ab497e92d41e1f4021dd5cb6ae3ce Mon Sep 17 00:00:00 2001

From: mliska <[email protected]>
Date: Thu, 2 Apr 2015 15:24:49 +0200
Subject: [PATCH] perf: fix wrong DEBUG configuration

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.
Assign default value for pointers that are identified by compiler as non-initialized.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/config/Makefile          | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +1 -1
index e3b3724..ca8d3e3 100644
@@ -129,7 +129,7 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -Og
 endif
 
 ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#3 lore raw

Em Wed, May 20, 2015 at 03:07:39PM +0200, Martin Liška escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo
Quoted 6 lines

Em Tue, May 19, 2015 at 03:41:03PM +0200, Martin Liška escreveu:

Quoted 1 line

Following patch is fix for wrong DEBUG configuration.

Can you please state, in the changelog, what you think is wrong and how your proposed change fixes it, so that after reading your intent I can go and look at the code to see if it matches that?

Ok, that improved things, thanks! A question:

Quoted 3 lines

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.

Is this is this -Og available in old gcc versions? When was it introduced? Do you know?

- Arnaldo

Quoted 78 lines

Assign default value for pointers that are identified by compiler as non-initialized.

Quoted 1 line

From b5f2b42b279ab497e92d41e1f4021dd5cb6ae3ce Mon Sep 17 00:00:00 2001

From: mliska <[email protected]>
Date: Thu, 2 Apr 2015 15:24:49 +0200
Subject: [PATCH] perf: fix wrong DEBUG configuration

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.
Assign default value for pointers that are identified by compiler as non-initialized.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/config/Makefile          | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +1 -1
index e3b3724..ca8d3e3 100644
@@ -129,7 +129,7 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -Og
 endif
 
 ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#4 lore raw
Quoted 12 lines · Arnaldo Carvalho de Melo

Em Wed, May 20, 2015 at 03:07:39PM +0200, Martin Liška escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo
Quoted 6 lines

Em Tue, May 19, 2015 at 03:41:03PM +0200, Martin Liška escreveu:

Quoted 1 line

Following patch is fix for wrong DEBUG configuration.

Can you please state, in the changelog, what you think is wrong and how your proposed change fixes it, so that after reading your intent I can go and look at the code to see if it matches that?

Ok, that improved things, thanks! A question:

Quoted 1 line
Quoted 7 lines
Quoted 2 lines

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.

Is this is this -Og available in old gcc versions? When was it introduced? Do you know?

- Arnaldo

Hi

GCC 4.8.0 is the first version capable of the option: https://gcc.gnu.org/gcc-4.8/changes.html.
That can be problematic, which GCC version do you support in linux/perf?

Martin

Quoted 80 lines
Quoted 79 lines

Assign default value for pointers that are identified by compiler as non-initialized.

Quoted 1 line

From b5f2b42b279ab497e92d41e1f4021dd5cb6ae3ce Mon Sep 17 00:00:00 2001

From: mliska <[email protected]>
Date: Thu, 2 Apr 2015 15:24:49 +0200
Subject: [PATCH] perf: fix wrong DEBUG configuration

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.
Assign default value for pointers that are identified by compiler as non-initialized.

---
  tools/perf/arch/common.c            | 2 +-
  tools/perf/config/Makefile          | 2 +-
  tools/perf/util/symbol.c            | 2 +-
  tools/perf/util/trace-event-parse.c | 2 +-
  4 files changed, 4 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
  static bool lookup_path(char *name)
  {
  	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
  	char buf[PATH_MAX];
  	char *env = getenv("PATH");
 
tools/perf/config/Makefile +1 -1
index e3b3724..ca8d3e3 100644
@@ -129,7 +129,7 @@ ifndef DEBUG
  endif
 
  ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -Og
  endif
 
  ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
  					    const char *name)
  {
  	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
  	if (symbols == NULL)
  		return NULL;
tools/perf/util/trace-event-parse.c +1 -2
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
  	char *line;
  	char *next = NULL;
  	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
  	line = strtok_r(file, "\n", &next);
  	while (line) {
--

2.1.4

#5 lore raw

Em Wed, May 20, 2015 at 03:29:37PM +0200, Martin Liška escreveu:

Quoted 10 lines · Arnaldo Carvalho de Melo
Quoted 6 lines

Em Wed, May 20, 2015 at 03:07:39PM +0200, Martin Liška escreveu:

Quoted 2 lines

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.

Is this is this -Og available in old gcc versions? When was it introduced? Do you know?

GCC 4.8.0 is the first version capable of the option: https://gcc.gnu.org/gcc-4.8/changes.html.
That can be problematic, which GCC version do you support in linux/perf?

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

Looking at the kernel's README, that is...

-------------

COMPILING the kernel:

- Make sure you have at least gcc 3.2 available. For more information, refer to Documentation/Changes.

--------------

The oldest toolchain I personally test from time to time is:

[acme@rhel5 ~]$ rpm -q gcc make binutils
gcc-4.1.2-55.el5
make-3.81-3.el5
binutils-2.17.50.0.6-26.el5
[acme@rhel5 ~]$

- Arnaldo

#6 lore raw
Quoted 6 lines · Arnaldo Carvalho de Melo

Em Wed, May 20, 2015 at 03:29:37PM +0200, Martin Liška escreveu:

Quoted 4 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Em Wed, May 20, 2015 at 03:07:39PM +0200, Martin Liška escreveu:

Quoted 2 lines

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Is this is this -Og available in old gcc versions? When was it introduced? Do you know?

GCC 4.8.0 is the first version capable of the option: https://gcc.gnu.org/gcc-4.8/changes.html.
That can be problematic, which GCC version do you support in linux/perf?

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

So we could use -Og if it works, like Kbuild does it:

        KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387)

the 'cc-option' Make function does some magic of silently calling GCC with that option and observing the result.

See:

  scripts/Kbuild.include:cc-option = $(call try-run,\

et al.

Thanks,

Ingo

#7 lore raw
Quoted 27 lines · Ingo Molnar
Quoted 6 lines · Arnaldo Carvalho de Melo

Em Wed, May 20, 2015 at 03:29:37PM +0200, Martin Liška escreveu:

Quoted 4 lines · Arnaldo Carvalho de Melo
Quoted 3 lines

Em Wed, May 20, 2015 at 03:07:39PM +0200, Martin Liška escreveu:

Quoted 2 lines

Currently, GCC optimizes -O6 same as -O3 level. Right optimize debugging experience is given by passing -Og to compiler.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Is this is this -Og available in old gcc versions? When was it introduced? Do you know?

GCC 4.8.0 is the first version capable of the option: https://gcc.gnu.org/gcc-4.8/changes.html.
That can be problematic, which GCC version do you support in linux/perf?

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

So we could use -Og if it works, like Kbuild does it:

         KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387)

the 'cc-option' Make function does some magic of silently calling GCC with that option and observing the result.

See:

Quoted 8 lines
   scripts/Kbuild.include:cc-option = $(call try-run,\

et al.

Thanks,

Ingo

Hi.

I am sorry, I did mistake in understanding of DEBUG variable. Following patch should be fixed, except missing auto-detection of -Og option.

Unfortunately, following hunk does not work, no -Ox is added to CFLAGS?

-- CFLAGS += -Og
++ CFLAGS += $(call cc-option,-Og,-O0)

Thanks,
Martin

Quoted 1 line

From 98ef10fcb27afc3ae7aaab3ebd157ab90b3b1afe Mon Sep 17 00:00:00 2001

From: mliska <[email protected]>
Date: Thu, 2 Apr 2015 15:24:49 +0200
Subject: [PATCH] perf: fix wrong DEBUG configuration

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O6.
Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/config/Makefile          | 4 +++-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 4 files changed, 6 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +3 -1
index e3b3724..6a29320 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += -Og
 endif
 
 ifdef PARSER_DEBUG
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#8 lore raw

Em Wed, May 20, 2015 at 06:16:51PM +0200, Martin Liška escreveu:

Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

So we could use -Og if it works, like Kbuild does it:

Quoted 8 lines
        KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387)

the 'cc-option' Make function does some magic of silently calling GCC with that option and observing the result.

See:

  scripts/Kbuild.include:cc-option = $(call try-run,\
Quoted 8 lines

I am sorry, I did mistake in understanding of DEBUG variable. Following patch should be fixed, except missing auto-detection of -Og option.

Unfortunately, following hunk does not work, no -Ox is added to CFLAGS?

-- CFLAGS += -Og
++ CFLAGS += $(call cc-option,-Og,-O0)

I don't know if we have this cc-option, perhaps Ingo is suggesting we get it in tools/build/? Or include scripts/Kbuild.include and then use it?

I.e. we have checks to see if we can use, for instance -fstack-protector-all, see tools/build/feature/Makefile, usong this cc-option thing, importing it from Kbuild would solve the issue at hand in a definitive way and in line with what we have been pursuing:
to make the tools/ build system be based on Kbuild.

- Arnaldo

#9 lore raw
Quoted 30 lines · Arnaldo Carvalho de Melo

Em Wed, May 20, 2015 at 06:16:51PM +0200, Martin Liška escreveu:

Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

So we could use -Og if it works, like Kbuild does it:

Quoted 8 lines
        KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387)

the 'cc-option' Make function does some magic of silently calling GCC with that option and observing the result.

See:

  scripts/Kbuild.include:cc-option = $(call try-run,\
Quoted 8 lines

I am sorry, I did mistake in understanding of DEBUG variable. Following patch should be fixed, except missing auto-detection of -Og option.

Unfortunately, following hunk does not work, no -Ox is added to CFLAGS?

-- CFLAGS += -Og
++ CFLAGS += $(call cc-option,-Og,-O0)

I don't know if we have this cc-option, perhaps Ingo is suggesting we get it in tools/build/? Or include scripts/Kbuild.include and then use it?

Quoted 5 lines

I.e. we have checks to see if we can use, for instance -fstack-protector-all, see tools/build/feature/Makefile, using this cc-option thing, importing it from Kbuild would solve the issue at hand in a definitive way and in line with what we have been
pursuing: to make the tools/ build system be based on Kbuild.

So I'd suggest copying any necessary functions instead of outright including all of Kbuild in the tooling build system which creates non-trivial dependencies that is not necessarily tested as thoroughly on the kbuild side as on the tooling side.

Thanks,

Ingo

#10 lore raw
Quoted 39 lines · Ingo Molnar
Quoted 30 lines · Arnaldo Carvalho de Melo

Em Wed, May 20, 2015 at 06:16:51PM +0200, Martin Liška escreveu:

Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

So we could use -Og if it works, like Kbuild does it:

Quoted 8 lines
         KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387)

the 'cc-option' Make function does some magic of silently calling GCC with that option and observing the result.

See:

   scripts/Kbuild.include:cc-option = $(call try-run,\
Quoted 8 lines

I am sorry, I did mistake in understanding of DEBUG variable. Following patch should be fixed, except missing auto-detection of -Og option.

Unfortunately, following hunk does not work, no -Ox is added to CFLAGS?

-- CFLAGS += -Og
++ CFLAGS += $(call cc-option,-Og,-O0)

I don't know if we have this cc-option, perhaps Ingo is suggesting we get it in tools/build/? Or include scripts/Kbuild.include and then use it?

Quoted 5 lines

I.e. we have checks to see if we can use, for instance -fstack-protector-all, see tools/build/feature/Makefile, using this cc-option thing, importing it from Kbuild would solve the issue at hand in a definitive way and in line with what we have been
pursuing: to make the tools/ build system be based on Kbuild.

Quoted 9 lines

So I'd suggest copying any necessary functions instead of outright including all of Kbuild in the tooling build system which creates non-trivial dependencies that is not necessarily tested as thoroughly on the kbuild side as on the tooling side.

Thanks,

Ingo

May I ask you Ingo for helping with that? It would be easy to test if you grab my patch and add necessary bash functions.

Thank you,
Martin

#11 lore raw
In reply to Ingo Molnar
Quoted 39 lines · Ingo Molnar
Quoted 30 lines · Arnaldo Carvalho de Melo

Em Wed, May 20, 2015 at 06:16:51PM +0200, Martin Liška escreveu:

Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

So the rule has been: What are the kernel requirements for the toolchain? tools/perf/ should build with that.

So we could use -Og if it works, like Kbuild does it:

Quoted 8 lines
         KBUILD_CFLAGS += $(call cc-option,-mno-fp-ret-in-387)

the 'cc-option' Make function does some magic of silently calling GCC with that option and observing the result.

See:

   scripts/Kbuild.include:cc-option = $(call try-run,\
Quoted 8 lines

I am sorry, I did mistake in understanding of DEBUG variable. Following patch should be fixed, except missing auto-detection of -Og option.

Unfortunately, following hunk does not work, no -Ox is added to CFLAGS?

-- CFLAGS += -Og
++ CFLAGS += $(call cc-option,-Og,-O0)

I don't know if we have this cc-option, perhaps Ingo is suggesting we get it in tools/build/? Or include scripts/Kbuild.include and then use it?

Quoted 5 lines

I.e. we have checks to see if we can use, for instance -fstack-protector-all, see tools/build/feature/Makefile, using this cc-option thing, importing it from Kbuild would solve the issue at hand in a definitive way and in line with what we have been
pursuing: to make the tools/ build system be based on Kbuild.

Quoted 9 lines

So I'd suggest copying any necessary functions instead of outright including all of Kbuild in the tooling build system which creates non-trivial dependencies that is not necessarily tested as thoroughly on the kbuild side as on the tooling side.

Thanks,

Ingo

Hi.

Thanks for advice, I copied necessary functions to include/utilities.mak. Following patch works for, tested both with a compiler capable of -Og and a not capable one.

Thanks,
Martin

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O6.
Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

---
 tools/perf/arch/common.c            |  2 +-
 tools/perf/config/Makefile          |  4 +++-
 tools/perf/config/utilities.mak     | 19 +++++++++++++++++++
 tools/perf/util/symbol.c            |  2 +-
 tools/perf/util/trace-event-parse.c |  2 +-
 5 files changed, 25 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
Acked-by
#12 lore raw
Quoted 2 lines · Martin Liška

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O6.

s/to -O6
to -O3

Quoted 3 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

s/identified by compiler
identified by the compiler

Quoted 15 lines

ifeq ($(DEBUG),0)

(quoted diff) +11 -1
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\

Review

Quoted 10 lines
(quoted diff) +10 -0
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))

Looks good to me!

Thanks,

Ingo

author Acked-by Ingo Molnar
#13 lore raw
Quoted 47 lines · Ingo Molnar
Quoted 2 lines · Martin Liška

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O6.

s/to -O6
to -O3

Quoted 3 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

s/identified by compiler
identified by the compiler

Quoted 15 lines
  ifeq ($(DEBUG),0)
(quoted diff) +11 -1
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
  endif
 
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\

Review

Quoted 10 lines
(quoted diff) +10 -0
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))

Looks good to me!

Quoted 6 lines

Thanks,

Ingo

Thank you for review.

This is final version of the patch, where I appended your acknowledgment.

Martin

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O3.
Right debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by the compiler as non-initialized.

---
 tools/perf/arch/common.c            |  2 +-
 tools/perf/config/Makefile          |  4 +++-
 tools/perf/config/utilities.mak     | 19 +++++++++++++++++++
 tools/perf/util/symbol.c            |  2 +-
 tools/perf/util/trace-event-parse.c |  2 +-
 5 files changed, 25 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#14 lore raw
Quoted 8 lines · Martin Liška
Quoted 6 lines
Quoted 3 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Or you can use what I suggested first:

Correct debugging experience is given by ...

Thanks,

Ingo

author Acked-by Ingo Molnar
#15 lore raw
Quoted 17 lines · Ingo Molnar
Quoted 8 lines · Martin Liška
Quoted 6 lines
Quoted 3 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Quoted 8 lines

Or you can use what I suggested first:

Correct debugging experience is given by ...

Thanks,

Ingo

Sorry Ingo for that, I overlooked that correction :)

Attached version should reflect that.

Thanks,
Martin

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O3.
Correct debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by the compiler as non-initialized.

---
 tools/perf/arch/common.c            |  2 +-
 tools/perf/config/Makefile          |  4 +++-
 tools/perf/config/utilities.mak     | 19 +++++++++++++++++++
 tools/perf/util/symbol.c            |  2 +-
 tools/perf/util/trace-event-parse.c |  2 +-
 5 files changed, 25 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#16 lore raw

Em Mon, May 25, 2015 at 01:12:44PM +0200, Martin Liška escreveu:

Quoted 20 lines · Ingo Molnar
Quoted 19 lines · Martin Liška
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Or you can use what I suggested first:

Quoted 8 lines
Quoted 6 lines

Correct debugging experience is given by ...

Thanks,

Ingo

Sorry Ingo for that, I overlooked that correction :)

Are you ok with this Ingo?

I can apply, but there seems to be two patches folded here, one that sets the possibly unitiliazed variables to NULL, and could be the first in the series, and the other, that deals with multiple versions of gcc and how should we ask something from them.

- Arnaldo

Quoted 105 lines

Attached version should reflect that.

Thanks,
Martin

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O3.
Correct debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by the compiler as non-initialized.

---
 tools/perf/arch/common.c            |  2 +-
 tools/perf/config/Makefile          |  4 +++-
 tools/perf/config/utilities.mak     | 19 +++++++++++++++++++
 tools/perf/util/symbol.c            |  2 +-
 tools/perf/util/trace-event-parse.c |  2 +-
 5 files changed, 25 insertions(+), 4 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4
#17 lore raw
Quoted 23 lines · Arnaldo Carvalho de Melo

Em Mon, May 25, 2015 at 01:12:44PM +0200, Martin Liška escreveu:

Quoted 22 lines · Ingo Molnar
Quoted 21 lines · Martin Liška
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Or you can use what I suggested first:

Correct debugging experience is given by ...

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Thanks,

Ingo

Sorry Ingo for that, I overlooked that correction :)

Are you ok with this Ingo?

Yeah, certainly!

Quoted 4 lines

I can apply, but there seems to be two patches folded here, one that sets the possibly unitiliazed variables to NULL, and could be the first in the series, and the other, that deals with multiple versions of gcc and how should we ask something from them.

Yeah.

Thanks,

Ingo

author Acked-by Ingo Molnar Acked-by Ingo Molnar
#18 lore raw
Quoted 42 lines · Ingo Molnar
Quoted 23 lines · Arnaldo Carvalho de Melo

Em Mon, May 25, 2015 at 01:12:44PM +0200, Martin Liška escreveu:

Quoted 22 lines · Ingo Molnar
Quoted 21 lines · Martin Liška
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Or you can use what I suggested first:

Correct debugging experience is given by ...

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Thanks,

Ingo

Sorry Ingo for that, I overlooked that correction :)

Are you ok with this Ingo?

Yeah, certainly!

Quoted 4 lines

I can apply, but there seems to be two patches folded here, one that sets the possibly unitiliazed variables to NULL, and could be the first in the series, and the other, that deals with multiple versions of gcc and how should we ask something from them.

Quoted 6 lines

Yeah.

Thanks,

Ingo

Hello.

As Arnaldo pointed, I split the patch to following 2 smaller patches.

Thanks,
Martin

Assign default value for pointers that are identified by the compiler as non-initialized.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 1 line
2.1.4

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O3.
Correct debugging experience is given by passing -Og to compiler.

---
 tools/perf/config/Makefile      |  4 +++-
 tools/perf/config/utilities.mak | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
Signature · 1 line
2.1.4
#19 lore raw

Em Tue, May 26, 2015 at 11:13:32AM +0200, Martin Liška escreveu:

Quoted 45 lines · Ingo Molnar
Quoted 44 lines · Arnaldo Carvalho de Melo
Quoted 24 lines

Em Mon, May 25, 2015 at 01:12:44PM +0200, Martin Liška escreveu:

Quoted 22 lines · Ingo Molnar
Quoted 21 lines · Martin Liška
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Or you can use what I suggested first:

Correct debugging experience is given by ...

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Thanks,

Ingo

Sorry Ingo for that, I overlooked that correction :)

Are you ok with this Ingo?

Yeah, certainly!

Quoted 4 lines

I can apply, but there seems to be two patches folded here, one that sets the possibly unitiliazed variables to NULL, and could be the first in the series, and the other, that deals with multiple versions of gcc and how should we ask something from them.

Yeah.

Quoted 8 lines
Quoted 4 lines

Thanks,

Ingo

Hello.

As Arnaldo pointed, I split the patch to following 2 smaller patches.

Thanks, but how did you generated the two attached patches?

[acme@ssdandy linux]$ git am /wb/0001-perf-Fix-compiler-warnings.patch
Patch format detection failed.

So I'll fix it up manually, next time please try:

git format-patch -n HEAD^^..

Or equivalent,

- Arnaldo

Quoted 113 lines

Thanks,
Martin

Assign default value for pointers that are identified by the compiler as non-initialized.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
Signature · 13 lines
2.1.4


Currently, GCC optimizes -O6 same as -O3 level, thus change the value
to -O3.
Correct debugging experience is given by passing -Og to compiler.

Signed-off-by: Martin Liska <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
---
 tools/perf/config/Makefile      |  4 +++-
 tools/perf/config/utilities.mak | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
 endif
 
 ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
Signature · 1 line
2.1.4
#20 lore raw

Em Tue, May 26, 2015 at 11:36:26AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 5 lines

Em Tue, May 26, 2015 at 11:13:32AM +0200, Martin Liška escreveu:

Quoted 1 line

As Arnaldo pointed, I split the patch to following 2 smaller patches.

Thanks, but how did you generated the two attached patches?

Quoted 8 lines

[acme@ssdandy linux]$ git am /wb/0001-perf-Fix-compiler-warnings.patch
Patch format detection failed.

So I'll fix it up manually, next time please try:

git format-patch -n HEAD^^..

Or equivalent,

I am also splitting the second patch into two, as it does two unrelated things:

1. Conditionally use -Og instead of -O0

2. Use -O3 instead of -O6, as -O6 is the same as -O3 currently

And I am not applying the second, as it doesn't fixes anything nor improves the current situation, right?

- Arnaldo

#21 lore raw
Quoted 18 lines · Arnaldo Carvalho de Melo

Em Tue, May 26, 2015 at 11:36:26AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 5 lines

Em Tue, May 26, 2015 at 11:13:32AM +0200, Martin Liška escreveu:

Quoted 1 line

As Arnaldo pointed, I split the patch to following 2 smaller patches.

Thanks, but how did you generated the two attached patches?

Quoted 8 lines

[acme@ssdandy linux]$ git am /wb/0001-perf-Fix-compiler-warnings.patch
Patch format detection failed.

So I'll fix it up manually, next time please try:

git format-patch -n HEAD^^..

Or equivalent,

I am also splitting the second patch into two, as it does two unrelated things:

Quoted 6 lines

1. Conditionally use -Og instead of -O0

2. Use -O3 instead of -O6, as -O6 is the same as -O3 currently

And I am not applying the second, as it doesn't fixes anything nor improves the current situation, right?

Hi.

Works for me, the first one is more important.

Martin

Quoted 3 lines

- Arnaldo

#22 lore raw
In reply to Arnaldo Carvalho de Melo
Quoted 60 lines · Arnaldo Carvalho de Melo

Em Tue, May 26, 2015 at 11:13:32AM +0200, Martin Liška escreveu:

Quoted 45 lines · Ingo Molnar
Quoted 44 lines · Arnaldo Carvalho de Melo
Quoted 24 lines

Em Mon, May 25, 2015 at 01:12:44PM +0200, Martin Liška escreveu:

Quoted 22 lines · Ingo Molnar
Quoted 21 lines · Martin Liška
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines

Right optimize debugging experience is given by passing -Og to compiler. Assign default value for pointers that are identified by compiler as non-initialized.

s/Right optimize debugging experience is given/
Correct debugging experience is given/

Right debugging experience is given by passing -Og to compiler.

So I fixed the spelling here once already :-/ If you want to use 'right' in this context then use it like this:

The right debugging experience is given by ...

Or you can use what I suggested first:

Correct debugging experience is given by ...

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

Thanks,

Ingo

Sorry Ingo for that, I overlooked that correction :)

Are you ok with this Ingo?

Yeah, certainly!

Quoted 4 lines

I can apply, but there seems to be two patches folded here, one that sets the possibly unitiliazed variables to NULL, and could be the first in the series, and the other, that deals with multiple versions of gcc and how should we ask something from them.

Yeah.

Quoted 8 lines
Quoted 4 lines

Thanks,

Ingo

Hello.

As Arnaldo pointed, I split the patch to following 2 smaller patches.

Thanks, but how did you generated the two attached patches?

[acme@ssdandy linux]$ git am /wb/0001-perf-Fix-compiler-warnings.patch
Patch format detection failed.

Quoted 7 lines

So I'll fix it up manually, next time please try:

git format-patch -n HEAD^^..

Or equivalent,

- Arnaldo

Hi.

That's quite strange, I've just rebuilt my branch to perf/perf/core and I was given equal patches for:

'git format-patch -2'

Maybe that's related to erased patch header(From, Date, Subject).

Thanks for re-application,
Martin

Quoted 117 lines
Quoted 116 lines

Thanks,
Martin

Assign default value for pointers that are identified by the compiler as non-initialized.

---
  tools/perf/arch/common.c            | 2 +-
  tools/perf/util/symbol.c            | 2 +-
  tools/perf/util/trace-event-parse.c | 2 +-
  3 files changed, 3 insertions(+), 3 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
  static bool lookup_path(char *name)
  {
  	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
  	char buf[PATH_MAX];
  	char *env = getenv("PATH");
 
tools/perf/util/symbol.c +1 -1
index 82a31fd..a19fbd4 100644
@@ -400,7 +400,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
  					    const char *name)
  {
  	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
  	if (symbols == NULL)
  		return NULL;
tools/perf/util/trace-event-parse.c +1 -2
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
  	char *line;
  	char *next = NULL;
  	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
  	line = strtok_r(file, "\n", &next);
  	while (line) {
--

2.1.4

Currently, GCC optimizes -O6 same as -O3 level, thus change the value to -O3.
Correct debugging experience is given by passing -Og to compiler.

---
  tools/perf/config/Makefile      |  4 +++-
  tools/perf/config/utilities.mak | 19 +++++++++++++++++++
  2 files changed, 22 insertions(+), 1 deletion(-)
tools/perf/config/Makefile +3 -1
index e3b3724..47e4ae1 100644
@@ -129,7 +129,9 @@ ifndef DEBUG
  endif
 
  ifeq ($(DEBUG),0)
-  CFLAGS += -O6
+  CFLAGS += -O3
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
  endif
 
  ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -1
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
  endef
  _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
  _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
--

2.1.4

#23 lore raw

Em Tue, May 26, 2015 at 05:48:27PM +0200, Martin Liška escreveu:

Quoted 15 lines · Arnaldo Carvalho de Melo
Quoted 13 lines

Em Tue, May 26, 2015 at 11:13:32AM +0200, Martin Liška escreveu:

Quoted 3 lines
Quoted 2 lines · Arnaldo Carvalho de Melo
Quoted 1 line

Em Mon, May 25, 2015 at 01:12:44PM +0200, Martin Liška escreveu:

As Arnaldo pointed, I split the patch to following 2 smaller patches.

Thanks, but how did you generated the two attached patches?

[acme@ssdandy linux]$ git am /wb/0001-perf-Fix-compiler-warnings.patch
Patch format detection failed.

So I'll fix it up manually, next time please try:

git format-patch -n HEAD^^..

Quoted 8 lines
Quoted 1 line

Or equivalent,

That's quite strange, I've just rebuilt my branch to perf/perf/core and I was given equal patches for:

'git format-patch -2'

Maybe that's related to erased patch header(From, Date, Subject).

Yes, I had to add at least From: and Subject:, but my scripts will pick as well the Message-ID to create the Link: line, Cc: info to add the CC and some other stuff that are present when fed with one message per patch.

Quoted 1 line

Thanks for re-application,

You're welcome, keep on contributing!

- Arnaldo

#24 lore raw

[tip:perf/core] perf tools: Assign default value for some pointers

In reply to Martin Liška
perf tools: Assign default value for some pointers
Commit-ID:  5bcaaca3e4d15ce39008a0b9c431c0ac4be784bd
Gitweb:     http://git.kernel.org/tip/5bcaaca3e4d15ce39008a0b9c431c0ac4be784bd
Author:     Martin Liška <[email protected]>
AuthorDate: Tue, 26 May 2015 11:41:37 -0300
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 27 May 2015 12:21:45 -0300

perf tools: Assign default value for some pointers

Assign default value for pointers that are identified by the compiler as non-initialized.

---
 tools/perf/arch/common.c            | 2 +-
 tools/perf/util/symbol.c            | 2 +-
 tools/perf/util/trace-event-parse.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
tools/perf/arch/common.c +1 -1
index 49776f1..b7bb42c 100644
@@ -61,7 +61,7 @@ const char *const mips_triplets[] = {
 static bool lookup_path(char *name)
 {
 	bool found = false;
-	char *path, *tmp;
+	char *path, *tmp = NULL;
 	char buf[PATH_MAX];
 	char *env = getenv("PATH");
 
tools/perf/util/symbol.c +1 -1
index 00b6b17..b9e3eb5 100644
@@ -398,7 +398,7 @@ static struct symbol *symbols__find_by_name(struct rb_root *symbols,
 					    const char *name)
 {
 	struct rb_node *n;
-	struct symbol_name_rb_node *s;
+	struct symbol_name_rb_node *s = NULL;
 
 	if (symbols == NULL)
 		return NULL;
tools/perf/util/trace-event-parse.c +1 -1
index 25d6c73..d495741 100644
@@ -173,7 +173,7 @@ void parse_ftrace_printk(struct pevent *pevent,
 	char *line;
 	char *next = NULL;
 	char *addr_str;
-	char *fmt;
+	char *fmt = NULL;
 
 	line = strtok_r(file, "\n", &next);
 	while (line) {
#25 lore raw

[tip:perf/core] perf tools: Improve setting of gcc debug option

In reply to Martin Liška
perf tools: Improve setting of gcc debug option
Commit-ID:  e8b7ea4356fdd3c4de5478f3418eb84f8dce2b61
Gitweb:     http://git.kernel.org/tip/e8b7ea4356fdd3c4de5478f3418eb84f8dce2b61
Author:     Martin Liska <[email protected]>
AuthorDate: Tue, 26 May 2015 12:23:24 -0300
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 27 May 2015 12:21:45 -0300

perf tools: Improve setting of gcc debug option

Correct debugging experience is given by passing -Og to compiler.

Do it in a way that supports older compilers

---
 tools/perf/config/Makefile      |  2 ++
 tools/perf/config/utilities.mak | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)
tools/perf/config/Makefile +2 -0
index e3b3724..317001c 100644
@@ -130,6 +130,8 @@ endif
 
 ifeq ($(DEBUG),0)
   CFLAGS += -O6
+else
+  CFLAGS += $(call cc-option,-Og,-O0)
 endif
 
 ifdef PARSER_DEBUG
tools/perf/config/utilities.mak +19 -0
index c16ce83..0ebef09 100644
@@ -177,3 +177,22 @@ $(if $($(1)),$(call _ge_attempt,$($(1)),$(1)),$(call _ge_attempt,$(2)))
 endef
 _ge_attempt = $(if $(get-executable),$(get-executable),$(call _gea_err,$(2)))
 _gea_err  = $(if $(1),$(error Please set '$(1)' appropriately))
+
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" is can be used as temporary file and
+# is automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP="$(TMPOUT).$$$$.tmp";	\
+	TMPO="$(TMPOUT).$$$$.o";	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi;				\
+	rm -f "$$TMP" "$$TMPO")
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+
+cc-option = $(call try-run,\
+	$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))