Re: [Xen-devel] [PATCH 2/2] x86/EFI: use less crude way of generating the build ID

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.08.16 at 18:14, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Aug 15, 2016 at 10:15:47AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote:
>> > >>> On 15.08.16 at 01:42, <konrad@xxxxxxxxxx> wrote:
>> > >> --- a/xen/arch/x86/efi/Makefile
>> > >> +++ b/xen/arch/x86/efi/Makefile
>> > >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
>> > >>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi 
> check.o 2>disabled && echo y))
>> > >>  efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call 
> create,boot.init.o); $(call create,runtime.o)))
>> > >>  
>> > >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
>> > >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
>> > >> +
>> > >> +%.o: %.ihex
>> > >> +       $(OBJCOPY) -I ihex -O binary $< $@
>> > > 
>> > > 
>> > > Under  Ubuntu 14.04.4 this fails compilation:
>> > > 
>> > > 
>> > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'.
>> > 
>> > That's extremely odd, namely considering that the rule is right
>> > there in the quoted text above. Could you double check
>> > xen/arch/x86/efi/buildid.ihex is actually there?
>> 
>> <sigh>
>> No it is not. I had issues with your email (git am -s) so I did it by
>> hand (patch -p2) and of course forgot to include the new file. And later
>> on built it on another OS after pulling from a git tree which missed this 
> file.
>> 
>> Sorry about the false report!
> 
> About the review:
> 
>> 
>> x86/EFI: use less crude way of generating the build ID
>> 
>> Recent enough binutils support --build-id also for COFF/PE output, and
>> hence we should use that in favor of the original hack when possible.
> 
> Could you mention the exact version that gained this?

Looks like that was 2.25; added.

>> This gets complicated by the linker requiring at least one COFF object
>> file to attach the .buildid section to. Hence the patch introduces a
> 
> Is that requirement spelled out in the manpage of the linker?

No. Since it mysteriously failed initially, I had to dig into their
sources to figure out that requirement.

>> buildid.ihex (in order to avoid introducing binary files into the repo)
>> which then gets converted to a binary minimal COFF object (no sections,
>> no symbols).
> 
> Otherwise:
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Thanks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel