[KBUILD] allow any PHONY in if_changed_dep

[KBUILD] allow any PHONY in if_changed_dep

[Milton Miller]

While all the if_changed family filter $(PHONY) from the list of newer
files, if_changed_dep was only filtering FORCE from the list of all
dependents.  This resulted in forced recompile every time.

Signed-off-by: Milton Miller <miltonm@bga.com>

--- 

Sam,

I have been working on some cleanups in arch/powerpc/boot/Makefile,
switching to if_changed and friends.  There are rules to copy headers
from the kernel environment to the boot environment, applying slight
cleanups.

Currently a subset of the c files depend on the headers, and there is
a manual dependency on the headers.  I was trying to get if_changed_dep
to depend only on the headers actually used, but copy all needed headers
before compiling any C files in the zImage code.  

Does the following patch look ok to you?   It removes all PHONY targets
from the list of all dependents, not just FORCE.  I'm thinking the clause
is to catch files haven't been generated and therefore are not newer but
are still required.  If you want to build every time you can just call cmd.

milton

Two excerpts from the proposed makefile follow:

> -$(addprefix $(obj)/,$(zlib) main.o): $(addprefix $(obj)/,$(zliblinuxheader)) $(addprefix $(obj)/,$(zlibheader))
> -#$(addprefix $(obj)/,main.o): $(addprefix $(obj)/,zlib.h)
> +linuxheader := zlib.h zconf.h zutil.h
> 
> [ $(obj-boot): %.o: %.c ]
>
> +$(obj-boot):  COPYHEADERS
> +COPYHEADERS:	$(addprefix $(obj)/,$(linuxheader) $(zlibheader))
> +PHONY	+= COPYHEADERS


