[XEN,for-4.13] xen: Fix race to build arch/x86/efi/relocs-dummy.o
diff mbox series

Message ID 20191114180542.1016867-1-anthony.perard@citrix.com
State New
Headers show
Series
  • [XEN,for-4.13] xen: Fix race to build arch/x86/efi/relocs-dummy.o
Related show

Commit Message

Anthony PERARD Nov. 14, 2019, 6:05 p.m. UTC
With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
will attempt to build that object. This result in the dependency file
been generated with relocs-dummy.o depending on efi/relocs-dummy.o.

Then, when arch/x86/efi/Makefile tries to build relocs-dummy.o, well
efi/relocs-dummy.S doesn't exist.

Have only one makefile responsible for building relocs-dummy.o.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 xen/arch/x86/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich Nov. 15, 2019, 10:06 a.m. UTC | #1
On 14.11.2019 19:05, Anthony PERARD wrote:
> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
> will attempt to build that object. This result in the dependency file
> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.

I cannot confirm this, what I see is

efi/relocs-dummy.o: efi/relocs-dummy.S \
 ...

Which isn't to say there's no issue here. IOW the actual adjustment
is ...

> Then, when arch/x86/efi/Makefile tries to build relocs-dummy.o, well
> efi/relocs-dummy.S doesn't exist.
> 
> Have only one makefile responsible for building relocs-dummy.o.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

But I'd prefer if the description aspect could either be clarified
(different versions of make behaving differently?) or adjusted.

Jan
Anthony PERARD Nov. 15, 2019, 12:12 p.m. UTC | #2
On Fri, Nov 15, 2019 at 11:06:27AM +0100, Jan Beulich wrote:
> On 14.11.2019 19:05, Anthony PERARD wrote:
> > With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
> > will attempt to build that object. This result in the dependency file
> > been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
> 
> I cannot confirm this, what I see is
> 
> efi/relocs-dummy.o: efi/relocs-dummy.S \
>  ...


I've written the commit message base on few evidences, but I don't know
if the race comes from trying to build $(TARGET).efi. Here is what I
have:

# Building Xen with make -j8 after git clean
make[3]: *** No rule to make target 'efi/relocs-dummy.S', needed by 'relocs-dummy.o'.
$ head -1 arch/x86/efi/.relocs-dummy.o.d
relocs-dummy.o: efi/relocs-dummy.S \

arch/x86/.relocs-dummy.o.d doesn't exist.

looking back at the make output, relocs-dummy was built with:
gcc -D__ASSEMBLY__ -m64 -DBUILD_ID -fno-strict-aliasing -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ -include /local/home/sheep/work/xen/xen/include/xen/config.h '-D__OBJECT_FILE__="efi/relocs-dummy.o"' -Wa,--strip-local-absolute -g -MMD -MF efi/.relocs-dummy.o.d -DXEN_BUILD_EFI -I/local/home/sheep/work/xen/xen/include -I/local/home/sheep/work/xen/xen/include/asm-x86/mach-generic -I/local/home/sheep/work/xen/xen/include/asm-x86/mach-default -DXEN_IMG_OFFSET=0x200000 '-D__OBJECT_LABEL__=arch$x86$efi$relocs_dummy.o' -msoft-float -fno-stack-protector -fno-exceptions -Wnested-externs -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND -DHAVE_AS_FSGSBASE -DHAVE_AS_XSAVEOPT -DHAVE_AS_RDSEED -DHAVE_AS_CLWB -U__OBJECT_LABEL__ -DHAVE_AS_QUOTED_SYM '-D__OBJECT_LABEL__=arch/x86/efi/relocs-dummy.o' -DHAVE_AS_INVPCID -DHAVE_AS_NEGATIVE_TRUE -DHAVE_AS_NOPS_DIRECTIVE -mno-red-zone -fpic -fno-asynchronous-unwind-tables -mno-sse -mskip-rax-setup -DGCC_HAS_VISIBILITY_ATTRIBUTE -mindirect-branch=thunk-extern -mindirect-branch-register -DCONFIG_INDIRECT_THUNK -fno-jump-tables -mpreferred-stack-boundary=3 -Wa,-I/local/home/sheep/work/xen/xen/include -DBUILD_ID_EFI -c efi/relocs-dummy.S -o efi/relocs-dummy.o

$ gcc --version
gcc (GCC) 9.2.0

I'm guessing that gcc behave differently between both our system?
On mine, `man gcc` seems to imply there's nothing wrong on my system.
If we want gcc to produce "efi/reloc-dummy.o: .." on my system, I think
we would need to add the -MT or -MQ option to the cflags.

