Re: [Xen-devel] [PATCH v3 1/2] x86/efi: move the logic to detect PE build support

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Wed, Jul 25, 2018 at 03:38:43PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 25, 2018 at 02:48:28PM +0200, Daniel Kiper wrote:
> > On Wed, Jul 18, 2018 at 12:27:32PM +0200, Roger Pau Monne wrote:
> > > So that it can be used by other components apart from the efi specific
> > > code. By moving the detection code creating a dummy efi/disabled file
> > > can be avoided.
> > >
> > > This is required so that the conditional used to define the efi symbol
> > > in the linker script can be removed and instead the definition of the
> > > efi symbol can be guarded using the preprocessor.
> > >
> > > The motivation behind this change is to be able to build Xen using lld
> > > (the LLVM linker), that at least on version 6.0.0 doesn't work
> > > properly with a DEFINED being used in a conditional expression:
> > >
> > > ld    -melf_x86_64_fbsd  -T xen.lds -N prelink.o --build-id=sha1 \
> > >     /root/src/xen/xen/common/symbols-dummy.o -o 
> > > /root/src/xen/xen/.xen-syms.0
> > > ld: error: xen.lds:233: symbol not found: efi
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > This patch does more than subject says. So, please split this patch into
> > 2 parts. One which really moves the detection logic and the second which
> > replaces DEFINED() with #ifdef.
>
> It's not possible to place the whole contents of the patch in a
> subject line, that's why there's also a commit message that I think
> correctly explains everything done in the patch.

OK, but the subject suggests that the main goal is the PE build support
detection code realignment but AIUI the main goal is DEFINED() drop. So,
this should be in subject. And current commit message is confusing too.
It discuss realignment first and then mentions DEFINED() drop. I think
that it should be done other way around.

> IMO just having a patch to change the DEFINED to an ifdef is quite
> pointless, it's better to switch it here together with the change that
> actually introduces the define itself.

I would do this split. If at some point we have to revert DEFINED()
change then we have to revert whole patch and the following one. If
we do the split we will avoid that problem. And it can be useful for
bissect.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel