[U-Boot] [PATCH] tools/Makefile: fix HOSTCFLAGS with CROSS_BUILD_TOOLS

Thu Nov 14 17:24:18 UTC 2019

On Thu, Nov 14, 2019 at 05:52:36PM +0100, Jan Kiszka wrote:
> On 14.11.19 17:41, Tom Rini wrote:
> > On Thu, Nov 14, 2019 at 05:28:06PM +0100, Jan Kiszka wrote:
> > > On 26.08.19 08:43, Fabrice Fontaine wrote:
> > > > Le lun. 26 août 2019 à 07:57, Jan Kiszka <jan.kiszka at web.de> a écrit :
> > > > > 
> > > > > On 25.08.19 21:11, Fabrice Fontaine wrote:
> > > > > > Le dim. 25 août 2019 à 17:49, Jan Kiszka <jan.kiszka at web.de> a écrit :
> > > > > > > 
> > > > > > > On 25.08.19 17:13, Fabrice Fontaine wrote:
> > > > > > > > Le dim. 25 août 2019 à 16:11, Jan Kiszka <jan.kiszka at web.de> a écrit :
> > > > > > > > > 
> > > > > > > > > On 25.08.19 15:43, Fabrice Fontaine wrote:
> > > > > > > > > > Hello,
> > > > > > > > > > Le dim. 25 août 2019 à 13:44, Jan Kiszka <jan.kiszka at web.de> a écrit :
> > > > > > > > > > > 
> > > > > > > > > > > On 01.05.19 15:08, Fabrice Fontaine wrote:
> > > > > > > > > > > > When CROSS_BUILD_TOOLS is set, set HOSTCFLAGS to CFLAGS otherwise CC
> > > > > > > > > > > > will be used with HOSTCFLAGS which seems wrong
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Fabrice Fontaine <fontaine.fabrice at gmail.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >        tools/Makefile | 1 +
> > > > > > > > > > > >        1 file changed, 1 insertion(+)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/tools/Makefile b/tools/Makefile
> > > > > > > > > > > > index 12a3027e23..eadeba417d 100644
> > > > > > > > > > > > --- a/tools/Makefile
> > > > > > > > > > > > +++ b/tools/Makefile
> > > > > > > > > > > > @@ -272,6 +272,7 @@ subdir- += env
> > > > > > > > > > > > 
> > > > > > > > > > > >        ifneq ($(CROSS_BUILD_TOOLS),)
> > > > > > > > > > > >        override HOSTCC = $(CC)
> > > > > > > > > > > > +override HOSTCFLAGS = $(CFLAGS)
> > > > > > > > > > > > 
> > > > > > > > > > > >        quiet_cmd_crosstools_strip = STRIP   $^
> > > > > > > > > > > >              cmd_crosstools_strip = $(STRIP) $^; touch $@
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > This eats - among other things - -O2, normally set in /Makefile. And that breaks
> > > > > > > > > > > CROSS_BUILD_TOOLS=y with CONFIG_FIT but without CONFIG_FIT_SIGNATURE because "if
> > > > > > > > > > > (!IMAGE_ENABLE_SIGN)" is no longer optimized. I tend to believe this should
> > > > > > > > > > > simply be reverted (which is what I'm doing locally in order to fix my builds).
> > > > > > > > > > I don't think this patch should be reverted, I sent it to fix a
> > > > > > > > > > cross-compilation build issue with host-openssl.
> > > > > > > > > > 
> > > > > > > > > > Indeed, without this patch, with CROSS_BUILD_TOOLS=y, tools/Makefile
> > > > > > > > > > sets HOSTCC=$(CC) but it forgets
> > > > > > > > > > to override HOSTCFLAGS, though, so the tools are built with target
> > > > > > > > > > compiler but host CFLAGS.
> > > > > > > > > > 
> > > > > > > > > > This raises a build failure if host-openssl is built before uboot-tools because
> > > > > > > > > > uboot-tools links with openssl headers from host which depends on pthread.h
> > > > > > > > > > 
> > > > > > > > > > More information can be found on buildroot's patchwork here:
> > > > > > > > > > - https://patchwork.ozlabs.org/patch/1092227
> > > > > > > > > > - https://patchwork.ozlabs.org/patch/1093385
> > > > > > > > > > > 
> > > > > > > > > > > Jan
> > > > > > > > > > 
> > > > > > > > > > Best Regards,
> > > > > > > > > > 
> > > > > > > > > > Fabrice
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > So what is your suggestion to fix the O2-regression best?
> > > > > > > > I'm far from being an expert in uboot so please excuse me if my answer
> > > > > > > > contains some mistakes.
> > > > > > > > However, IMHO, we should not pass -O2 or any other optimization flags.
> > > > > > > >     From my understanding, CFLAGS (and so HOSTCFLAGS when building tools
> > > > > > > > for the target through cross-tools) will (or should?) contain -O2 or
> > > > > > > > -Os depending on CONFIG_CC_OPTIMIZE_FOR_SIZE value.
> > > > > > > > However, I do understand that this is a change in the former behavior
> > > > > > > > that was always passing -O2 in HOSTCFLAGS.
> > > > > > > > So, if you really want to pass -O2, I would add it to HOSTCFLAGS in
> > > > > > > > tools/Makefile (after the override).
> > > > > > > 
> > > > > > > This is a hack. U-boot relies on >= -O2 for proper build, so it must inject this
> > > > > > > setting on its own - which it did prior to your change. I think top-level
> > > > > > > HOSTCFLAGS belongs to the build, irrespective of native vs. cross. You may allow
> > > > > > > appending (and, thus, overwriting) settings but you cannot remove mandatory ones.
> > > > > > > 
> > > > > > > > However, I have to confess that I don't understand why the build
> > > > > > > > breaks "if (!IMAGE_ENABLE_SIGN)" is no longer optimized."
> > > > > > > 
> > > > > > > IMAGE_ENABLE_SIGN is 0 when CONFIG_FIT_SIGNATURE is not set. That will remove
> > > > > > > all code from tools/image-host.c which is not reachable then. When it's not
> > > > > > > removed because of default optimization settings, we get linker errors because
> > > > > > > common/image.sig.c is not built without CONFIG_FIT_SIGNATURE. I assume this way
> > > > > > > was preferred over #ifdef'ery, but it's broken now.
> > > > > > Thanks a lot for your explanation.
> > > > > > I was able to reproduce the issue and I think that
> > > > > > https://github.com/ffontaine/u-boot/commit/61a162e5c0343fc55304902042f8c9adfe531f51
> > > > > > should fixed it.
> > > > > > Can you review it? I can also directly send a PR.
> > > > > 
> > > > > I still think that dropping the content of HOSTCFLAGS is the actual bug. There
> > > > > is more than -O2 that you lose. If you want your CFLAGS to be respected, append
> > > > > them.
> > > > I add Arnout to the list to get his feedback as he is the one that
> > > > suggested this solution during review of
> > > > https://patchwork.ozlabs.org/patch/1092227.
> > > 
> > > Nothing happened, bug is still present in master. Any suggestions how to
> > > resolve it?
> > 
> > OK, I've looked over the thread again.  Where and how are you hitting a
> > problem here again?  This area is indeed a bit funky and I see
> > OpenEmbedded has long been doing "shove everything we need in via CC, do
> > not use CFLAGS/etc".  Thanks!
> 
> This is how you can easily reproduce it:
> 
> make avnet_ultra96_rev1_defconfig
> make CROSS_COMPILE=aarch64-linux-gnu- CROSS_BUILD_TOOLS=y

Ah, OK.  So, I wonder about:
commit 72c69ea8d603fd2448dd1d7c399c4f77b77773b7
Author: Fabrice Fontaine <fontaine.fabrice at gmail.com>
Date:   Wed May 1 15:08:25 2019 +0200

    tools/Makefile: fix HOSTCFLAGS with CROSS_BUILD_TOOLS

    When CROSS_BUILD_TOOLS is set, set HOSTCFLAGS to CFLAGS otherwise CC
    will be used with HOSTCFLAGS which seems wrong

    Signed-off-by: Fabrice Fontaine <fontaine.fabrice at gmail.com>

As while it may seem wrong we consistently use KBUILD_CFLAGS for U-Boot
itself (ala the kernel) and HOSTCFLAGS for the tools, both when we run
them on the build host and when we build them for the target.  So for
example the above commit also breaks CONFIG_TOOLS_DEBUG.  Buildroot may
need to fix the problem by appending to HOSTCFLAGS or similar.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191114/f1b89c9a/attachment.sig>