Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1228243 > unrolled thread

tools build: Unused function, incomplete rename

Started byArnaldo Carvalho de Melo <acme@kernel.org>
First post2015-09-18 21:30 +0200
Last post2015-09-19 15:40 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-18 21:30 +0200
    Re: tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-18 21:40 +0200
      Re: tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-18 21:50 +0200
        Re: tools build: Unused function, incomplete rename Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-18 22:50 +0200
          Re: tools build: Unused function, incomplete rename Jiri Olsa <jolsa@redhat.com> - 2015-09-19 15:50 +0200
    Re: tools build: Unused function, incomplete rename Jiri Olsa <jolsa@redhat.com> - 2015-09-19 15:40 +0200

#1228243 — tools build: Unused function, incomplete rename

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-18 21:30 +0200
Subjecttools build: Unused function, incomplete rename
Message-ID<qa9w5-3sg-9@gated-at.bofh.it>
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)))
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1228244

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-18 21:40 +0200
Message-ID<qa9FL-3Dm-11@gated-at.bofh.it>
In reply to#1228243
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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228249

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-18 21:50 +0200
Message-ID<qa9Ps-3OD-9@gated-at.bofh.it>
In reply to#1228244
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228268

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-18 22:50 +0200
Message-ID<qaaLv-58K-1@gated-at.bofh.it>
In reply to#1228249
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)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228447

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-19 15:50 +0200
Message-ID<qaqGC-2Zp-11@gated-at.bofh.it>
In reply to#1228268
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1228446

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-19 15:40 +0200
Message-ID<qaqwW-2NP-9@gated-at.bofh.it>
In reply to#1228243
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web