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@xxxxxxx>
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@xxxxxxx>
Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
---
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
