[1/2] perf build: Update build rule for generated files

  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(+)
  

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