Cheers,
Jan Beulich Nov. 15, 2019, 12:23 p.m. UTC | #3
On 15.11.2019 13:12, Anthony PERARD wrote:
> On Fri, Nov 15, 2019 at 11:06:27AM +0100, Jan Beulich wrote:
>> On 14.11.2019 19:05, Anthony PERARD wrote:
>>> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
>>> will attempt to build that object. This result in the dependency file
>>> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
>>
>> I cannot confirm this, what I see is
>>
>> efi/relocs-dummy.o: efi/relocs-dummy.S \
>>  ...
> 
> 
> I've written the commit message base on few evidences, but I don't know
> if the race comes from trying to build $(TARGET).efi. Here is what I
> have:
> 
> # Building Xen with make -j8 after git clean
> make[3]: *** No rule to make target 'efi/relocs-dummy.S', needed by 'relocs-dummy.o'.
> $ head -1 arch/x86/efi/.relocs-dummy.o.d
> relocs-dummy.o: efi/relocs-dummy.S \
> 
> arch/x86/.relocs-dummy.o.d doesn't exist.

So I guess I'd like to include "may" then in that specific sentence of
the commit message, which would be easy enough to do while committing.

> looking back at the make output, relocs-dummy was built with:
> gcc -D__ASSEMBLY__ -m64 -DBUILD_ID -fno-strict-aliasing -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla -pipe -D__XEN__ -include /local/home/sheep/work/xen/xen/include/xen/config.h '-D__OBJECT_FILE__="efi/relocs-dummy.o"' -Wa,--strip-local-absolute -g -MMD -MF efi/.relocs-dummy.o.d -DXEN_BUILD_EFI -I/local/home/sheep/work/xen/xen/include -I/local/home/sheep/work/xen/xen/include/asm-x86/mach-generic -I/local/home/sheep/work/xen/xen/include/asm-x86/mach-default -DXEN_IMG_OFFSET=0x200000 '-D__OBJECT_LABEL__=arch$x86$efi$relocs_dummy.o' -msoft-float -fno-stack-protector -fno-exceptions -Wnested-externs -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND -DHAVE_AS_FSGSBASE -DHAVE_AS_XSAVEOPT -DHAVE_AS_RDSEED -DHAVE_AS_CLWB -U__OBJECT_LABEL__ -DHAVE_AS_QUOTED_SYM '-D__OBJECT_LABEL__=arch/x86/efi/relocs-dummy.o' -DHAVE_AS_INVPCID -DHAVE_AS_NEGATIVE_TRUE -DHAVE_AS_NOPS_DIRECTIVE -mno-red-zone -fpic -fno-asynchronous-unwind-tables -mno-sse -mskip-rax-setup -DGCC_HAS_VISIBILITY_ATTRIBUTE -mindirect-branch=thunk-extern -mindirect-branch-register -DCONFIG_INDIRECT_THUNK -fno-jump-tables -mpreferred-stack-boundary=3 -Wa,-I/local/home/sheep/work/xen/xen/include -DBUILD_ID_EFI -c efi/relocs-dummy.S -o efi/relocs-dummy.o
> 
> $ gcc --version
> gcc (GCC) 9.2.0
> 
> I'm guessing that gcc behave differently between both our system?

Quite possible, albeit it's 9.2.0 here too. Different set of patches
on top of the upstream version, I guess.

Jan
Jürgen Groß Nov. 15, 2019, 12:26 p.m. UTC | #4
On 14.11.19 19:05, Anthony PERARD wrote:
> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
> will attempt to build that object. This result in the dependency file
> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
> 
> Then, when arch/x86/efi/Makefile tries to build relocs-dummy.o, well
> efi/relocs-dummy.S doesn't exist.
> 
> Have only one makefile responsible for building relocs-dummy.o.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen
Anthony PERARD Nov. 15, 2019, 12:40 p.m. UTC | #5
On Fri, Nov 15, 2019 at 01:23:46PM +0100, Jan Beulich wrote:
> On 15.11.2019 13:12, Anthony PERARD wrote:
> > On Fri, Nov 15, 2019 at 11:06:27AM +0100, Jan Beulich wrote:
> >> On 14.11.2019 19:05, Anthony PERARD wrote:
> >>> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
> >>> will attempt to build that object. This result in the dependency file
> >>> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
[..]
> So I guess I'd like to include "may" then in that specific sentence of
> the commit message, which would be easy enough to do while committing.

Ok, so that first paragraph can be rewritten:

With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
will attempt to build that object. This result in the dependency file
been generated that may have relocs-dummy.o depending on
efi/relocs-dummy.o.

