[Buildroot] [PATCH master 0/2] uboot: better fix for host-dtc influence

[Buildroot] [PATCH master 0/2] uboot: better fix for host-dtc influence

[Thomas De Schampheleire]

While testing 2018.02-rc1 with a vendor-provided old U-boot tree, I came across
build errors in uboot. The attempted fix currently in Buildroot is not
sufficient.

In this series I provide an alternative fix, tested on the original failure
scenario as well, and then backout the previous fix.

Best regards,
Thomas


Thomas De Schampheleire (2):
  uboot: use local fdt headers
  Revert "uboot: use local libfdt.h"

 boot/uboot/uboot.mk | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

-- 
2.13.6

[Buildroot] [PATCH master 1/2] uboot: use local fdt headers

[Thomas De Schampheleire]

After commit b8c3e941731d ("package/dtc: qemu system build need libfdt")
changed the dtc install target from 'install-bin' to 'install', uboot
compilation failures occurred because libfdt related headers were now
suddenly taken from output/host/include rather than from the uboot sources
itself.

Commit 3a6573ccee26 ("uboot: use local libfdt.h") solved this by patching
one specific uboot source file, tools/fdtgrep.c, to replace '<...>'-style
includes by '"..."'-style includes.

However, depending on the uboot version, this may not be enough: there may
be other references to fdt header files. In particular taking into account
that it is not uncommon to have vendor-provided uboot trees which have
custom changes.

The root of the problem is that the uboot.mk file passes the host compiler
as follows:
	UBOOT_MAKE_OPTS += \
		...
		HOSTCC="$(HOSTCC) $(HOST_CFLAGS)" \
		...
where HOST_CFLAGS contains the string '-I$(HOST_DIR)/include'
The uboot makefiles then use constructs of the form:
	$(CC) $(CPPFLAGS) $(CFLAGS) .....
where CPPFLAGS may contain -I references pointing to local directories.

On the expanded compiler command-line, Buildroot's '-I$(HOST_DIR)/include'
is thus present _before_ any -I to local directories, and thus takes
precedence.  And that becomes a problem for header files present both
locally as in the Buildroot host directory, which is the case for libfdt.

To fix this problem without having to patch u-boot sources, use '-idirafter'
rather than '-I' to pass the Buildroot host include directory. '-idirafter'
is basically the same thing, but adds the specified directory at the end
of the include precedence chain, rather than at the beginning.

---

Note: it is to be discussed whether we want to change $(HOST_CFLAGS)
globally to use -idirafter rather than -I. It may be risky.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 boot/uboot/uboot.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index d2f241cd8b..977f44cad8 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -131,7 +131,7 @@ endif
 UBOOT_MAKE_OPTS += \
 	CROSS_COMPILE="$(TARGET_CROSS)" \
 	ARCH=$(UBOOT_ARCH) \
-	HOSTCC="$(HOSTCC) $(HOST_CFLAGS)" \
+	HOSTCC="$(HOSTCC) $(subst -I/,-idirafter /,$(subst -I /,-idirafter /,$(HOST_CFLAGS)))" \
 	HOSTLDFLAGS="$(HOST_LDFLAGS)"
 
 ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y)
-- 
2.13.6

[Buildroot] [PATCH master 2/2] Revert "uboot: use local libfdt.h"

[Thomas De Schampheleire]

This reverts commit 3a6573ccee2624de0c604abf2c7df6704a4cf566.

It is no longer necessary after solving the problem differently.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 boot/uboot/uboot.mk | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 977f44cad8..c7cd739150 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -188,13 +188,6 @@ define UBOOT_APPLY_LOCAL_PATCHES
 endef
 UBOOT_POST_PATCH_HOOKS += UBOOT_APPLY_LOCAL_PATCHES
 
-# Bug: https://patchwork.ozlabs.org/patch/833760/
-define UBOOT_FIX_LIBFDT_SYSTEM_PATH
-	[ ! -e $(@D)/tools/fdtgrep.c ] || \
-	$(SED) 's%<../include/libfdt.h>%"../include/libfdt.h"%' $(@D)/tools/fdtgrep.c
-endef
-UBOOT_POST_PATCH_HOOKS += UBOOT_FIX_LIBFDT_SYSTEM_PATH
-
 ifeq ($(BR2_TARGET_UBOOT_BUILD_SYSTEM_LEGACY),y)
 define UBOOT_CONFIGURE_CMDS
 	$(TARGET_CONFIGURE_OPTS) 	\
-- 
2.13.6

[Buildroot] [PATCH master 0/2] uboot: better fix for host-dtc influence

[Thomas Petazzoni]

