[Qemu-devel] [PATCH] fix tool/libuser makefile race

[Qemu-devel] [PATCH] fix tool/libuser makefile race

[Paolo Bonzini]

I just had this race happen on me while building qemu.  The problematic
file in my case was cutils.o.  I'm using GNU make's order-only
dependencies to avoid that "make recurse-all" builds the tools as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index d555bb2..373a861 100644
--- a/Makefile
+++ b/Makefile
@@ -79,7 +79,10 @@ romsubdir-%:
 
 ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS))
 
-recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
+# Some files are shared between the tools and the emulators.  So there
+# can be a race when the main makefile starts building xyz.o, while
+# the recursive make sees a partially built xyz.o and ar(1) fails.
+recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES) | $(TOOLS)
 
 #######################################################################
 # QObject
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> I just had this race happen on me while building qemu.  The problematic
> file in my case was cutils.o.  I'm using GNU make's order-only
> dependencies to avoid that "make recurse-all" builds the tools as well.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  Makefile |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index d555bb2..373a861 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -79,7 +79,10 @@ romsubdir-%:
>  
>  ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS))
>  
> -recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
> +# Some files are shared between the tools and the emulators.  So there
> +# can be a race when the main makefile starts building xyz.o, while
> +# the recursive make sees a partially built xyz.o and ar(1) fails.
> +recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES) | $(TOOLS)
>  
>  #######################################################################
>  # QObject

I still don't understand why that is needed :( (make mysteries).

All shared stuff between $(TOOLS) and $(SUBDIR_RULES) are made from the
same Makefile invocation.  I don't see where the recursive make can
build a file that is also built by the normal Makefile.

And if we have one file that is build in both places, that is the bug
that we have to fix.

Notice that cutils.o (from Makefile) is built in the root dir, and
cutils.o (from Makefile.user) is built into libuser/ directory.

I am confused how one can interfer with the other.

VPATH vs vpath problems?

Later, Juan.

[Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Paolo Bonzini]

> Notice that cutils.o (from Makefile) is built in the root dir, and
> cutils.o (from Makefile.user) is built into libuser/ directory.

Ah, since that was the file that was built, that would be a problem
with my reasoning.

Definitely I saw the following in the log

    AR     libuser.a
    CC     cutils.o
    error: invalid cutils.o

or something like that.

> VPATH vs vpath problems?

Definitely a possibility.

Paolo

Re: [Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Andreas Färber]

Hey,

Am 18.12.2009 um 17:16 schrieb Juan Quintela:

> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> I just had this race happen on me while building qemu.  The  
>> problematic
>> file in my case was cutils.o.  I'm using GNU make's order-only
>> dependencies to avoid that "make recurse-all" builds the tools as  
>> well.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> Makefile |    5 ++++-
>> 1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index d555bb2..373a861 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -79,7 +79,10 @@ romsubdir-%:
>>
>> ALL_SUBDIRS=$(TARGET_DIRS) $(patsubst %,pc-bios/%, $(ROMS))
>>
>> -recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES)
>> +# Some files are shared between the tools and the emulators.  So  
>> there
>> +# can be a race when the main makefile starts building xyz.o, while
>> +# the recursive make sees a partially built xyz.o and ar(1) fails.
>> +recurse-all: $(SUBDIR_RULES) $(ROMSUBDIR_RULES) | $(TOOLS)
>>
>> #######################################################################
>> # QObject
>
> I still don't understand why that is needed :( (make mysteries).
>
> All shared stuff between $(TOOLS) and $(SUBDIR_RULES) are made from  
> the
> same Makefile invocation.  I don't see where the recursive make can
> build a file that is also built by the normal Makefile.

Think of my porposed Makefile patch!

> And if we have one file that is build in both places, that is the bug
> that we have to fix.
>
> Notice that cutils.o (from Makefile) is built in the root dir, and
> cutils.o (from Makefile.user) is built into libuser/ directory.
>
> I am confused how one can interfer with the other.
>
> VPATH vs vpath problems?

