The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org --- tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE + $(call rule_mkdir) + $(call if_changed_dep,$(host)cc_o_c) + +$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE + $(call rule_mkdir) + $(call if_changed_dep,$(host)cc_o_c) + # Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif + +$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C) + $(call rule_mkdir) + $(call if_changed_dep,$(host)cc_o_c)
The flex and bison generate header files from the source. When user specified a build directory with O= option, it'd generate files under the directory. The build command has -I option to specify the header include directory.
But the -I option only affects the files included like <...>. Let's change the flex and bison headers to use it instead of "...".
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org --- tools/perf/pmu-events/jevents.py | 2 +- tools/perf/util/bpf-filter.c | 4 ++-- tools/perf/util/expr.c | 4 ++-- tools/perf/util/parse-events.c | 4 ++-- tools/perf/util/pmu.c | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py index 12e80bb7939b..82c42c46d886 100755 --- a/tools/perf/pmu-events/jevents.py +++ b/tools/perf/pmu-events/jevents.py @@ -999,7 +999,7 @@ such as "arm/cortex-a34".''', _args = ap.parse_args()
_args.output_file.write(""" -#include "pmu-events/pmu-events.h" +#include <pmu-events/pmu-events.h> #include "util/header.h" #include "util/pmu.h" #include <string.h> diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c index 0b30688d78a7..47f01df658d9 100644 --- a/tools/perf/util/bpf-filter.c +++ b/tools/perf/util/bpf-filter.c @@ -9,8 +9,8 @@ #include "util/evsel.h"
#include "util/bpf-filter.h" -#include "util/bpf-filter-flex.h" -#include "util/bpf-filter-bison.h" +#include <util/bpf-filter-flex.h> +#include <util/bpf-filter-bison.h>
#include "bpf_skel/sample-filter.h" #include "bpf_skel/sample_filter.skel.h" diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c index 4814262e3805..7410a165f68b 100644 --- a/tools/perf/util/expr.c +++ b/tools/perf/util/expr.c @@ -10,8 +10,8 @@ #include "debug.h" #include "evlist.h" #include "expr.h" -#include "expr-bison.h" -#include "expr-flex.h" +#include <util/expr-bison.h> +#include <util/expr-flex.h> #include "util/hashmap.h" #include "smt.h" #include "tsc.h" diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index acde097e327c..f3773467c103 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -18,8 +18,8 @@ #include "debug.h" #include <api/fs/tracing_path.h> #include <perf/cpumap.h> -#include "parse-events-bison.h" -#include "parse-events-flex.h" +#include <util/parse-events-bison.h> +#include <util/parse-events-flex.h> #include "pmu.h" #include "pmus.h" #include "asm/bug.h" diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index 7f984a7f16ca..b6654b9f55d2 100644 --- a/tools/perf/util/pmu.c +++ b/tools/perf/util/pmu.c @@ -19,8 +19,8 @@ #include "evsel.h" #include "pmu.h" #include "pmus.h" -#include "pmu-bison.h" -#include "pmu-flex.h" +#include <util/pmu-bison.h> +#include <util/pmu-flex.h> #include "parse-events.h" #include "print-events.h" #include "header.h"
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote:
The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org
tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)+$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)
Hi Namhyung,
as we have: ``` $(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c) ``` I'm not sure what the 2 additional rules achieve.
# Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif
+$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C)
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)
If we add this, do the Makefile.build changes still need to happen?
Thanks, Ian
-- 2.41.0.487.g6d72f3e995-goog
Hi Ian,
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote:
The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org
tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)+$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)Hi Namhyung,
as we have:
$(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
# Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif
+$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C)
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Thanks, Namhyung
On Wed, Jul 26, 2023 at 3:57 PM Namhyung Kim namhyung@kernel.org wrote:
Hi Ian,
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote:
The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org
tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)+$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)Hi Namhyung,
as we have:
$(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
# Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif
+$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C)
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Got it, you are right I was confusing the flex/bison with the jevents case. Can we get away with a single rule then: ``` $(OUTPUT)%.o: $(OUTPUT)%.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c) ```
Thanks, Ian
Thanks, Namhyung
On Wed, Jul 26, 2023 at 5:36 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 3:57 PM Namhyung Kim namhyung@kernel.org wrote:
Hi Ian,
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote:
The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org
tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)+$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)Hi Namhyung,
as we have:
$(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
# Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif
+$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C)
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Got it, you are right I was confusing the flex/bison with the jevents case. Can we get away with a single rule then:
$(OUTPUT)%.o: $(OUTPUT)%.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)
Probably, but I wonder if it affects the normal .c files expecting them in the OUTPUT directory.
Thanks, Namhyung
On Wed, Jul 26, 2023 at 6:01 PM Namhyung Kim namhyung@kernel.org wrote:
On Wed, Jul 26, 2023 at 5:36 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 3:57 PM Namhyung Kim namhyung@kernel.org wrote:
Hi Ian,
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote:
The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org
tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)+$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)Hi Namhyung,
as we have:
$(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
# Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif
+$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C)
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Got it, you are right I was confusing the flex/bison with the jevents case. Can we get away with a single rule then:
$(OUTPUT)%.o: $(OUTPUT)%.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)Probably, but I wonder if it affects the normal .c files expecting them in the OUTPUT directory.
Hmm.. I think the longer matches may be necessary to trigger the "more specific" ordering: https://www.gnu.org/software/make/manual/html_node/Pattern-Match.html
I'm not keen on these extra rules that mirror existing rules, it is a bit cryptic what is going on. I wonder if it would be cleaner just to fail the build if the bogus pmu-events.c exists. For example: ``` --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -12,6 +12,14 @@ EMPTY_PMU_EVENTS_C = pmu-events/empty-pmu-events.c PMU_EVENTS_C = $(OUTPUT)pmu-events/pmu-events.c METRIC_TEST_LOG = $(OUTPUT)pmu-events/metric_test.log
+ifneq ($(OUTPUT),) +ifneq ($(wildcard pmu-events/pmu-events.c),) +dummy := $(error OUTPUT set but tools/perf/pmu-events/pmu-events.c exists. \ + This can mean the generated version of pmu-events.c is not compiled. \ + Try 'cd tools/perf; make clean; cd -' and then rebuilding.) +endif +endif + ifeq ($(JEVENTS_ARCH),) JEVENTS_ARCH=$(SRCARCH) endif ```
Thanks, Ian
Thanks, Namhyung
On Wed, Jul 26, 2023 at 9:48 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:01 PM Namhyung Kim namhyung@kernel.org wrote:
On Wed, Jul 26, 2023 at 5:36 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 3:57 PM Namhyung Kim namhyung@kernel.org wrote:
Hi Ian,
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote:
The bison and flex generate C files from the source (.y and .l) files. When O= option is used, they are saved in a separate directory but the default build rule assumes the .C files are in the source directory. So it might read invalid file if there are generated files from an old version. The same is true for the pmu-events files.
For example, the following command would cause a build failure:
$ git checkout v6.3 $ make -C tools/perf # build in the same directory
$ git checkout v6.5-rc2 $ mkdir build # create a build directory $ make -C tools/perf O=build # build in a different directory but it # refers files in the source directory
Let's update the build rule to specify those cases explicitly to depend on the files in the output directory.
Note that it's not a complete fix and it needs the next patch for the include path too.
Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") Cc: stable@vger.kernel.org Signed-off-by: Namhyung Kim namhyung@kernel.org
tools/build/Makefile.build | 8 ++++++++ tools/perf/pmu-events/Build | 4 ++++ 2 files changed, 12 insertions(+)
diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build index 89430338a3d9..f9396696fcbf 100644 --- a/tools/build/Makefile.build +++ b/tools/build/Makefile.build @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,cc_s_c)
+$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)+$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)Hi Namhyung,
as we have:
$(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
# Gather build data: # obj-y - list of build objects # subdir-y - list of directories to nest diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build index 150765f2baee..f38a27765604 100644 --- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L $(call rule_mkdir) $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ endif
+$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C)
$(call rule_mkdir)$(call if_changed_dep,$(host)cc_o_c)If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Got it, you are right I was confusing the flex/bison with the jevents case. Can we get away with a single rule then:
$(OUTPUT)%.o: $(OUTPUT)%.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)Probably, but I wonder if it affects the normal .c files expecting them in the OUTPUT directory.
Hmm.. I think the longer matches may be necessary to trigger the "more specific" ordering: https://www.gnu.org/software/make/manual/html_node/Pattern-Match.html
I'm not keen on these extra rules that mirror existing rules, it is a bit cryptic what is going on. I wonder if it would be cleaner just to fail the build if the bogus pmu-events.c exists. For example:
I prefer just making it build instead of failing. But not strongly against your idea. It'd be nice to hear what others think.. Arnaldo?
--- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -12,6 +12,14 @@ EMPTY_PMU_EVENTS_C = pmu-events/empty-pmu-events.c PMU_EVENTS_C = $(OUTPUT)pmu-events/pmu-events.c METRIC_TEST_LOG = $(OUTPUT)pmu-events/metric_test.log +ifneq ($(OUTPUT),) +ifneq ($(wildcard pmu-events/pmu-events.c),) +dummy := $(error OUTPUT set but tools/perf/pmu-events/pmu-events.c exists. \ + This can mean the generated version of pmu-events.c is not compiled. \ + Try 'cd tools/perf; make clean; cd -' and then rebuilding.)
I've tested this and this message is buried in the stream of parallel build outputs. :(
Thanks, Namhyung
+endif +endif
ifeq ($(JEVENTS_ARCH),) JEVENTS_ARCH=$(SRCARCH) endif
Em Wed, Jul 26, 2023 at 11:45:44PM -0700, Namhyung Kim escreveu:
On Wed, Jul 26, 2023 at 9:48 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:01 PM Namhyung Kim namhyung@kernel.org wrote:
On Wed, Jul 26, 2023 at 5:36 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 3:57 PM Namhyung Kim namhyung@kernel.org wrote:
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote: > The bison and flex generate C files from the source (.y and .l) > files. When O= option is used, they are saved in a separate directory > but the default build rule assumes the .C files are in the source > directory. So it might read invalid file if there are generated files > from an old version. The same is true for the pmu-events files. > > For example, the following command would cause a build failure: > > $ git checkout v6.3 > $ make -C tools/perf # build in the same directory > > $ git checkout v6.5-rc2 > $ mkdir build # create a build directory > $ make -C tools/perf O=build # build in a different directory but it > # refers files in the source directory > > Let's update the build rule to specify those cases explicitly to depend > on the files in the output directory. > > Note that it's not a complete fix and it needs the next patch for the > include path too. > > Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") > Cc: stable@vger.kernel.org > Signed-off-by: Namhyung Kim namhyung@kernel.org > --- > tools/build/Makefile.build | 8 ++++++++ > tools/perf/pmu-events/Build | 4 ++++ > 2 files changed, 12 insertions(+) > > diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build > index 89430338a3d9..f9396696fcbf 100644 > --- a/tools/build/Makefile.build > +++ b/tools/build/Makefile.build > @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE > $(call rule_mkdir) > $(call if_changed_dep,cc_s_c) > > +$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE > + $(call rule_mkdir) > + $(call if_changed_dep,$(host)cc_o_c) > + > +$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE > + $(call rule_mkdir) > + $(call if_changed_dep,$(host)cc_o_c) > +
Hi Namhyung,
as we have:
$(OUTPUT)%.o: %.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
> # Gather build data: > # obj-y - list of build objects > # subdir-y - list of directories to nest > diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build > index 150765f2baee..f38a27765604 100644 > --- a/tools/perf/pmu-events/Build > +++ b/tools/perf/pmu-events/Build > @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L > $(call rule_mkdir) > $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ > endif > + > +$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C) > + $(call rule_mkdir) > + $(call if_changed_dep,$(host)cc_o_c)
If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Got it, you are right I was confusing the flex/bison with the jevents case. Can we get away with a single rule then:
$(OUTPUT)%.o: $(OUTPUT)%.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)Probably, but I wonder if it affects the normal .c files expecting them in the OUTPUT directory.
Hmm.. I think the longer matches may be necessary to trigger the "more specific" ordering: https://www.gnu.org/software/make/manual/html_node/Pattern-Match.html
I'm not keen on these extra rules that mirror existing rules, it is a bit cryptic what is going on. I wonder if it would be cleaner just to fail the build if the bogus pmu-events.c exists. For example:
I prefer just making it build instead of failing. But not strongly against your idea. It'd be nice to hear what others think.. Arnaldo?
I think that we should just ignore any build files in the source tree when using 'O=', i.e.:
make -C tools/perf
genereated files (in the source tree) should not be used when, right after running it, we run:
make -C tools/perf O=/some/build/dir
If we run 'make -C tools/perf' and there is a pmu-events.c, use it if its timestamp is more recent than the files from which it was generated, as usual for a make managed build.
- Arnaldo
--- a/tools/perf/pmu-events/Build +++ b/tools/perf/pmu-events/Build @@ -12,6 +12,14 @@ EMPTY_PMU_EVENTS_C = pmu-events/empty-pmu-events.c PMU_EVENTS_C = $(OUTPUT)pmu-events/pmu-events.c METRIC_TEST_LOG = $(OUTPUT)pmu-events/metric_test.log +ifneq ($(OUTPUT),) +ifneq ($(wildcard pmu-events/pmu-events.c),) +dummy := $(error OUTPUT set but tools/perf/pmu-events/pmu-events.c exists. \ + This can mean the generated version of pmu-events.c is not compiled. \ + Try 'cd tools/perf; make clean; cd -' and then rebuilding.)I've tested this and this message is buried in the stream of parallel build outputs. :(
Thanks, Namhyung
+endif +endif
ifeq ($(JEVENTS_ARCH),) JEVENTS_ARCH=$(SRCARCH) endif
Hi Arnaldo,
On Thu, Jul 27, 2023 at 4:31 AM Arnaldo Carvalho de Melo acme@kernel.org wrote:
Em Wed, Jul 26, 2023 at 11:45:44PM -0700, Namhyung Kim escreveu:
On Wed, Jul 26, 2023 at 9:48 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 6:01 PM Namhyung Kim namhyung@kernel.org wrote:
On Wed, Jul 26, 2023 at 5:36 PM Ian Rogers irogers@google.com wrote:
On Wed, Jul 26, 2023 at 3:57 PM Namhyung Kim namhyung@kernel.org wrote:
On Wed, Jul 26, 2023 at 8:48 AM Ian Rogers irogers@google.com wrote: > On Wed, Jul 26, 2023 at 6:36 AM Namhyung Kim namhyung@kernel.org wrote: > > The bison and flex generate C files from the source (.y and .l) > > files. When O= option is used, they are saved in a separate directory > > but the default build rule assumes the .C files are in the source > > directory. So it might read invalid file if there are generated files > > from an old version. The same is true for the pmu-events files. > > > > For example, the following command would cause a build failure: > > > > $ git checkout v6.3 > > $ make -C tools/perf # build in the same directory > > > > $ git checkout v6.5-rc2 > > $ mkdir build # create a build directory > > $ make -C tools/perf O=build # build in a different directory but it > > # refers files in the source directory > > > > Let's update the build rule to specify those cases explicitly to depend > > on the files in the output directory. > > > > Note that it's not a complete fix and it needs the next patch for the > > include path too. > > > > Fixes: 80eeb67fe577 ("perf jevents: Program to convert JSON file") > > Cc: stable@vger.kernel.org > > Signed-off-by: Namhyung Kim namhyung@kernel.org > > --- > > tools/build/Makefile.build | 8 ++++++++ > > tools/perf/pmu-events/Build | 4 ++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build > > index 89430338a3d9..f9396696fcbf 100644 > > --- a/tools/build/Makefile.build > > +++ b/tools/build/Makefile.build > > @@ -117,6 +117,14 @@ $(OUTPUT)%.s: %.c FORCE > > $(call rule_mkdir) > > $(call if_changed_dep,cc_s_c) > > > > +$(OUTPUT)%-bison.o: $(OUTPUT)%-bison.c FORCE > > + $(call rule_mkdir) > > + $(call if_changed_dep,$(host)cc_o_c) > > + > > +$(OUTPUT)%-flex.o: $(OUTPUT)%-flex.c FORCE > > + $(call rule_mkdir) > > + $(call if_changed_dep,$(host)cc_o_c) > > + > > Hi Namhyung, > > as we have: > ``` > $(OUTPUT)%.o: %.c FORCE > $(call rule_mkdir) > $(call if_changed_dep,$(host)cc_o_c) > ``` > I'm not sure what the 2 additional rules achieve.
The above rule assumes the .c files are in the source directory (without $(OUTPUT) prefix). It caused a trouble when the flex and bison files are generated in the output directory and you have an old version of them in the source directory.
> > # Gather build data: > > # obj-y - list of build objects > > # subdir-y - list of directories to nest > > diff --git a/tools/perf/pmu-events/Build b/tools/perf/pmu-events/Build > > index 150765f2baee..f38a27765604 100644 > > --- a/tools/perf/pmu-events/Build > > +++ b/tools/perf/pmu-events/Build > > @@ -35,3 +35,7 @@ $(PMU_EVENTS_C): $(JSON) $(JSON_TEST) $(JEVENTS_PY) $(METRIC_PY) $(METRIC_TEST_L > > $(call rule_mkdir) > > $(Q)$(call echo-cmd,gen)$(PYTHON) $(JEVENTS_PY) $(JEVENTS_ARCH) $(JEVENTS_MODEL) pmu-events/arch $@ > > endif > > + > > +$(OUTPUT)pmu-events/pmu-events.o: $(PMU_EVENTS_C) > > + $(call rule_mkdir) > > + $(call if_changed_dep,$(host)cc_o_c) > > If we add this, do the Makefile.build changes still need to happen?
The Makefile.build changes are specific to flex and bison files. So yes, we need this for pmu-events.c to work properly with O= option.
Got it, you are right I was confusing the flex/bison with the jevents case. Can we get away with a single rule then:
$(OUTPUT)%.o: $(OUTPUT)%.c FORCE $(call rule_mkdir) $(call if_changed_dep,$(host)cc_o_c)Probably, but I wonder if it affects the normal .c files expecting them in the OUTPUT directory.
Hmm.. I think the longer matches may be necessary to trigger the "more specific" ordering: https://www.gnu.org/software/make/manual/html_node/Pattern-Match.html
I'm not keen on these extra rules that mirror existing rules, it is a bit cryptic what is going on. I wonder if it would be cleaner just to fail the build if the bogus pmu-events.c exists. For example:
I prefer just making it build instead of failing. But not strongly against your idea. It'd be nice to hear what others think.. Arnaldo?
I think that we should just ignore any build files in the source tree when using 'O=', i.e.:
make -C tools/perf
genereated files (in the source tree) should not be used when, right after running it, we run:
make -C tools/perf O=/some/build/dir
If we run 'make -C tools/perf' and there is a pmu-events.c, use it if its timestamp is more recent than the files from which it was generated, as usual for a make managed build.
Right, this is what my patch does by adding specific rules for generated files. I'll add a comment to describe why it's needed.
Thanks, Namhyung
linux-stable-mirror@lists.linaro.org
-
Arnaldo Carvalho de Melo -
Ian Rogers -
Namhyung Kim