Thanks,
Jan Beulich Nov. 19, 2019, 4:27 p.m. UTC | #6
On 14.11.2019 19:05, Anthony PERARD wrote:
> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
> will attempt to build that object. This result in the dependency file
> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
> 
> Then, when arch/x86/efi/Makefile tries to build relocs-dummy.o, well
> efi/relocs-dummy.S doesn't exist.
> 
> Have only one makefile responsible for building relocs-dummy.o.

On a system with too old a tool chain for the EFI build to get
enabled I now get about a dozen instances per build of

nm: 'efi/relocs-dummy.o': No such file

I don't suppose you did try out your change in such an oldish
environment? I assume the problem are these two lines:

$(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
$(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')

I'm not sure it is well defined when make would evaluate such
target specific variable assignments (i.e. I'm not sure this
doesn't point out an issue even on EFI capable tool chains).
Then again these not using := should cause them to get
evaluated only upon use, i.e. never. But that's clearly not
the case here; of course make is also the now pretty dated
3.81 one.
Jan Beulich Nov. 19, 2019, 4:32 p.m. UTC | #7
On 19.11.2019 17:27, Jan Beulich wrote:
> On 14.11.2019 19:05, Anthony PERARD wrote:
>> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
>> will attempt to build that object. This result in the dependency file
>> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
>>
>> Then, when arch/x86/efi/Makefile tries to build relocs-dummy.o, well
>> efi/relocs-dummy.S doesn't exist.
>>
>> Have only one makefile responsible for building relocs-dummy.o.
> 
> On a system with too old a tool chain for the EFI build to get
> enabled I now get about a dozen instances per build of
> 
> nm: 'efi/relocs-dummy.o': No such file
> 
> I don't suppose you did try out your change in such an oldish
> environment? I assume the problem are these two lines:
> 
> $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
> $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
> 
> I'm not sure it is well defined when make would evaluate such
> target specific variable assignments (i.e. I'm not sure this
> doesn't point out an issue even on EFI capable tool chains).
> Then again these not using := should cause them to get
> evaluated only upon use, i.e. never.

Ah, this was wrong - the $(guard) prefix causes them to get
evaluated even when xen.efi cannot be built. So I guess this is
just a cosmetic issue then, which would however still be nice
to see addressed.

Jan
Anthony PERARD Nov. 19, 2019, 4:58 p.m. UTC | #8
On Tue, Nov 19, 2019 at 05:32:53PM +0100, Jan Beulich wrote:
> On 19.11.2019 17:27, Jan Beulich wrote:
> > On 14.11.2019 19:05, Anthony PERARD wrote:
> >> With $(TARGET).efi depending on efi/relocs-dummy.o, arch/x86/Makefile
> >> will attempt to build that object. This result in the dependency file
> >> been generated with relocs-dummy.o depending on efi/relocs-dummy.o.
> >>
> >> Then, when arch/x86/efi/Makefile tries to build relocs-dummy.o, well
> >> efi/relocs-dummy.S doesn't exist.
> >>
> >> Have only one makefile responsible for building relocs-dummy.o.
> > 
> > On a system with too old a tool chain for the EFI build to get
> > enabled I now get about a dozen instances per build of
> > 
> > nm: 'efi/relocs-dummy.o': No such file
> > 
> > I don't suppose you did try out your change in such an oldish
> > environment? I assume the problem are these two lines:
> > 
> > $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p')
> > $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
> > 
> > I'm not sure it is well defined when make would evaluate such
> > target specific variable assignments (i.e. I'm not sure this
> > doesn't point out an issue even on EFI capable tool chains).
> > Then again these not using := should cause them to get
> > evaluated only upon use, i.e. never.
> 
> Ah, this was wrong - the $(guard) prefix causes them to get
> evaluated even when xen.efi cannot be built. So I guess this is
> just a cosmetic issue then, which would however still be nice
> to see addressed.

That $(guard) thing is weird, and can probably be replace now.

I'll try to remove that thing, and also avoid having $(TARGET).efi
depending on efi/relocs-dummy.o when it isn't going to be built (when
XEN_BUILD_EFI=n).

Patch
diff mbox series

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5e6b9d7028db..a6df19e901b3 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -219,8 +219,8 @@  $(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o efi/mkreloc
 		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort >$(@D)/$(@F).map; fi
 	rm -f $(@D)/.$(@F).[0-9]* $(@D)/..$(@F).[0-9]*
 
-efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
-efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
+efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o efi/relocs-dummy.o: $(BASEDIR)/arch/x86/efi/built_in.o
+efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o efi/relocs-dummy.o: ;
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(BASEDIR)/include/asm-x86/asm-macros.h
 	$(CC) $(filter-out -Wa$(comma)% -flto,$(CFLAGS)) -S -o $@ $<