libuser builds some .o files from .c files in $(SRC_PATH). They end up  
in libuser. For those that were built already in .., the .o files are  
taken from there instead.

Andreas

>
> Later, Juan.
>
>

Re: [Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Paolo Bonzini]

On 12/18/2009 07:59 PM, Andreas Färber wrote:
>
> Think of my porposed Makefile patch!

Which one?

Paolo

Re: [Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Andreas Färber]

Am 20.12.2009 um 19:22 schrieb Paolo Bonzini:

> On 12/18/2009 07:59 PM, Andreas Färber wrote:
>>
>> Think of my porposed Makefile patch!
>
> Which one?

In particular my add-on patch for libuser.a, which tackled exactly  
that issue:

http://www.mail-archive.com/qemu-devel@nongnu.org/msg20948.html

Andreas

Re: [Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Paolo Bonzini]

On 12/20/2009 07:30 PM, Andreas Färber wrote:
>
> Am 20.12.2009 um 19:22 schrieb Paolo Bonzini:
>
>> On 12/18/2009 07:59 PM, Andreas Färber wrote:
>>>
>>> Think of my porposed Makefile patch!
>>
>> Which one?
>
> In particular my add-on patch for libuser.a, which tackled exactly that
> issue:
>
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg20948.html

Ah, thanks.  Just to be clear, this issue was on Linux, not on Solaris. 
  It is completely unrelated to --whole-archive and is a real bug.

Paolo

Re: [Qemu-devel] Re: [PATCH] fix tool/libuser makefile race

[Andreas Färber]

Am 20.12.2009 um 19:38 schrieb Paolo Bonzini:

> On 12/20/2009 07:30 PM, Andreas Färber wrote:
>>
>> Am 20.12.2009 um 19:22 schrieb Paolo Bonzini:
>>
>>> On 12/18/2009 07:59 PM, Andreas Färber wrote:
>>>>
>>>> Think of my porposed Makefile patch!
>>>
>>> Which one?
>>
>> In particular my add-on patch for libuser.a, which tackled exactly  
>> that
>> issue:
>>
>> http://www.mail-archive.com/qemu-devel@nongnu.org/msg20948.html
>
> Ah, thanks.  Just to be clear, this issue was on Linux, not on  
> Solaris.  It is completely unrelated to --whole-archive and is a  
> real bug.

I did understand that. My patch was for Linux (there being no solaris- 
user) and acknowledged that there is such an unexpected mixture of  
libuser.a and libqemu_common.a object files, so it wasn't an all new  
discovery now.
I didn't attempt to change that behavior though, just Made It Work(tm)  
for regular builds.

Since we're bound to GNU make anyway, I have no objections to your  
patch if it works.

Andreas

[Qemu-devel] [PATCH] Use vpath directive

[Paolo Bonzini]

The vpath directive has two advantages over the VPATH variable:
1) it allows to skip searching of .o files; 2) the default semantics
are to append to the vpath, so there is no confusion between "VPATH=xyz"
and "VPATH+=xyz".

