[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Kevin Brodsky]

arm64/kernel/{vdso,signal}.c include vdso-offsets.h, as well as any
file that includes asm/vdso.h. Therefore, vdso-offsets.h must be
generated before these files are compiled.

The current rules in arm64/kernel/Makefile do not actually enforce
this, because even though $(obj)/vdso is listed as a prerequisite for
vdso-offsets.h, this does not result in the intended effect of
building the vdso subdirectory (before all the other objects). As a
consequence, depending on the order in which the rules are followed,
vdso-offsets.h is updated or not before arm64/kernel/{vdso,signal}.o
are built. The current rules also impose an unnecessary dependency on
vdso-offsets.h for all arm64/kernel/*.o, resulting in unnecessary
rebuilds. This is made obvious when using make -j:

  touch arch/arm64/kernel/vdso/gettimeofday.S && make -j$NCPUS arch/arm64/kernel

will sometimes result in none of arm64/kernel/*.o being
rebuilt, sometimes all of them, or even just some of them.

It is quite difficult to ensure that a header is generated before it
is used with recursive Makefiles by using normal rules.  Instead,
arch-specific generated headers are normally built in the archprepare
recipe in the arch Makefile (see for instance arch/ia64/Makefile).
Unfortunately, asm-offsets.h is included in gettimeofday.S, and must
therefore be generated before vdso-offsets.h, which is not the case if
archprepare is used. For this reason, a rule run after archprepare has
to be used.

This commit adds rules in arm64/Makefile to build vdso-offsets.h
during the prepare step, ensuring that vdso-offsets.h is generated
before building anything. It also removes the now-unnecessary
dependencies on vdso-offsets.h in arm64/kernel/Makefile. Finally, it
removes the duplication of asm-offsets.h between arm64/kernel/vdso/
and include/generated/ and makes include/generated/vdso-offsets.h a
target in arm64/kernel/vdso/Makefile.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Michal Marek <mmarek@suse.com>
Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---

Resending with linux-kbuild in cc, hopefully some people there will have
an opinion on this.

I am not completely satisfied with the fix, since it uses a hack with
the prepare and prepare0 rules that should not be used in arch
Makefiles. However, all of my other attempts (including explicit
dependencies on gettimeofday.S, etc. in arm64/kernel/Makefile) failed
in some way. Hopefully, a Makefile wizard will come up with a better
solution.

 arch/arm64/Makefile             | 10 ++++++++++
 arch/arm64/kernel/Makefile      |  4 ----
 arch/arm64/kernel/vdso/Makefile |  7 +++----
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 354d75402ace..7bb702a0f1e2 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -119,6 +119,16 @@ archclean:
 	$(Q)$(MAKE) $(clean)=$(boot)
 	$(Q)$(MAKE) $(clean)=$(boot)/dts
 
+# We need to generate vdso-offsets.h before compiling certain files in kernel/.
+# In order to do that, we should use the archprepare target, but we can't since
+# asm-offsets.h is included in some files used to generate vdso-offsets.h, and
+# asm-offsets.h is built in prepare0, for which archprepare is a dependency.
+# Therefore we need to generate the header after prepare0 has been made, hence
+# this hack.
+prepare: vdso_prepare
+vdso_prepare: prepare0
+	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
+
 define archhelp
   echo  '* Image.gz      - Compressed kernel image (arch/$(ARCH)/boot/Image.gz)'
   echo  '  Image         - Uncompressed kernel image (arch/$(ARCH)/boot/Image)'
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 3793003e16a2..bab85990691d 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -50,7 +50,3 @@ obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
 head-y					:= head.o
 extra-y					+= $(head-y) vmlinux.lds
-
-# vDSO - this must be built first to generate the symbol offsets
-$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
-$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index b467fd0a384b..62c84f7cb01b 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -23,7 +23,7 @@ GCOV_PROFILE := n
 ccflags-y += -Wl,-shared
 
 obj-y += vdso.o
-extra-y += vdso.lds vdso-offsets.h
+extra-y += vdso.lds
 CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
 
 # Force dependency (incbin is bad)
@@ -42,11 +42,10 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
 gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
 quiet_cmd_vdsosym = VDSOSYM $@
 define cmd_vdsosym
-	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@ && \
-	cp $@ include/generated/
+	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
 endef
 
-$(obj)/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
+include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
 	$(call if_changed,vdsosym)
 
 # Assembly rules for the .S files
-- 
2.8.0

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Catalin Marinas]

On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
> +# We need to generate vdso-offsets.h before compiling certain files in kernel/.
> +# In order to do that, we should use the archprepare target, but we can't since
> +# asm-offsets.h is included in some files used to generate vdso-offsets.h, and
> +# asm-offsets.h is built in prepare0, for which archprepare is a dependency.
> +# Therefore we need to generate the header after prepare0 has been made, hence
> +# this hack.
> +prepare: vdso_prepare
> +vdso_prepare: prepare0
> +	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h

This indeed looks dodgy. I'm not sure about the makefile rules but would
the above override the "prepare" target in the top Makefile?

I think a dependency problem we have is that arch/arm64/kernel/signal.o
depends on include/generated/vdso-offsets.h. However, we don't have any
target for the latter, only for
$(objtree)arch/arm64/kernel/vdso/vdso-offsets.h which no-one is
including. Because of this, we have a fake dependency in
arch/arm64/kernel/Makefile.

It's been a long time since I wrote makefiles but I think a simpler
approach may be to just get the include/generated/vdso-offsets.h target
and remove the arch/arm64/kernel/vdso/vdso-offsets.h one entirely like
below. Otherwise, we give up on storing the generated file in
include/generated/ and move arch/arm64/include/asm/vdso.h to
arch/arm64/kernel/vdso/.

-----------8<-------------------------
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 7700c0c23962..9bdacbf59091 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -55,5 +55,4 @@ head-y					:= head.o
 extra-y					+= $(head-y) vmlinux.lds
 
 # vDSO - this must be built first to generate the symbol offsets
-$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
-$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
+$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index b467fd0a384b..44c20e3c9f43 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -23,7 +23,7 @@ GCOV_PROFILE := n
 ccflags-y += -Wl,-shared
 
 obj-y += vdso.o
-extra-y += vdso.lds vdso-offsets.h
+extra-y += vdso.lds ../../../../include/generated/vdso-offsets.h
 CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
 
 # Force dependency (incbin is bad)
@@ -42,11 +42,10 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
 gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
 quiet_cmd_vdsosym = VDSOSYM $@
 define cmd_vdsosym
-	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@ && \
-	cp $@ include/generated/
+	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
 endef
 
-$(obj)/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
+$(obj)/../../../../include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
 	$(call if_changed,vdsosym)
 
 # Assembly rules for the .S files

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Catalin Marinas]

On Wed, Jul 06, 2016 at 06:57:39PM +0100, Catalin Marinas wrote:
> On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
> > +# We need to generate vdso-offsets.h before compiling certain files in kernel/.
> > +# In order to do that, we should use the archprepare target, but we can't since
> > +# asm-offsets.h is included in some files used to generate vdso-offsets.h, and
> > +# asm-offsets.h is built in prepare0, for which archprepare is a dependency.
> > +# Therefore we need to generate the header after prepare0 has been made, hence
> > +# this hack.
> > +prepare: vdso_prepare
> > +vdso_prepare: prepare0
> > +	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
> 
> This indeed looks dodgy. I'm not sure about the makefile rules but would
> the above override the "prepare" target in the top Makefile?
> 
> I think a dependency problem we have is that arch/arm64/kernel/signal.o
> depends on include/generated/vdso-offsets.h. However, we don't have any
> target for the latter, only for
> $(objtree)arch/arm64/kernel/vdso/vdso-offsets.h which no-one is
> including. Because of this, we have a fake dependency in
> arch/arm64/kernel/Makefile.
> 
> -----------8<-------------------------
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 7700c0c23962..9bdacbf59091 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -55,5 +55,4 @@ head-y					:= head.o
>  extra-y					+= $(head-y) vmlinux.lds
>  
>  # vDSO - this must be built first to generate the symbol offsets
> -$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
> -$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
> +$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso

This didn't work either. Basically, when building
arch/arm64/kernel/signal.o for example, it doesn't figure out that
vdso-offsets.h has additional dependencies like the vdso.so.dbg so that
it builds the vdso object first. I'll look for a little longer, maybe I
find a better workaround.

-- 
Catalin

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Catalin Marinas]

On Thu, Jul 07, 2016 at 11:26:01AM +0100, Catalin Marinas wrote:
> On Wed, Jul 06, 2016 at 06:57:39PM +0100, Catalin Marinas wrote:
> > On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
> > > +# We need to generate vdso-offsets.h before compiling certain files in kernel/.
> > > +# In order to do that, we should use the archprepare target, but we can't since
> > > +# asm-offsets.h is included in some files used to generate vdso-offsets.h, and
> > > +# asm-offsets.h is built in prepare0, for which archprepare is a dependency.
> > > +# Therefore we need to generate the header after prepare0 has been made, hence
> > > +# this hack.
> > > +prepare: vdso_prepare
> > > +vdso_prepare: prepare0
> > > +	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
> > 
> > This indeed looks dodgy. I'm not sure about the makefile rules but would
> > the above override the "prepare" target in the top Makefile?
> > 
> > I think a dependency problem we have is that arch/arm64/kernel/signal.o
> > depends on include/generated/vdso-offsets.h. However, we don't have any
> > target for the latter, only for
> > $(objtree)arch/arm64/kernel/vdso/vdso-offsets.h which no-one is
> > including. Because of this, we have a fake dependency in
> > arch/arm64/kernel/Makefile.
> > 
> > -----------8<-------------------------
> > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > index 7700c0c23962..9bdacbf59091 100644
> > --- a/arch/arm64/kernel/Makefile
> > +++ b/arch/arm64/kernel/Makefile
> > @@ -55,5 +55,4 @@ head-y					:= head.o
> >  extra-y					+= $(head-y) vmlinux.lds
> >  
> >  # vDSO - this must be built first to generate the symbol offsets
> > -$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
> > -$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
> > +$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso
> 
> This didn't work either. Basically, when building
> arch/arm64/kernel/signal.o for example, it doesn't figure out that
> vdso-offsets.h has additional dependencies like the vdso.so.dbg so that
> it builds the vdso object first. I'll look for a little longer, maybe I
> find a better workaround.

If I also add the patch below, it works fine but I'm not sure we can
guarantee that kbuild first dives into subdirectories with a parallel
build:

--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -49,11 +49,7 @@ arm64-obj-$(CONFIG_HIBERNATION)		+= hibernate.o hibernate-asm.o
 arm64-obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o	\
 					   cpu-reset.o
 
-obj-y					+= $(arm64-obj-y) vdso/
+obj-y					+= vdso/ $(arm64-obj-y)
 obj-m					+= $(arm64-obj-m)
 head-y					:= head.o
 extra-y					+= $(head-y) vmlinux.lds

-- 
Catalin

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Catalin Marinas]

On Thu, Jul 07, 2016 at 12:23:25PM +0100, Catalin Marinas wrote:
> On Thu, Jul 07, 2016 at 11:26:01AM +0100, Catalin Marinas wrote:
> > On Wed, Jul 06, 2016 at 06:57:39PM +0100, Catalin Marinas wrote:
> > > On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
> > > > +# We need to generate vdso-offsets.h before compiling certain files in kernel/.
> > > > +# In order to do that, we should use the archprepare target, but we can't since
> > > > +# asm-offsets.h is included in some files used to generate vdso-offsets.h, and
> > > > +# asm-offsets.h is built in prepare0, for which archprepare is a dependency.
> > > > +# Therefore we need to generate the header after prepare0 has been made, hence
> > > > +# this hack.
> > > > +prepare: vdso_prepare
> > > > +vdso_prepare: prepare0
> > > > +	$(Q)$(MAKE) $(build)=arch/arm64/kernel/vdso include/generated/vdso-offsets.h
> > > 
> > > This indeed looks dodgy. I'm not sure about the makefile rules but would
> > > the above override the "prepare" target in the top Makefile?
> > > 
> > > I think a dependency problem we have is that arch/arm64/kernel/signal.o
> > > depends on include/generated/vdso-offsets.h. However, we don't have any
> > > target for the latter, only for
> > > $(objtree)arch/arm64/kernel/vdso/vdso-offsets.h which no-one is
> > > including. Because of this, we have a fake dependency in
> > > arch/arm64/kernel/Makefile.
> > > 
> > > -----------8<-------------------------
> > > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > > index 7700c0c23962..9bdacbf59091 100644
> > > --- a/arch/arm64/kernel/Makefile
> > > +++ b/arch/arm64/kernel/Makefile
> > > @@ -55,5 +55,4 @@ head-y					:= head.o
> > >  extra-y					+= $(head-y) vmlinux.lds
> > >  
> > >  # vDSO - this must be built first to generate the symbol offsets
> > > -$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
> > > -$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
> > > +$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso
> > 
> > This didn't work either. Basically, when building
> > arch/arm64/kernel/signal.o for example, it doesn't figure out that
> > vdso-offsets.h has additional dependencies like the vdso.so.dbg so that
> > it builds the vdso object first. I'll look for a little longer, maybe I
> > find a better workaround.
> 
> If I also add the patch below, it works fine but I'm not sure we can
> guarantee that kbuild first dives into subdirectories with a parallel
> build:
> 
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -49,11 +49,7 @@ arm64-obj-$(CONFIG_HIBERNATION)		+= hibernate.o hibernate-asm.o
>  arm64-obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o	\
>  					   cpu-reset.o
>  
> -obj-y					+= $(arm64-obj-y) vdso/
> +obj-y					+= vdso/ $(arm64-obj-y)

This one works "most of the time" but not always. What I found to work
reliably (until further notice ;)) is to force the vdso build like
below:

----------------------8<----------------------------
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 7700c0c23962..366e4f26071f 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -55,5 +55,5 @@ head-y					:= head.o
 extra-y					+= $(head-y) vmlinux.lds
 
 # vDSO - this must be built first to generate the symbol offsets
-$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
-$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
+$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso
+	$(Q)$(MAKE) $(build)=$< $@
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index b467fd0a384b..238bb2db76be 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -23,7 +23,7 @@ GCOV_PROFILE := n
 ccflags-y += -Wl,-shared
 
 obj-y += vdso.o
-extra-y += vdso.lds vdso-offsets.h
+extra-y += vdso.lds ../../../../include/generated/vdso-offsets.h
 CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
 
 # Force dependency (incbin is bad)
@@ -42,11 +42,10 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
 gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
 quiet_cmd_vdsosym = VDSOSYM $@
 define cmd_vdsosym
-	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@ && \
-	cp $@ include/generated/
+	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
 endef
 
-$(obj)/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
+$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
 	$(call if_changed,vdsosym)
 
 # Assembly rules for the .S files
----------------------8<----------------------------

This was my last attempt@solving this. Unless someone has a better
idea or thinkg the above doesn't work (or it's uglier than Kevin's
original patch) I plan to merge it.

Other advantages are that it only generates a single vdso-offets.h file.
It also only rebuilds those objects that depend on vdso-offests.h rather
than everything under arch/arm64/kernel/ (Kevin's patch was also solving
this).

-- 
Catalin

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Catalin Marinas]

On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
> I am not completely satisfied with the fix, since it uses a hack with
> the prepare and prepare0 rules that should not be used in arch
> Makefiles. However, all of my other attempts (including explicit
> dependencies on gettimeofday.S, etc. in arm64/kernel/Makefile) failed
> in some way. Hopefully, a Makefile wizard will come up with a better
> solution.

This is the patch I'm going to push to arm64 for-next/core. Thanks for
the report and attempt at fixing it, it saved me from trying to
understand what was going on:

-------------8<------------------------------------
>From 19a5ab422b6ca3b3c8f656ca6d697dbfb577d360 Mon Sep 17 00:00:00 2001
From: Catalin Marinas <catalin.marinas@arm.com>
Date: Fri, 8 Jul 2016 12:13:20 +0100
Subject: [PATCH] arm64: Fix vdso-offsets.h dependency

arch/arm64/kernel/{vdso,signal}.c include generated/vdso-offsets.h, and
therefore the symbol offsets must be generated before these files are
compiled.

The current rules in arm64/kernel/Makefile do not actually enforce
this, because even though $(obj)/vdso is listed as a prerequisite for
vdso-offsets.h, this does not result in the intended effect of
building the vdso subdirectory (before all the other objects). As a
consequence, depending on the order in which the rules are followed,
vdso-offsets.h is updated or not before arm64/kernel/{vdso,signal}.o
are built. The current rules also impose an unnecessary dependency on
vdso-offsets.h for all arm64/kernel/*.o, resulting in unnecessary
rebuilds.

This patch removes the arch/arm64/kernel/vdso/vdso-offsets.h file
generation, leaving only the include/generated/vdso-offsets.h one. It
adds a forced dependency check of the vdso-offsets.h file in
arch/arm64/kernel/Makefile which, if not up to date according to the
arch/arm64/kernel/vdso/Makefile rules (depending on vdso.so.dbg), will
trigger the vdso/ subdirectory build and vdso-offsets.h re-generation.
Automatic kbuild dependency rules between kernel/{vdso,signal}.c rules
and vdso-offsets.h will guarantee that the vDSO object is built first,
followed by the generated symbol offsets header file.

Reported-by: Kevin Brodsky <kevin.brodsky@arm.com>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/kernel/Makefile      | 7 ++++---
 arch/arm64/kernel/vdso/Makefile | 7 +++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 7700c0c23962..b4f0a03dc830 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -54,6 +54,7 @@ obj-m					+= $(arm64-obj-m)
 head-y					:= head.o
 extra-y					+= $(head-y) vmlinux.lds
 
-# vDSO - this must be built first to generate the symbol offsets
-$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
-$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
+# Check that the vDSO symbol offsets header file is up to date and re-generate
+# it if necessary.
+$(objtree)/include/generated/vdso-offsets.h: FORCE
+	$(Q)$(MAKE) $(build)=$(obj)/vdso $@
diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index b467fd0a384b..70fb663ddf7b 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -23,7 +23,7 @@ GCOV_PROFILE := n
 ccflags-y += -Wl,-shared
 
 obj-y += vdso.o
-extra-y += vdso.lds vdso-offsets.h
+extra-y += vdso.lds
 CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
 
 # Force dependency (incbin is bad)
@@ -42,11 +42,10 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
 gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
 quiet_cmd_vdsosym = VDSOSYM $@
 define cmd_vdsosym
-	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@ && \
-	cp $@ include/generated/
+	$(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
 endef
 
-$(obj)/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
+$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg
 	$(call if_changed,vdsosym)
 
 # Assembly rules for the .S files

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Kevin Brodsky]

Hi Catalin,

On 08/07/16 12:27, Catalin Marinas wrote:
> On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
>> I am not completely satisfied with the fix, since it uses a hack with
>> the prepare and prepare0 rules that should not be used in arch
>> Makefiles. However, all of my other attempts (including explicit
>> dependencies on gettimeofday.S, etc. in arm64/kernel/Makefile) failed
>> in some way. Hopefully, a Makefile wizard will come up with a better
>> solution.
> This is the patch I'm going to push to arm64 for-next/core. Thanks for
> the report and attempt at fixing it, it saved me from trying to
> understand what was going on:

First, thanks for taking care of this! Sorry for the delay in replying, I've been
having trouble recently with my email client not showing up new messages in subfolders...

Now, unfortunately, I had already tried this solution (I think almost exactly this
patch in fact), and it does not work. I confirmed this just now by applying the patch
on master and compiling from a clean tree. The compilation of signal.c failed with:

 > In file included from ../arch/arm64/kernel/signal.c:36:0:
 > ../arch/arm64/include/asm/vdso.h:30:36: fatal error: generated/vdso-offsets.h: No
such file or directory
 >  #include <generated/vdso-offsets.h>

Note that I compile with 40 threads (make -j40), and that's the core of the problem.
Indeed, by adding the forced dependency on
$(objtree)/include/generated/vdso-offsets.h in kernel/Makefile, you say that the
recipe must always be run, but you don't say that it has to be run before any other
*.c file in the directory is compiled. When compiling with a single thread (or maybe
only a small number), this happens to work because the Makefile is executed more or
less sequentially, but with a bigger number of threads it breaks quite easily.

Therefore, please do not merge this patch, it can break the compilation quite easily.

Back to the problem, I think I haven't been clear enough: there is _no way_ with
Kbuild to ensure that a header is generated before its sources are compiled, using
only normal Makefile dependencies. We _must_ generate the header before recursing
into any Makefile that compiles files depending on this header. Assuming that we have
no choice but to keep this vdso-offsets.h header, there is no way around modifying
arm64/Makefile to ensure that it is generated first.

To reply to your concern about my patch:

 > This indeed looks dodgy. I'm not sure about the makefile rules but would the above
override the "prepare" target in the top Makefile?

Rules are cumulative, they do not override each other. I am only making
"vdso_prepare" an additional prerequisite of "prepare", with "vdso_prepare" depending
on "prepare0" to ensure that asm-offsets.h is generated first. What is dodgy is that
we are not supposed to add prerequisites to "prepare" in arch Makefiles, but again, I
don't see how we can avoid doing that here. It seems to me that this is an oversight
in the top-level Makefile, and I don't think that adding a prerequisite to "prepare"
is unreasonable.

Thanks,
Kevin

>
> -------------8<------------------------------------
>  From 19a5ab422b6ca3b3c8f656ca6d697dbfb577d360 Mon Sep 17 00:00:00 2001
> From: Catalin Marinas <catalin.marinas@arm.com>
> Date: Fri, 8 Jul 2016 12:13:20 +0100
> Subject: [PATCH] arm64: Fix vdso-offsets.h dependency
>
> arch/arm64/kernel/{vdso,signal}.c include generated/vdso-offsets.h, and
> therefore the symbol offsets must be generated before these files are
> compiled.
>
> The current rules in arm64/kernel/Makefile do not actually enforce
> this, because even though $(obj)/vdso is listed as a prerequisite for
> vdso-offsets.h, this does not result in the intended effect of
> building the vdso subdirectory (before all the other objects). As a
> consequence, depending on the order in which the rules are followed,
> vdso-offsets.h is updated or not before arm64/kernel/{vdso,signal}.o
> are built. The current rules also impose an unnecessary dependency on
> vdso-offsets.h for all arm64/kernel/*.o, resulting in unnecessary
> rebuilds.
>
> This patch removes the arch/arm64/kernel/vdso/vdso-offsets.h file
> generation, leaving only the include/generated/vdso-offsets.h one. It
> adds a forced dependency check of the vdso-offsets.h file in
> arch/arm64/kernel/Makefile which, if not up to date according to the
> arch/arm64/kernel/vdso/Makefile rules (depending on vdso.so.dbg), will
> trigger the vdso/ subdirectory build and vdso-offsets.h re-generation.
> Automatic kbuild dependency rules between kernel/{vdso,signal}.c rules
> and vdso-offsets.h will guarantee that the vDSO object is built first,
> followed by the generated symbol offsets header file.
>
> Reported-by: Kevin Brodsky <kevin.brodsky@arm.com>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>   arch/arm64/kernel/Makefile      | 7 ++++---
>   arch/arm64/kernel/vdso/Makefile | 7 +++----
>   2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 7700c0c23962..b4f0a03dc830 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -54,6 +54,7 @@ obj-m                                       += $(arm64-obj-m)
>   head-y                                      := head.o
>   extra-y                                     += $(head-y) vmlinux.lds
>
> -# vDSO - this must be built first to generate the symbol offsets
> -$(call objectify,$(arm64-obj-y)): $(obj)/vdso/vdso-offsets.h
> -$(obj)/vdso/vdso-offsets.h: $(obj)/vdso
> +# Check that the vDSO symbol offsets header file is up to date and re-generate
> +# it if necessary.
> +$(objtree)/include/generated/vdso-offsets.h: FORCE
> +     $(Q)$(MAKE) $(build)=$(obj)/vdso $@
> diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
> index b467fd0a384b..70fb663ddf7b 100644
> --- a/arch/arm64/kernel/vdso/Makefile
> +++ b/arch/arm64/kernel/vdso/Makefile
> @@ -23,7 +23,7 @@ GCOV_PROFILE := n
>   ccflags-y += -Wl,-shared
>
>   obj-y += vdso.o
> -extra-y += vdso.lds vdso-offsets.h
> +extra-y += vdso.lds
>   CPPFLAGS_vdso.lds += -P -C -U$(ARCH)
>
>   # Force dependency (incbin is bad)
> @@ -42,11 +42,10 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
>   gen-vdsosym := $(srctree)/$(src)/gen_vdso_offsets.sh
>   quiet_cmd_vdsosym = VDSOSYM $@
>   define cmd_vdsosym
> -     $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@ && \
> -     cp $@ include/generated/
> +     $(NM) $< | $(gen-vdsosym) | LC_ALL=C sort > $@
>   endef
>
> -$(obj)/vdso-offsets.h: $(obj)/vdso.so.dbg FORCE
> +$(objtree)/include/generated/vdso-offsets.h: $(obj)/vdso.so.dbg
>       $(call if_changed,vdsosym)
>
>   # Assembly rules for the .S files
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Catalin Marinas]

On Mon, Jul 11, 2016 at 04:29:26PM +0100, Kevin Brodsky wrote:
> On 08/07/16 12:27, Catalin Marinas wrote:
> >On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
> >>I am not completely satisfied with the fix, since it uses a hack with
> >>the prepare and prepare0 rules that should not be used in arch
> >>Makefiles. However, all of my other attempts (including explicit
> >>dependencies on gettimeofday.S, etc. in arm64/kernel/Makefile) failed
> >>in some way. Hopefully, a Makefile wizard will come up with a better
> >>solution.
> >This is the patch I'm going to push to arm64 for-next/core. Thanks for
> >the report and attempt at fixing it, it saved me from trying to
> >understand what was going on:
> 
> First, thanks for taking care of this! Sorry for the delay in replying, I've been
> having trouble recently with my email client not showing up new messages in subfolders...
> 
> Now, unfortunately, I had already tried this solution (I think almost exactly this
> patch in fact), and it does not work. I confirmed this just now by applying the patch
> on master and compiling from a clean tree.The compilation of signal.c failed with:

I noticed this as well after an mrproper. The code seemed to be compiled
in order as long as there was an original generated/asm-offsets.h in
place.

> Therefore, please do not merge this patch, it can break the compilation quite easily.

Too late ;). But I'm reverting it now.

> > This indeed looks dodgy. I'm not sure about the makefile rules but would the above
> > override the "prepare" target in the top Makefile?
> 
> Rules are cumulative, they do not override each other. I am only making
> "vdso_prepare" an additional prerequisite of "prepare", with "vdso_prepare" depending
> on "prepare0" to ensure that asm-offsets.h is generated first. What is dodgy is that
> we are not supposed to add prerequisites to "prepare" in arch Makefiles, but again, I
> don't see how we can avoid doing that here. It seems to me that this is an oversight
> in the top-level Makefile, and I don't think that adding a prerequisite to "prepare"
> is unreasonable.

I'll merge your patch. An alternative would be to parse the vdso ELF at
run-time in the kernel and generate the offsets.

-- 
Catalin

[PATCH RESEND] arm64: fix vdso-offsets.h dependency

[Kevin Brodsky]

On 11/07/16 17:19, Catalin Marinas wrote:
> On Mon, Jul 11, 2016 at 04:29:26PM +0100, Kevin Brodsky wrote:
>> On 08/07/16 12:27, Catalin Marinas wrote:
>>> On Thu, May 12, 2016 at 05:39:15PM +0100, Kevin Brodsky wrote:
>>>> I am not completely satisfied with the fix, since it uses a hack with
>>>> the prepare and prepare0 rules that should not be used in arch
>>>> Makefiles. However, all of my other attempts (including explicit
>>>> dependencies on gettimeofday.S, etc. in arm64/kernel/Makefile) failed
>>>> in some way. Hopefully, a Makefile wizard will come up with a better
>>>> solution.
>>> This is the patch I'm going to push to arm64 for-next/core. Thanks for
>>> the report and attempt at fixing it, it saved me from trying to
>>> understand what was going on:
>> First, thanks for taking care of this! Sorry for the delay in replying, I've been
>> having trouble recently with my email client not showing up new messages in subfolders...
>>
>> Now, unfortunately, I had already tried this solution (I think almost exactly this
>> patch in fact), and it does not work. I confirmed this just now by applying the patch
>> on master and compiling from a clean tree.The compilation of signal.c failed with:
> I noticed this as well after an mrproper. The code seemed to be compiled
> in order as long as there was an original generated/asm-offsets.h in
> place.
>
>> Therefore, please do not merge this patch, it can break the compilation quite easily.
> Too late ;). But I'm reverting it now.
>
>>> This indeed looks dodgy. I'm not sure about the makefile rules but would the above
>>> override the "prepare" target in the top Makefile?
>> Rules are cumulative, they do not override each other. I am only making
>> "vdso_prepare" an additional prerequisite of "prepare", with "vdso_prepare" depending
>> on "prepare0" to ensure that asm-offsets.h is generated first. What is dodgy is that
>> we are not supposed to add prerequisites to "prepare" in arch Makefiles, but again, I
>> don't see how we can avoid doing that here. It seems to me that this is an oversight
>> in the top-level Makefile, and I don't think that adding a prerequisite to "prepare"
>> is unreasonable.
> I'll merge your patch. An alternative would be to parse the vdso ELF at
> run-time in the kernel and generate the offsets.
>

Okay thanks! Yes runtime inspection would also work, but I think it's clearly more
acceptable to have a small hack in a Makefile than deferring the work to runtime.

Kevin

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.