Index: kernel/scripts/Kbuild.include
===================================================================
--- kernel.orig/scripts/Kbuild.include	2006-07-01 00:58:38.926249877 -0500
+++ kernel/scripts/Kbuild.include	2006-07-01 01:02:00.909937514 -0500
@@ -131,7 +131,7 @@ if_changed = $(if $(strip $(filter-out $
 # execute the command and also postprocess generated .d dependencies
 # file
 if_changed_dep = $(if $(strip $(filter-out $(PHONY),$?)  \
-		$(filter-out FORCE $(wildcard $^),$^)    \
+		$(filter-out $(PHONY) $(wildcard $^),$^)    \
 	$(call arg-check, $(cmd_$(1)), $(cmd_$@)) ),     \
 	@set -e; \
 	$(echo-cmd) $(cmd_$(1)); \

Re: [KBUILD] allow any PHONY in if_changed_dep

[Sam Ravnborg]

On Sat, Jul 01, 2006 at 04:16:56AM -0500, Milton Miller wrote:
> While all the if_changed family filter $(PHONY) from the list of newer
> files, if_changed_dep was only filtering FORCE from the list of all
> dependents.  This resulted in forced recompile every time.

I see where you are heading with this. But on the principle of minimal
suprise this is not good. Why does it not include file.o in the link
when I mark it phony?

The root cause is buried in the powerpc/boot/Makefile restructuring.
I do not see what is wrong with current approch if you do a simple
s/cmd/if_changed/g and then assign targets += <relevant targets>

You need to post a full diff of the Makefile before I can give better
feedback.

	Sam

Re: [KBUILD] allow any PHONY in if_changed_dep

[Milton Miller]

On Jul 1, 2006, at 4:39 AM, Sam Ravnborg wrote:

> On Sat, Jul 01, 2006 at 04:16:56AM -0500, Milton Miller wrote:
>> While all the if_changed family filter $(PHONY) from the list of newer
>> files, if_changed_dep was only filtering FORCE from the list of all
>> dependents.  This resulted in forced recompile every time.
>
> I see where you are heading with this. But on the principle of minimal
> suprise this is not good. Why does it not include file.o in the link
> when I mark it phony?

This is your suprise argument, right?  (Not my current problem).

Actually this is only changing weither file.o gets considered in
causing the build command to be executed.

> The root cause is buried in the powerpc/boot/Makefile restructuring.
> I do not see what is wrong with current approch if you do a simple
> s/cmd/if_changed/g and then assign targets += <relevant targets>
>

The issue is I need headers copied for some of the files.  The current
approach is any file that might need a copied header gets a dependency
on all of the copied .h files.  I was planning on coping some more
headers used in different files.  if_changed_dep

The issue is specifically COPYFILES.  The .o can not be built until
its dependencys are met, but it never exists itself.  The actual .h
files a given source will be detected by if_changed_dep and fixdep.

> You need to post a full diff of the Makefile before I can give better
> feedback.
>

Sure.  I'm reposting the series, I'll cc you and copy the makefile
patch to linux-kernel, its 3/5.    That patch is independent of 1 and 2 
while 4 and 5 require them.  The full series is available on the 
linuxppc-dev mailing list.

milton

[3/5][POWERPC] boot: use more Kbuild rules

[Milton Miller]

Switch to if_changed_dep for bootcc, cmd for known rebuilds without deps,
and if_changed for other commands.  Add FORCE for all if_changed rule uses.
Make sure targets includes all generated files.

Move clean-files to targets for if_changed files.

Rename OBJCOPYFLAGS to OBJCOPY_SEC_FLAGS so we can use $(objcopy) rule.

Rename zliblinuxheader to linuxheader

Change from explicit dependency of main and zlib files and zlib headers
to generate all headers before compiling the source.  fixdep will give
us the exact headers, but not on a clean compile.

Note: the obj-sec .c files are created/touched when the content file changes.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: kernel/arch/powerpc/boot/Makefile
===================================================================
--- kernel.orig/arch/powerpc/boot/Makefile	2006-07-01 00:03:41.940189322 -0500
+++ kernel/arch/powerpc/boot/Makefile	2006-07-01 01:33:55.937370129 -0500
@@ -25,16 +25,13 @@ HOSTCC		:= gcc
 BOOTCFLAGS	:= $(HOSTCFLAGS) -fno-builtin -nostdinc -isystem \
 		   $(shell $(CROSS32CC) -print-file-name=include) -fPIC
 BOOTAFLAGS	:= -D__ASSEMBLY__ $(BOOTCFLAGS) -traditional -nostdinc
-OBJCOPYFLAGS    := contents,alloc,load,readonly,data
+OBJCOPY_SEC_FLAGS := contents,alloc,load,readonly,data
 OBJCOPY_COFF_ARGS := -O aixcoff-rs6000 --set-start 0x500000
 OBJCOPY_MIB_ARGS  := -O aixcoff-rs6000 -R .stab -R .stabstr -R .comment
 
 zlib       := infblock.c infcodes.c inffast.c inflate.c inftrees.c infutil.c
 zlibheader := infblock.h infcodes.h inffast.h inftrees.h infutil.h
-zliblinuxheader := zlib.h zconf.h zutil.h
-
-$(addprefix $(obj)/,$(zlib) main.o): $(addprefix $(obj)/,$(zliblinuxheader)) $(addprefix $(obj)/,$(zlibheader))
-#$(addprefix $(obj)/,main.o): $(addprefix $(obj)/,zlib.h)
+linuxheader := zlib.h zconf.h zutil.h
 
 src-boot := crt0.S string.S prom.c stdio.c main.c div64.S
 src-boot += $(zlib)
@@ -48,20 +45,23 @@ quiet_cmd_copy_zlib = COPY    $@
 
 quiet_cmd_copy_zlibheader = COPY    $@
       cmd_copy_zlibheader = sed "s@<linux/\([^>]\+\).*@\"\1\"@" $< > $@
+
 # stddef.h for NULL
-quiet_cmd_copy_zliblinuxheader = COPY    $@
-      cmd_copy_zliblinuxheader = sed "s@<linux/string.h>@\"string.h\"@;s@<linux/kernel.h>@<stddef.h>@;s@<linux/\([^>]\+\).*@\"\1\"@" $< > $@
+quiet_cmd_copy_linuxheader = COPY    $@
+      cmd_copy_linuxheader = sed "s@<linux/string.h>@\"string.h\"@;s@<linux/kernel.h>@<stddef.h>@;s@<linux/\([^>]\+\).*@\"\1\"@" $< > $@
 
-$(addprefix $(obj)/,$(zlib)): $(obj)/%: $(srctree)/lib/zlib_inflate/%
-	$(call cmd,copy_zlib)
+$(addprefix $(obj)/,$(zlib)): $(obj)/%: $(srctree)/lib/zlib_inflate/% FORCE
+	$(call if_changed,copy_zlib)
 
-$(addprefix $(obj)/,$(zlibheader)): $(obj)/%: $(srctree)/lib/zlib_inflate/%
-	$(call cmd,copy_zlibheader)
+$(addprefix $(obj)/,$(zlibheader)): $(obj)/%: $(srctree)/lib/zlib_inflate/% \
+		FORCE
+	$(call if_changed,copy_zlibheader)
 
-$(addprefix $(obj)/,$(zliblinuxheader)): $(obj)/%: $(srctree)/include/linux/%
-	$(call cmd,copy_zliblinuxheader)
+$(addprefix $(obj)/,$(linuxheader)): $(obj)/%: $(srctree)/include/linux/% FORCE
+	$(call if_changed,copy_linuxheader)
 
-clean-files := $(zlib) $(zlibheader) $(zliblinuxheader)
+targets += $(zlib) $(zlibheader) $(linuxheader)
+targets += $(patsubst $(obj)/%,%, $(obj-boot))
 
 
 quiet_cmd_bootcc = BOOTCC  $@
@@ -73,11 +73,16 @@ quiet_cmd_bootas = BOOTAS  $@
 quiet_cmd_bootld = BOOTLD  $@
       cmd_bootld = $(CROSS32LD) -T $(srctree)/$(src)/$(3) -o $@ $(2)
 
-$(patsubst %.c,%.o, $(filter %.c, $(src-boot))): %.o: %.c
+$(patsubst %.c,%.o, $(filter %.c, $(src-boot))): %.o: %.c FORCE
 	$(call if_changed_dep,bootcc)
-$(patsubst %.S,%.o, $(filter %.S, $(src-boot))): %.o: %.S
+
+$(patsubst %.S,%.o, $(filter %.S, $(src-boot))): %.o: %.S FORCE
 	$(call if_changed_dep,bootas)
 
+$(obj-boot):  COPYHEADERS
+COPYHEADERS:	$(addprefix $(obj)/,$(linuxheader) $(zlibheader))
+PHONY	+= COPYHEADERS
+
 #-----------------------------------------------------------
 # ELF sections within the zImage bootloader/wrapper
 #-----------------------------------------------------------
@@ -104,18 +109,20 @@ quiet_cmd_ramdisk = RAMDISK $@
 quiet_cmd_stripvm = STRIP   $@
       cmd_stripvm = $(STRIP) -s -R .comment $< -o $@
 
-vmlinux.strip: vmlinux
+vmlinux.strip: vmlinux FORCE
 	$(call if_changed,stripvm)
-$(obj)/vmlinux.initrd: vmlinux.strip $(obj)/addRamDisk $(obj)/ramdisk.image.gz
+$(obj)/vmlinux.initrd: vmlinux.strip $(obj)/addRamDisk \
+		$(obj)/ramdisk.image.gz FORCE
 	$(call if_changed,ramdisk)
 
 quiet_cmd_addsection = ADDSEC  $@
       cmd_addsection = $(CROSS32OBJCOPY) $@ \
 		--add-section=.kernel:$(strip $(patsubst $(obj)/kernel-%.o,%, $@))=$(patsubst %.o,%.gz, $@) \
-		--set-section-flags=.kernel:$(strip $(patsubst $(obj)/kernel-%.o,%, $@))=$(OBJCOPYFLAGS)
+		--set-section-flags=.kernel:$(strip $(patsubst $(obj)/kernel-%.o,%, $@))=$(OBJCOPY_SEC_FLAGS)
 
 quiet_cmd_addnote = ADDNOTE $@
-      cmd_addnote = $(obj)/addnote $@
+      cmd_addnote = @cp -f $< $@ && \
+		    $(obj)/addnote $@
 
 quiet_cmd_gen-miboot = GEN     $@
       cmd_gen-miboot = $(OBJCOPY) $(OBJCOPY_MIB_ARGS) \
@@ -125,7 +132,19 @@ quiet_cmd_gencoff = COFF    $@
       cmd_gencoff = $(OBJCOPY) $(OBJCOPY_COFF_ARGS) $@ && \
 		    $(obj)/hack-coff $@
 
-$(call gz-sec, $(required)): $(obj)/kernel-%.gz: %
+# a single rule so that we can do if_changed correctly
+define rule_ldcoff
+	$(call echo-cmd,bootld) $(cmd_bootld);			  \
+	$(call echo-cmd,gencoff) $(cmd_gencoff)
+endef
+
+# $(required) is built in $(objtree) not $(obj) so read cmd files manually
+cmd_files := $(wildcard $(foreach f,$(required),$(dir $(f)).$(notdir $(f)).cmd))
+ifneq ($(cmd_files),)
+  include $(cmd_files)
+endif
+
+$(call gz-sec, $(required)): $(obj)/kernel-%.gz: % FORCE
 	$(call if_changed,gzip)
 
 $(obj)/kernel-initrd.gz: $(obj)/ramdisk.image.gz
@@ -135,16 +154,16 @@ $(call src-sec, $(required) $(initrd)): 
 	@touch $@
 
 $(call obj-sec, $(required) $(initrd)): $(obj)/kernel-%.o: $(obj)/kernel-%.c
-	$(call if_changed_dep,bootcc)
+	$(call cmd,bootcc)
 	$(call cmd,addsection)
 
 $(obj)/zImage.vmode $(obj)/zImage.coff: obj-boot += $(call obj-sec, $(required))
-$(obj)/zImage.vmode: $(call obj-sec, $(required)) $(obj-boot) $(srctree)/$(src)/zImage.lds
-	$(call cmd,bootld,$(obj-boot),zImage.lds)
+$(obj)/zImage.vmode: $(call obj-sec, $(required)) $(obj-boot) $(srctree)/$(src)/zImage.lds FORCE
+	$(call if_changed,bootld,$(obj-boot),zImage.lds)
 
 $(obj)/zImage.initrd.vmode $(obj)/zImage.initrd.coff: obj-boot += $(call obj-sec, $(required) $(initrd))
-$(obj)/zImage.initrd.vmode: $(call obj-sec, $(required) $(initrd)) $(obj-boot) $(srctree)/$(src)/zImage.lds
-	$(call cmd,bootld,$(obj-boot),zImage.lds)
+$(obj)/zImage.initrd.vmode: $(call obj-sec, $(required) $(initrd)) $(obj-boot) $(srctree)/$(src)/zImage.lds FORCE
+	$(call if_changed,bootld,$(obj-boot),zImage.lds)
 
 # For 32-bit powermacs, build the COFF and miboot images
 # as well as the ELF images.
@@ -154,30 +173,29 @@ mibootimg-$(CONFIG_PPC_PMAC)-$(CONFIG_PP
 mibrdimg-$(CONFIG_PPC_PMAC)-$(CONFIG_PPC32)  := $(obj)/miboot.initrd.image
 
 $(obj)/zImage: $(obj)/zImage.vmode $(obj)/addnote $(coffimage-y-y) \
-			$(mibootimg-y-y)
-	@cp -f $< $@
+			$(mibootimg-y-y) FORCE
 	$(call if_changed,addnote)
+	@true
 
 $(obj)/zImage.initrd: $(obj)/zImage.initrd.vmode $(obj)/addnote \
-			$(coffrdimg-y-y) $(mibrdimg-y-y)
-	@cp -f $< $@
+			$(coffrdimg-y-y) $(mibrdimg-y-y) FORCE
 	$(call if_changed,addnote)
+	@true
 
 $(obj)/zImage.coff: $(call obj-sec, $(required)) $(obj-boot) \
-			$(srctree)/$(src)/zImage.coff.lds $(obj)/hack-coff
-	$(call cmd,bootld,$(obj-boot),zImage.coff.lds)
-	$(call cmd,gencoff)
+			$(srctree)/$(src)/zImage.coff.lds $(obj)/hack-coff FORCE
+	$(call if_changed_rule,ldcoff,$(obj-boot),zImage.coff.lds)
 
 $(obj)/zImage.initrd.coff: $(call obj-sec, $(required) $(initrd)) $(obj-boot) \
-			   $(srctree)/$(src)/zImage.coff.lds $(obj)/hack-coff
-	$(call cmd,bootld,$(obj-boot),zImage.coff.lds)
-	$(call cmd,gencoff)
+			   $(srctree)/$(src)/zImage.coff.lds $(obj)/hack-coff \
+			   FORCE
+	$(call if_changed_rule,ldcoff,$(obj-boot),zImage.coff.lds)
 
-$(obj)/miboot.image: $(obj)/dummy.o $(obj)/vmlinux.gz
-	$(call cmd,gen-miboot,image)
+$(obj)/miboot.image: $(obj)/dummy.o $(obj)/vmlinux.gz FORCE
+	$(call if_changed,gen-miboot,image)
 
-$(obj)/miboot.initrd.image: $(obj)/miboot.image $(images)/ramdisk.image.gz
-	$(call cmd,gen-miboot,initrd)
+$(obj)/miboot.initrd.image: $(obj)/miboot.image $(images)/ramdisk.image.gz FORCE
+	$(call if_changed,gen-miboot,initrd)
 
 #-----------------------------------------------------------
 # build u-boot images
@@ -199,6 +217,7 @@ extra-y		+= vmlinux.bin vmlinux.gz
 
 $(obj)/vmlinux.bin: vmlinux FORCE
 	$(call if_changed,objbin)
+	@true
 
 $(obj)/vmlinux.gz: $(obj)/vmlinux.bin FORCE
 	$(call if_changed,mygzip)

Re: [KBUILD] allow any PHONY in if_changed_dep

[Sam Ravnborg]

On Wed, Jul 05, 2006 at 10:02:08AM -0500, Milton Miller wrote:
> While all the if_changed family filter $(PHONY) from the list of newer
> files, if_changed_dep was only filtering FORCE from the list of all
> dependents.  This resulted in forced recompile every time.

I have just committed following change to kbuild.git.
It should address your input while making parts of Kbuild.include almost
readable too.


Working on the "why did we build this target path" at the moment.
Will have it finished tonight I hope.

Thanks for your kbuild work!
Even if I do not apply them verbatim it triggers me to do some long needed
improvements.

	Sam

commit f9361616df9f8fc80a5bbe8c0a67cf964a10cbf8
Author: Sam Ravnborg <sam@mars.ravnborg.org>
Date:   Wed Jul 5 16:46:02 2006 +0200

    kbuild: clean up scripts/Kbuild.include
    
    Makes part of Kbuild.include remotely readable by
    refactoring some functionality to independent variables.
    The different if_xxx are now consistent in their way of determining
    if a target needs to be rebuild.
    
    Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 2180c88..e36f916 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -8,9 +8,13 @@ empty   :=
 space   := $(empty) $(empty)
 
 ###
+# Name of target with a '.' added in front. foo/bar.o => foo/.bar.o
+dot-target = $(dir $@).$(notdir $@)
+
+###
 # The temporary file to save gcc -MD generated dependencies must not
 # contain a comma
-depfile = $(subst $(comma),_,$(@D)/.$(@F).d)
+depfile = $(subst $(comma),_,$(dot-target).d)
 
 ###
 # filename of target with directory and extension stripped
@@ -113,7 +117,8 @@ # See Documentation/kbuild/makefiles.txt
 ifneq ($(KBUILD_NOCMDDEP),1)
 # Check if both arguments has same arguments. Result in empty string if equal
 # User may override this check using make KBUILD_NOCMDDEP=1
-arg-check = $(strip $(filter-out $(1), $(2)) $(filter-out $(2), $(1)) )
+arg-check = $(strip $(filter-out $(cmd_$(1)), $(cmd_$@)) \
+                    $(filter-out $(cmd_$@),   $(cmd_$(1))) )
 endif
 
 # echo command. Short version is $(quiet) equals quiet, otherwise full command
@@ -122,31 +127,34 @@ echo-cmd = $(if $($(quiet)cmd_$(1)), \
 
 make-cmd = $(subst \#,\\\#,$(subst $$,$$$$,$(call escsq,$(cmd_$(1)))))
 
+# Any prerequisites that require target to be built - phony prereq's skipped
+# Any prerequisites that does not exist
+any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
+
 # function to only execute the passed command if necessary
-# >'< substitution is for echo to work, >$< substitution to preserve $ when reloading .cmd file
-# note: when using inline perl scripts [perl -e '...$$t=1;...'] in $(cmd_xxx) double $$ your perl vars
+# >'< substitution is for echo to work,
+# >$< substitution to preserve $ when reloading .cmd file
+# note: when using inline perl scripts [perl -e '...$$t=1;...']
+# in $(cmd_xxx) double $$ your perl vars
 #
-if_changed = $(if $(strip $(filter-out $(PHONY),$?)          \
-		$(call arg-check, $(cmd_$(1)), $(cmd_$@)) ), \
-	@set -e; \
-	$(echo-cmd) $(cmd_$(1)); \
-	echo 'cmd_$@ := $(make-cmd)' > $(@D)/.$(@F).cmd)
+if_changed = $(if $(strip $(any-prereq) $(arg-check)),                       \
+	@set -e;                                                             \
+	$(echo-cmd) $(cmd_$(1));                                             \
+	echo 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd)
 
 # execute the command and also postprocess generated .d dependencies
 # file
-if_changed_dep = $(if $(strip $(filter-out $(PHONY),$?)  \
-		$(filter-out FORCE $(wildcard $^),$^)    \
-	$(call arg-check, $(cmd_$(1)), $(cmd_$@)) ),     \
-	@set -e; \
-	$(echo-cmd) $(cmd_$(1)); \
-	scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(@D)/.$(@F).tmp; \
-	rm -f $(depfile); \
-	mv -f $(@D)/.$(@F).tmp $(@D)/.$(@F).cmd)
+if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ),                  \
+	@set -e;                                                             \
+	$(echo-cmd) $(cmd_$(1));                                             \
+	scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
+	rm -f $(depfile);                                                    \
+	mv -f $(dot-target).tmp $(dot-target).cmd)
 
 # Usage: $(call if_changed_rule,foo)
 # will check if $(cmd_foo) changed, or any of the prequisites changed,
 # and if so will execute $(rule_foo)
-if_changed_rule = $(if $(strip $(filter-out $(PHONY),$?)            \
-			$(call arg-check, $(cmd_$(1)), $(cmd_$@)) ),\
-			@set -e; \
-			$(rule_$(1)))
+if_changed_rule = $(if $(strip $(any-prereq) $(arg-check) ),                 \
+	@set -e;                                                             \
+	$(rule_$(1)))
+
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 3cb445c..e2ad2dc 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -191,9 +191,10 @@ define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
 	$(call echo-cmd,cc_o_c) $(cmd_cc_o_c);				  \
 	$(cmd_modversions)						  \
-	scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,cc_o_c)' > $(@D)/.$(@F).tmp;  \
+	scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,cc_o_c)' >    \
+	                                              $(dot-target).tmp;  \
 	rm -f $(depfile);						  \
-	mv -f $(@D)/.$(@F).tmp $(@D)/.$(@F).cmd
+	mv -f $(dot-target).tmp $(dot-target).cmd
 endef
 
 # Built-in and composite module parts
diff --git a/usr/Makefile b/usr/Makefile
index e938242..2c8f362 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -3,6 +3,8 @@ # kbuild file for usr/ - including initr
 #
 
 klibcdirs:;
+PHONY += klibcdirs
+
 
 # Generate builtin.o based on initramfs_data.o
 obj-y := initramfs_data.o