Since "vpath %.c %.h PATH" is not valid, I'm introducing a wrapper
macro to append one or more directories to the vpath.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	This one also fixes the same race (by building cutils.o twice).
        However, it is nicer in that it fixes the wrong occurrence of
        "vpath %.c %.h PATH" (sorry for missing that on my review of
        Kirill's patch).

 Makefile                   |    2 +-
 Makefile.hw                |    2 +-
 Makefile.target            |   11 +++++++----
 Makefile.user              |    6 +-----
 pc-bios/optionrom/Makefile |    3 ++-
 rules.mak                  |    2 ++
 tests/Makefile             |    3 ++-
 7 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index ec52ee2..645ec82 100644
--- a/Makefile
+++ b/Makefile
@@ -25,7 +25,7 @@ configure: ;
 .PHONY: all clean cscope distclean dvi html info install install-doc \
 	recurse-all speed tar tarbin test build-all
 
-VPATH=$(SRC_PATH):$(SRC_PATH)/hw
+$(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
 
 LIBS+=-lz $(LIBS_TOOLS)
 
diff --git a/Makefile.hw b/Makefile.hw
index bd252f5..781c006 100644
--- a/Makefile.hw
+++ b/Makefile.hw
@@ -7,7 +7,7 @@ include $(SRC_PATH)/rules.mak
 
 .PHONY: all
 
-VPATH=$(SRC_PATH):$(SRC_PATH)/hw
+$(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
 
 QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu
 
diff --git a/Makefile.target b/Makefile.target
index 0504d3b..92c2027 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -9,7 +9,7 @@ include config-target.mak
 include $(SRC_PATH)/rules.mak
 
 TARGET_PATH=$(SRC_PATH)/target-$(TARGET_BASE_ARCH)
-VPATH=$(SRC_PATH):$(TARGET_PATH):$(SRC_PATH)/hw
+$(call set-vpath, $(SRC_PATH):$(TARGET_PATH):$(SRC_PATH)/hw)
 QEMU_CFLAGS+= -I.. -I$(TARGET_PATH) -DNEED_CPU_H
 
 ifdef CONFIG_USER_ONLY
@@ -87,7 +87,8 @@ signal.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
 
 ifdef CONFIG_LINUX_USER
 
-VPATH+=:$(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
+$(call set-vpath, $(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR))
+
 QEMU_CFLAGS+=-I$(SRC_PATH)/linux-user -I$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
 obj-y = main.o syscall.o strace.o mmap.o signal.o thunk.o \
       elfload.o linuxload.o uaccess.o gdbstub.o
@@ -115,7 +116,8 @@ endif #CONFIG_LINUX_USER
 
 ifdef CONFIG_DARWIN_USER
 
-VPATH+=:$(SRC_PATH)/darwin-user
+$(call set-vpath, $(SRC_PATH)/darwin-user)
+
 QEMU_CFLAGS+=-I$(SRC_PATH)/darwin-user -I$(SRC_PATH)/darwin-user/$(TARGET_ARCH)
 
 # Leave some space for the regular program loading zone
@@ -137,7 +139,8 @@ endif #CONFIG_DARWIN_USER
 
 ifdef CONFIG_BSD_USER
 
-VPATH+=:$(SRC_PATH)/bsd-user
+$(call set-vpath, $(SRC_PATH)/bsd-user)
+
 QEMU_CFLAGS+=-I$(SRC_PATH)/bsd-user -I$(SRC_PATH)/bsd-user/$(TARGET_ARCH)
 
 obj-y = main.o bsdload.o elfload.o mmap.o signal.o strace.o syscall.o \
diff --git a/Makefile.user b/Makefile.user
index 7daedef..5df114f 100644
--- a/Makefile.user
+++ b/Makefile.user
@@ -6,11 +6,7 @@ include $(SRC_PATH)/rules.mak
 
 .PHONY: all
 
-# Do not take %.o from $(SRC_PATH), only %.c and %.h
-# All %.o for user targets should be built with -fpie, when
-# configured with --enable-user-pie, so we don't want to
-# take %.o from $(SRC_PATH), since they built without -fpie
-vpath %.c %.h $(SRC_PATH)
+$(call set-vpath, $(SRC_PATH))
 
 QEMU_CFLAGS+=-I..
 
diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
index 54db882..b4be31e 100644
--- a/pc-bios/optionrom/Makefile
+++ b/pc-bios/optionrom/Makefile
@@ -5,7 +5,8 @@ all: build-all
 include ../../config-host.mak
 include $(SRC_PATH)/rules.mak
 
-VPATH=$(SRC_PATH)/pc-bios/optionrom
+$(call set-vpath, $(SRC_PATH)/pc-bios/optionrom)
+
 .PHONY : all clean build-all
 
 CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin
diff --git a/rules.mak b/rules.mak
index 5d9f684..9cd67f0 100644
--- a/rules.mak
+++ b/rules.mak
@@ -39,6 +39,8 @@ quiet-command = $(if $(V),$1,$(if $(2),@echo $2 && $1, @$1))
 cc-option = $(if $(shell $(CC) $1 $2 -S -o /dev/null -xc /dev/null \
               >/dev/null 2>&1 && echo OK), $2, $3)
 
+set-vpath = $(if $1,$(foreach PATTERN,%.c %.h %.S, $(eval vpath $(PATTERN) $1)))
+
 # Generate timestamp files for .h include files
 
 %.h: %.h-timestamp
diff --git a/tests/Makefile b/tests/Makefile
index 69092e5..ff7f787 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -1,5 +1,6 @@
 -include ../config-host.mak
-VPATH=$(SRC_PATH)/tests
+
+$(call set-vpath, $(SRC_PATH)/tests)
 
 CFLAGS=-Wall -O2 -g -fno-strict-aliasing
 #CFLAGS+=-msse2
-- 
1.6.5.2

Re: [Qemu-devel] [PATCH] Use vpath directive

[Anthony Liguori]

On 12/21/2009 03:06 AM, Paolo Bonzini wrote:
> The vpath directive has two advantages over the VPATH variable:
> 1) it allows to skip searching of .o files; 2) the default semantics
> are to append to the vpath, so there is no confusion between "VPATH=xyz"
> and "VPATH+=xyz".
>
> Since "vpath %.c %.h PATH" is not valid, I'm introducing a wrapper
> macro to append one or more directories to the vpath.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>    


Applied.  Thanks.

Regards,

Anthony Liguori


> ---
> 	This one also fixes the same race (by building cutils.o twice).
>          However, it is nicer in that it fixes the wrong occurrence of
>          "vpath %.c %.h PATH" (sorry for missing that on my review of
>          Kirill's patch).
>
>   Makefile                   |    2 +-
>   Makefile.hw                |    2 +-
>   Makefile.target            |   11 +++++++----
>   Makefile.user              |    6 +-----
>   pc-bios/optionrom/Makefile |    3 ++-
>   rules.mak                  |    2 ++
>   tests/Makefile             |    3 ++-
>   7 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index ec52ee2..645ec82 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -25,7 +25,7 @@ configure: ;
>   .PHONY: all clean cscope distclean dvi html info install install-doc \
>   	recurse-all speed tar tarbin test build-all
>
> -VPATH=$(SRC_PATH):$(SRC_PATH)/hw
> +$(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
>
>   LIBS+=-lz $(LIBS_TOOLS)
>
> diff --git a/Makefile.hw b/Makefile.hw
> index bd252f5..781c006 100644
> --- a/Makefile.hw
> +++ b/Makefile.hw
> @@ -7,7 +7,7 @@ include $(SRC_PATH)/rules.mak
>
>   .PHONY: all
>
> -VPATH=$(SRC_PATH):$(SRC_PATH)/hw
> +$(call set-vpath, $(SRC_PATH):$(SRC_PATH)/hw)
>
>   QEMU_CFLAGS+=-I.. -I$(SRC_PATH)/fpu
>
> diff --git a/Makefile.target b/Makefile.target
> index 0504d3b..92c2027 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -9,7 +9,7 @@ include config-target.mak
>   include $(SRC_PATH)/rules.mak
>
>   TARGET_PATH=$(SRC_PATH)/target-$(TARGET_BASE_ARCH)
> -VPATH=$(SRC_PATH):$(TARGET_PATH):$(SRC_PATH)/hw
> +$(call set-vpath, $(SRC_PATH):$(TARGET_PATH):$(SRC_PATH)/hw)
>   QEMU_CFLAGS+= -I.. -I$(TARGET_PATH) -DNEED_CPU_H
>
>   ifdef CONFIG_USER_ONLY
> @@ -87,7 +87,8 @@ signal.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
>
>   ifdef CONFIG_LINUX_USER
>
> -VPATH+=:$(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
> +$(call set-vpath, $(SRC_PATH)/linux-user:$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR))
> +
>   QEMU_CFLAGS+=-I$(SRC_PATH)/linux-user -I$(SRC_PATH)/linux-user/$(TARGET_ABI_DIR)
>   obj-y = main.o syscall.o strace.o mmap.o signal.o thunk.o \
>         elfload.o linuxload.o uaccess.o gdbstub.o
> @@ -115,7 +116,8 @@ endif #CONFIG_LINUX_USER
>
>   ifdef CONFIG_DARWIN_USER
>
> -VPATH+=:$(SRC_PATH)/darwin-user
> +$(call set-vpath, $(SRC_PATH)/darwin-user)
> +
>   QEMU_CFLAGS+=-I$(SRC_PATH)/darwin-user -I$(SRC_PATH)/darwin-user/$(TARGET_ARCH)
>
>   # Leave some space for the regular program loading zone
> @@ -137,7 +139,8 @@ endif #CONFIG_DARWIN_USER
>
>   ifdef CONFIG_BSD_USER
>
> -VPATH+=:$(SRC_PATH)/bsd-user
> +$(call set-vpath, $(SRC_PATH)/bsd-user)
> +
>   QEMU_CFLAGS+=-I$(SRC_PATH)/bsd-user -I$(SRC_PATH)/bsd-user/$(TARGET_ARCH)
>
>   obj-y = main.o bsdload.o elfload.o mmap.o signal.o strace.o syscall.o \
> diff --git a/Makefile.user b/Makefile.user
> index 7daedef..5df114f 100644
> --- a/Makefile.user
> +++ b/Makefile.user
> @@ -6,11 +6,7 @@ include $(SRC_PATH)/rules.mak
>
>   .PHONY: all
>
> -# Do not take %.o from $(SRC_PATH), only %.c and %.h
> -# All %.o for user targets should be built with -fpie, when
> -# configured with --enable-user-pie, so we don't want to
> -# take %.o from $(SRC_PATH), since they built without -fpie
> -vpath %.c %.h $(SRC_PATH)
> +$(call set-vpath, $(SRC_PATH))
>
>   QEMU_CFLAGS+=-I..
>
> diff --git a/pc-bios/optionrom/Makefile b/pc-bios/optionrom/Makefile
> index 54db882..b4be31e 100644
> --- a/pc-bios/optionrom/Makefile
> +++ b/pc-bios/optionrom/Makefile
> @@ -5,7 +5,8 @@ all: build-all
>   include ../../config-host.mak
>   include $(SRC_PATH)/rules.mak
>
> -VPATH=$(SRC_PATH)/pc-bios/optionrom
> +$(call set-vpath, $(SRC_PATH)/pc-bios/optionrom)
> +
>   .PHONY : all clean build-all
>
>   CFLAGS := -Wall -Wstrict-prototypes -Werror -fomit-frame-pointer -fno-builtin
> diff --git a/rules.mak b/rules.mak
> index 5d9f684..9cd67f0 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -39,6 +39,8 @@ quiet-command = $(if $(V),$1,$(if $(2),@echo $2&&  $1, @$1))
>   cc-option = $(if $(shell $(CC) $1 $2 -S -o /dev/null -xc /dev/null \
>                 >/dev/null 2>&1&&  echo OK), $2, $3)
>
> +set-vpath = $(if $1,$(foreach PATTERN,%.c %.h %.S, $(eval vpath $(PATTERN) $1)))
> +
>   # Generate timestamp files for .h include files
>
>   %.h: %.h-timestamp
> diff --git a/tests/Makefile b/tests/Makefile
> index 69092e5..ff7f787 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -1,5 +1,6 @@
>   -include ../config-host.mak
> -VPATH=$(SRC_PATH)/tests
> +
> +$(call set-vpath, $(SRC_PATH)/tests)
>
>   CFLAGS=-Wall -O2 -g -fno-strict-aliasing
>   #CFLAGS+=-msse2
>