On 26.08.19 08:43, Fabrice Fontaine wrote:
Le lun. 26 août 2019 à 07:57, Jan Kiszka jan.kiszka@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@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@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@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@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/61a162e5c0343fc55304902042f8c9adf... 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?
Jan