Hello,

On Mon, 19 Feb 2018 16:56:30 +0100, Thomas De Schampheleire wrote:

> Thomas De Schampheleire (2):
>   uboot: use local fdt headers
>   Revert "uboot: use local libfdt.h"

Applied to master. Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[Buildroot] [master,1/2] uboot: use local fdt headers

[Trent Piepho]

On Mon, 2018-02-19 at 16:56 +0100, Thomas De Schampheleire wrote:
> 
> On the expanded compiler command-line, Buildroot's '-I$(HOST_DIR)/include'
> is thus present _before_ any -I to local directories, and thus takes
> precedence.  And that becomes a problem for header files present both
> locally as in the Buildroot host directory, which is the case for libfdt.
> 
> To fix this problem without having to patch u-boot sources, use '-idirafter'
> rather than '-I' to pass the Buildroot host include directory. '-idirafter'
> is basically the same thing, but adds the specified directory at the end
> of the include precedence chain, rather than at the beginning.

This is breaking the build of uboot host tools for me.

My issue is that uboot's build of the host object rsa-sign.o is using
the system installed openssl header files.  One can verify this by
looking at the file build/uboot-<hash>/tools/lib/rsa/.rsa-sign.o.cmd,
which will have the auto-dependency info, and thus shows exactly what
header files were used to compile the object.

buildroot is also compiling host openssl, with headers and libraries in
the buildroot host output tree.  When uboot tries to link the host
programs, it uses the buildroot built openssl library (like it should),
but this doesn't match the headers.

It see the include dir order should be:
u-boot
buildroot host
host os

But this patch gives:
u-boot
host os
buildroot host

Like all "host system's headers" problems, if your host and buildroot
have nearly the same openssl version, or you have no openssl devel on
your host, then you wouldn't see this.

I think the real solution here is to get u-boot's build system to
support building with an external libfdt and do that.

[Buildroot] [master,1/2] uboot: use local fdt headers

[Peter Korsgaard]

>>>>> "Trent" == Trent Piepho <tpiepho@impinj.com> writes:

Hi,

 > On Mon, 2018-02-19 at 16:56 +0100, Thomas De Schampheleire wrote:
 >> 
 >> On the expanded compiler command-line, Buildroot's '-I$(HOST_DIR)/include'
 >> is thus present _before_ any -I to local directories, and thus takes
 >> precedence.  And that becomes a problem for header files present both
 >> locally as in the Buildroot host directory, which is the case for libfdt.
 >> 
 >> To fix this problem without having to patch u-boot sources, use '-idirafter'
 >> rather than '-I' to pass the Buildroot host include directory. '-idirafter'
 >> is basically the same thing, but adds the specified directory at the end
 >> of the include precedence chain, rather than at the beginning.

 > This is breaking the build of uboot host tools for me.

Gaah, that's not what I wanted to hear now that Buildroot 2018.02 is
already one day overdue :/

 > It see the include dir order should be:
 > u-boot
 > buildroot host
 > host os

 > But this patch gives:
 > u-boot
 > host os
 > buildroot host

So what do we do? Would it work to use -isystem instead of -idirafter?
As I read it, this would put it after the -I lines but before the built
in system directories. Can you give that a try?

What other alternatives do we have? Reverting this change would fix it
for people using mainline u-boot with a config needing openssl, but
would break it for Thomas' use cases.

-- 
Bye, Peter Korsgaard

[Buildroot] [master,1/2] uboot: use local fdt headers

[Trent Piepho]

On Thu, 2018-03-01 at 08:42 +0100, Peter Korsgaard wrote:
> > > > > > 
>  > It see the include dir order should be:
>  > u-boot
>  > buildroot host
>  > host os
> 
>  > But this patch gives:
>  > u-boot
>  > host os
>  > buildroot host
> 
> So what do we do? Would it work to use -isystem instead of -idirafter?
> As I read it, this would put it after the -I lines but before the built
> in system directories. Can you give that a try?

This appears to work for me.  I see that uboot is pulling the buildroot
host openssl and not the system openssl.  I also tried installing
libfdt-devel, and uboot is not using those headers.  It's getting some
from uboot and some from the buildroot host includes.  I don't know
what example is supposed to happen or not happen with libfdt, so I'm
not sure how well I have tested against that issue.

> What other alternatives do we have? Reverting this change would fix it
> for people using mainline u-boot with a config needing openssl, but
> would break it for Thomas' use cases.

I think the concept of building multiple libfdts (buildroot's dtc
package's, uboot's, host-uboot-tools', etc.) and trying to get some
code to use one while other code uses another by rejiggering include
paths is doomed.