[U-Boot] [PATCH] 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@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 $@
On Wed, May 01, 2019 at 03:08:25PM +0200, 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
Applied to u-boot/master, thanks!
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).
Jan
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
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:
Jan
Best Regards,
Fabrice
So what is your suggestion to fix the O2-regression best?
Jan
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:
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). However, I have to confess that I don't understand why the build breaks "if (!IMAGE_ENABLE_SIGN)" is no longer optimized."
Jan
Fabrice
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:
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.
Jan
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:
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.
Jan
Fabrice
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:
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.
Jan
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:
> > 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.
Jan
Fabrice
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
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@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?
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!
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@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?
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
Jan
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@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?
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@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@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.
participants (3)
-
Fabrice Fontaine -
Jan Kiszka -
Tom Rini