* tools build: Unused function, incomplete rename
@ 2015-09-18 19:23 Arnaldo Carvalho de Melo
2015-09-18 19:38 ` Arnaldo Carvalho de Melo
2015-09-19 13:35 ` Jiri Olsa
0 siblings, 2 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-09-18 19:23 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern,
Linux Kernel Mailing List
Hi Jiri, Ingo,
While trying to figure out why the bpf feature test is always
triggering the display of the "Auto-detecting system features" I noticed
this pattern:
[acme@felicio linux]$ egrep '^define|eval' tools/build/Makefile.feature
feature_check = $(eval $(feature_check_code))
define feature_check_code
feature_set = $(eval $(feature_set_code))
define feature_set_code
set_test_all_flags = $(eval $(set_test_all_flags_code))
define set_test_all_flags_code
feature_print_status = $(eval $(feature_print_status_code)) $(info $(MSG))
define feature_print_status_code
feature_print_text = $(eval $(feature_print_text_code)) $(info $(MSG))
define feature_print_text_code
feature_display_check = $(eval $(feature_check_code))
define feature_display_check_code
[acme@felicio linux]$
In all but one case the eval matches the following define, except for the last
one, don't we need the following patch?
$ git diff tools/build/Makefile.feature
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 0caeaf2cae5f..072ec879b84f 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -142,7 +142,7 @@ ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)")
feature_display := 1
endif
-feature_display_check = $(eval $(feature_check_code))
+feature_display_check = $(eval $(feature_check_display_code))
define feature_display_check_code
ifneq ($(feature-$(1)), 1)
feature_display := 1
---------------------------------------------------------------------
I guess the bug was introduced here:
commit 58d4f00ff13f20468f8fa8edcb57a195c31af46d
Author: Jiri Olsa <jolsa@kernel.org>
Date: Thu Mar 19 20:48:49 2015 +0100
perf build: Fix feature_check name clash
We have 2 feature_check functions, which conflict with each other.
Fixing it by renaming the latter to feature_display_check.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <david.ahern@oracle.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-wmyccro6qeffseforipu5kcl@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index e7f83b15fcbf..81d8c2bbc4df 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -805,14 +805,14 @@ ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)")
feature_display := 1
endif
-feature_check = $(eval $(feature_check_code))
-define feature_check_code
+feature_display_check = $(eval $(feature_check_code))
+define feature_display_check_code
ifneq ($(feature-$(1)), 1)
feature_display := 1
endif
endef
-$(foreach feat,$(FEATURE_DISPLAY),$(call feature_check,$(feat)))
+$(foreach feat,$(FEATURE_DISPLAY),$(call feature_display_check,$(feat)))
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename
2015-09-18 19:23 tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo
@ 2015-09-18 19:38 ` Arnaldo Carvalho de Melo
2015-09-18 19:48 ` Arnaldo Carvalho de Melo
2015-09-19 13:35 ` Jiri Olsa
1 sibling, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-09-18 19:38 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan,
Linux Kernel Mailing List
Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu:
> Hi Jiri, Ingo,
>
> While trying to figure out why the bpf feature test is always
> triggering the display of the "Auto-detecting system features" I noticed
> this pattern:
Another problem, this time in how tools/lib/bpf/ specifies which
features to test for and which ones should have the feature detection
shown, does the following patch makes sense? I think it does because
FEATURE_TESTS looks like the ones that will be tested, and
FEATURE_DISPLAY the ones that will appear...:
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index f68d23a0b487..604c12081b4b 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -64,8 +64,8 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
-FEATURE_DISPLAY = libelf libelf-getphdrnum libelf-mmap bpf
-FEATURE_TESTS = libelf bpf
+FEATURE_TESTS = libelf libelf-getphdrnum libelf-mmap bpf
+FEATURE_DISPLAY = libelf bpf
INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/arch/$(ARCH)/include/uapi -I$(srctree)/include/uapi
FEATURE_CHECK_CFLAGS-bpf = $(INCLUDES)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename
2015-09-18 19:38 ` Arnaldo Carvalho de Melo
@ 2015-09-18 19:48 ` Arnaldo Carvalho de Melo
2015-09-18 20:42 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-09-18 19:48 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan,
Linux Kernel Mailing List
Em Fri, Sep 18, 2015 at 04:38:42PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu:
> > While trying to figure out why the bpf feature test is always
> > triggering the display of the "Auto-detecting system features" I noticed
> > this pattern:
> Another problem, this time in how tools/lib/bpf/ specifies which
> features to test for and which ones should have the feature detection
> shown, does the following patch makes sense? I think it does because
> FEATURE_TESTS looks like the ones that will be tested, and
> FEATURE_DISPLAY the ones that will appear...:
So the original problem seems to be this:
[acme@felicio linux]$ cat /tmp/build/perf/FEATURE-DUMP
feature-libelf(1) feature-libelf-getphdrnum(1) feature-libelf-mmap(1) feature-bpf(1)
This is the content at the end of a build, i.e. the FEATURE-DUMP for
tools/lib/ebpf/ usage of the feature detection system, since
tools/perf/ uses the same file and selects a different set of
features.
I think that ebpf should use a separate directory, inside $(OUTPUT),
this way we would have $(OUTPUT)/FEATURE-DUMP for perf and
$(OUTPUT)/bpf/FEATURE-DUMP for ebpf.
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename
2015-09-18 19:48 ` Arnaldo Carvalho de Melo
@ 2015-09-18 20:42 ` Arnaldo Carvalho de Melo
2015-09-19 13:45 ` Jiri Olsa
0 siblings, 1 reply; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2015-09-18 20:42 UTC (permalink / raw)
To: Jiri Olsa
Cc: Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern, Wang Nan,
Linux Kernel Mailing List
Em Fri, Sep 18, 2015 at 04:48:52PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Sep 18, 2015 at 04:38:42PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > While trying to figure out why the bpf feature test is always
> > > triggering the display of the "Auto-detecting system features" I noticed
> > > this pattern:
>
> > Another problem, this time in how tools/lib/bpf/ specifies which
> > features to test for and which ones should have the feature detection
> > shown, does the following patch makes sense? I think it does because
> > FEATURE_TESTS looks like the ones that will be tested, and
> > FEATURE_DISPLAY the ones that will appear...:
>
> So the original problem seems to be this:
>
> [acme@felicio linux]$ cat /tmp/build/perf/FEATURE-DUMP
> feature-libelf(1) feature-libelf-getphdrnum(1) feature-libelf-mmap(1) feature-bpf(1)
>
> This is the content at the end of a build, i.e. the FEATURE-DUMP for
> tools/lib/ebpf/ usage of the feature detection system, since
> tools/perf/ uses the same file and selects a different set of
> features.
>
> I think that ebpf should use a separate directory, inside $(OUTPUT),
> this way we would have $(OUTPUT)/FEATURE-DUMP for perf and
> $(OUTPUT)/bpf/FEATURE-DUMP for ebpf.
[acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP
-rw-rw-r--. 1 acme acme 338 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP
[acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP.libbpf
-rw-rw-r--. 1 acme acme 85 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP.libbpf
[acme@felicio linux]$
Ok, patch below fixes this one, now the second run doesn't auto detects
things again, i.e. libbpf feature detection doesn't stomps on perf's,
I'll get those patches in a patchkit and send over the weekend. If you
find anything fishy with it, holler.
- Arnaldo
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 0caeaf2cae5f..0dedb3d245a1 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -123,8 +123,9 @@ define feature_print_text_code
MSG = $(shell printf '...%30s: %s' $(1) $(2))
endef
+FEATURE_DUMP_FILENAME = $(OUTPUT)FEATURE-DUMP$(FEATURE_USER)
FEATURE_DUMP := $(foreach feat,$(FEATURE_DISPLAY),feature-$(feat)($(feature-$(feat))))
-FEATURE_DUMP_FILE := $(shell touch $(OUTPUT)FEATURE-DUMP; cat $(OUTPUT)FEATURE-DUMP)
+FEATURE_DUMP_FILE := $(shell touch $(FEATURE_DUMP_FILENAME); cat $(FEATURE_DUMP_FILENAME))
ifeq ($(dwarf-post-unwind),1)
FEATURE_DUMP += dwarf-post-unwind($(dwarf-post-unwind-text))
@@ -138,6 +139,6 @@ endif
# - VF is enabled
ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)")
- $(shell echo "$(FEATURE_DUMP)" > $(OUTPUT)FEATURE-DUMP)
+ $(shell echo "$(FEATURE_DUMP)" > $(FEATURE_DUMP_FILENAME))
feature_display := 1
endif
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index f68d23a0b487..01b4ff16a4e0 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -64,8 +64,8 @@ srctree := $(patsubst %/,%,$(dir $(srctree)))
#$(info Determined 'srctree' to be $(srctree))
endif
+FEATURE_USER = .libbpf
FEATURE_DISPLAY = libelf libelf-getphdrnum libelf-mmap bpf
FEATURE_TESTS = libelf bpf
INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/arch/$(ARCH)/include/uapi -I$(srctree)/include/uapi
FEATURE_CHECK_CFLAGS-bpf = $(INCLUDES)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename
2015-09-18 19:23 tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo
2015-09-18 19:38 ` Arnaldo Carvalho de Melo
@ 2015-09-19 13:35 ` Jiri Olsa
1 sibling, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2015-09-19 13:35 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern,
Linux Kernel Mailing List
On Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo wrote:
> Hi Jiri, Ingo,
>
> While trying to figure out why the bpf feature test is always
> triggering the display of the "Auto-detecting system features" I noticed
> this pattern:
>
> [acme@felicio linux]$ egrep '^define|eval' tools/build/Makefile.feature
> feature_check = $(eval $(feature_check_code))
> define feature_check_code
> feature_set = $(eval $(feature_set_code))
> define feature_set_code
> set_test_all_flags = $(eval $(set_test_all_flags_code))
> define set_test_all_flags_code
> feature_print_status = $(eval $(feature_print_status_code)) $(info $(MSG))
> define feature_print_status_code
> feature_print_text = $(eval $(feature_print_text_code)) $(info $(MSG))
> define feature_print_text_code
> feature_display_check = $(eval $(feature_check_code))
> define feature_display_check_code
> [acme@felicio linux]$
>
>
> In all but one case the eval matches the following define, except for the last
> one, don't we need the following patch?
>
> $ git diff tools/build/Makefile.feature
> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> index 0caeaf2cae5f..072ec879b84f 100644
> --- a/tools/build/Makefile.feature
> +++ b/tools/build/Makefile.feature
> @@ -142,7 +142,7 @@ ifneq ("$(FEATURE_DUMP)","$(FEATURE_DUMP_FILE)")
> feature_display := 1
> endif
>
> -feature_display_check = $(eval $(feature_check_code))
> +feature_display_check = $(eval $(feature_check_display_code))
> define feature_display_check_code
> ifneq ($(feature-$(1)), 1)
> feature_display := 1
ouch, that is a bug.. nice catch!
thanks,
jirka
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tools build: Unused function, incomplete rename
2015-09-18 20:42 ` Arnaldo Carvalho de Melo
@ 2015-09-19 13:45 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2015-09-19 13:45 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Ingo Molnar, Namhyung Kim, Adrian Hunter, David Ahern,
Wang Nan, Linux Kernel Mailing List
On Fri, Sep 18, 2015 at 05:42:47PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 18, 2015 at 04:48:52PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Sep 18, 2015 at 04:38:42PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, Sep 18, 2015 at 04:23:34PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > While trying to figure out why the bpf feature test is always
> > > > triggering the display of the "Auto-detecting system features" I noticed
> > > > this pattern:
> >
> > > Another problem, this time in how tools/lib/bpf/ specifies which
> > > features to test for and which ones should have the feature detection
> > > shown, does the following patch makes sense? I think it does because
> > > FEATURE_TESTS looks like the ones that will be tested, and
> > > FEATURE_DISPLAY the ones that will appear...:
> >
> > So the original problem seems to be this:
> >
> > [acme@felicio linux]$ cat /tmp/build/perf/FEATURE-DUMP
> > feature-libelf(1) feature-libelf-getphdrnum(1) feature-libelf-mmap(1) feature-bpf(1)
> >
> > This is the content at the end of a build, i.e. the FEATURE-DUMP for
> > tools/lib/ebpf/ usage of the feature detection system, since
> > tools/perf/ uses the same file and selects a different set of
> > features.
> >
> > I think that ebpf should use a separate directory, inside $(OUTPUT),
> > this way we would have $(OUTPUT)/FEATURE-DUMP for perf and
> > $(OUTPUT)/bpf/FEATURE-DUMP for ebpf.
>
> [acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP
> -rw-rw-r--. 1 acme acme 338 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP
> [acme@felicio linux]$ ls -la /tmp/build/perf/FEATURE-DUMP.libbpf
> -rw-rw-r--. 1 acme acme 85 Sep 18 17:38 /tmp/build/perf/FEATURE-DUMP.libbpf
> [acme@felicio linux]$
>
> Ok, patch below fixes this one, now the second run doesn't auto detects
> things again, i.e. libbpf feature detection doesn't stomps on perf's,
> I'll get those patches in a patchkit and send over the weekend. If you
> find anything fishy with it, holler.
it looks ok.. btw IMO sharing output directory for perf
and related libs could bite us in the future.. I think
we should base it to tools directory.. I'll try to come
up with something
thanks,
jirka
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-19 13:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-18 19:23 tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo
2015-09-18 19:38 ` Arnaldo Carvalho de Melo
2015-09-18 19:48 ` Arnaldo Carvalho de Melo
2015-09-18 20:42 ` Arnaldo Carvalho de Melo
2015-09-19 13:45 ` Jiri Olsa
2015-09-19 13:35 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox