| Message ID | 20240903104940.3514994-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Reinstate -nostdinc for CFLAGS_x86_32 | expand |
On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: > Most of Xen is build using -nostdinc and a fully specified include path. > However, the makefile line: > > $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > > discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > > Reinstate -nostdinc, and add the arch include path to the command line. This > is a latent bug for now, but it breaks properly with subsequent include > changes. > > Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Anthony PERARD <anthony.perard@vates.tech> > --- > xen/arch/x86/boot/Makefile | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile > index 03d8ce3a9e48..19eec01e176e 100644 > --- a/xen/arch/x86/boot/Makefile > +++ b/xen/arch/x86/boot/Makefile > @@ -13,11 +13,11 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) > > CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) > $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) > -CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float > +CFLAGS_x86_32 += -Werror -nostdinc -fno-builtin -g0 -msoft-float > ifneq ($(abs_objtree),$(abs_srctree)) > -CFLAGS_x86_32 += -I$(objtree)/include > +CFLAGS_x86_32 += -I$(objtree)/include -I$(objtree)/arch/$(SRCARCH)/include > endif > -CFLAGS_x86_32 += -I$(srctree)/include > +CFLAGS_x86_32 += -I$(srctree)/include -I$(srctree)/arch/$(SRCARCH)/include I think it might be best to just filter out the include paths from XEN_CFLAGS, iow: CFLAGS_x86_32 += $(filter -I%,$(XEN_CFLAGS)) Instead of having to open-code the paths for the include search paths here again. Using the filter leads to the following CC command line: clang -MMD -MP -MF arch/x86/boot/.cmdline.o.d -m32 -march=i686 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Werror -fno-builtin -g0 -msoft-float -mregparm=3 -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -fpic '-D__OBJECT_LABEL__=arch/x86/boot/cmdline.o' -c arch/x86/boot/cmdline.c -o arch/x86/boot/.cmdline.o.tmp -MQ arch/x86/boot/cmdline.o Thanks, Roger.
On 03/09/2024 11:54 am, Roger Pau Monné wrote: > On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: >> Most of Xen is build using -nostdinc and a fully specified include path. >> However, the makefile line: >> >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >> >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >> >> Reinstate -nostdinc, and add the arch include path to the command line. This >> is a latent bug for now, but it breaks properly with subsequent include >> changes. >> >> Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Anthony PERARD <anthony.perard@vates.tech> >> --- >> xen/arch/x86/boot/Makefile | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile >> index 03d8ce3a9e48..19eec01e176e 100644 >> --- a/xen/arch/x86/boot/Makefile >> +++ b/xen/arch/x86/boot/Makefile >> @@ -13,11 +13,11 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) >> >> CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) >> $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) >> -CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float >> +CFLAGS_x86_32 += -Werror -nostdinc -fno-builtin -g0 -msoft-float >> ifneq ($(abs_objtree),$(abs_srctree)) >> -CFLAGS_x86_32 += -I$(objtree)/include >> +CFLAGS_x86_32 += -I$(objtree)/include -I$(objtree)/arch/$(SRCARCH)/include >> endif >> -CFLAGS_x86_32 += -I$(srctree)/include >> +CFLAGS_x86_32 += -I$(srctree)/include -I$(srctree)/arch/$(SRCARCH)/include > I think it might be best to just filter out the include paths from > XEN_CFLAGS, iow: > > CFLAGS_x86_32 += $(filter -I%,$(XEN_CFLAGS)) > > Instead of having to open-code the paths for the include search paths > here again. Using the filter leads to the following CC command line: > > clang -MMD -MP -MF arch/x86/boot/.cmdline.o.d -m32 -march=i686 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Werror -fno-builtin -g0 -msoft-float -mregparm=3 -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -fpic '-D__OBJECT_LABEL__=arch/x86/boot/cmdline.o' -c arch/x86/boot/cmdline.c -o arch/x86/boot/.cmdline.o.tmp -MQ arch/x86/boot/cmdline.o FWIW, https://cirrus-ci.com/build/5930269490806784 shows a good build on FreeBSD with this patch in place. I'd be happy with that approach. It's probably less fragile, although I'll probably go with: CFLAGS_x86_32 += -nostdinc $(filter -I%,$(XEN_CFLAGS)) to handle all the include changes together. Lemme spin a v2. ~Andrew
On 03/09/2024 12:08 pm, Andrew Cooper wrote: > On 03/09/2024 11:54 am, Roger Pau Monné wrote: >> On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: >>> Most of Xen is build using -nostdinc and a fully specified include path. >>> However, the makefile line: >>> >>> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >>> >>> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >>> >>> Reinstate -nostdinc, and add the arch include path to the command line. This >>> is a latent bug for now, but it breaks properly with subsequent include >>> changes. >>> >>> Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >>> --- >>> CC: Jan Beulich <JBeulich@suse.com> >>> CC: Roger Pau Monné <roger.pau@citrix.com> >>> CC: Anthony PERARD <anthony.perard@vates.tech> >>> --- >>> xen/arch/x86/boot/Makefile | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile >>> index 03d8ce3a9e48..19eec01e176e 100644 >>> --- a/xen/arch/x86/boot/Makefile >>> +++ b/xen/arch/x86/boot/Makefile >>> @@ -13,11 +13,11 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) >>> >>> CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) >>> $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) >>> -CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float >>> +CFLAGS_x86_32 += -Werror -nostdinc -fno-builtin -g0 -msoft-float >>> ifneq ($(abs_objtree),$(abs_srctree)) >>> -CFLAGS_x86_32 += -I$(objtree)/include >>> +CFLAGS_x86_32 += -I$(objtree)/include -I$(objtree)/arch/$(SRCARCH)/include >>> endif >>> -CFLAGS_x86_32 += -I$(srctree)/include >>> +CFLAGS_x86_32 += -I$(srctree)/include -I$(srctree)/arch/$(SRCARCH)/include >> I think it might be best to just filter out the include paths from >> XEN_CFLAGS, iow: >> >> CFLAGS_x86_32 += $(filter -I%,$(XEN_CFLAGS)) >> >> Instead of having to open-code the paths for the include search paths >> here again. Using the filter leads to the following CC command line: >> >> clang -MMD -MP -MF arch/x86/boot/.cmdline.o.d -m32 -march=i686 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Werror -fno-builtin -g0 -msoft-float -mregparm=3 -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -fpic '-D__OBJECT_LABEL__=arch/x86/boot/cmdline.o' -c arch/x86/boot/cmdline.c -o arch/x86/boot/.cmdline.o.tmp -MQ arch/x86/boot/cmdline.o > FWIW, https://cirrus-ci.com/build/5930269490806784 shows a good build on > FreeBSD with this patch in place. > > I'd be happy with that approach. It's probably less fragile, although > I'll probably go with: > > CFLAGS_x86_32 += -nostdinc $(filter -I%,$(XEN_CFLAGS)) > > to handle all the include changes together. Lemme spin a v2. Actually, it's not quite that easy. From a regular Xen object file, we have: * -Wa,-I,./include (twice, for some reason). * -include ./include/xen/config.h The former can be added to the filter reasonably easily, but the latter cannot. I guess we've gone this long without it... ~Andrew
On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: > Most of Xen is build using -nostdinc and a fully specified include path. > However, the makefile line: > > $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > > discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > > Reinstate -nostdinc, and add the arch include path to the command line. This > is a latent bug for now, but it breaks properly with subsequent include > changes. > > Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") I disagree with this. I'm pretty sure I've check that no command line have changed. Also, this commit shows that CFLAGS was only coming from root's Config.mk: > diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot > deleted file mode 100644 > index e90680cd9f..0000000000 > --- a/xen/arch/x86/boot/build32.mk > +++ /dev/null > @@ -1,40 +0,0 @@ > -override XEN_TARGET_ARCH=x86_32 > -CFLAGS = > -include $(XEN_ROOT)/Config.mk So, I'm pretty sure, -nostdinc was never used before. But happy to be told that I've come to the wrong conclusion. (We would need to check by building with an old version without this commit to be sure.) "xen/arch/x86/boot" as it's own sets of CFLAGS, which is annoying and I haven't really tried to change that. This is also why XEN_CFLAGS is been discarded. Cheers,
On 03/09/2024 12:20 pm, Anthony PERARD wrote: > On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: >> Most of Xen is build using -nostdinc and a fully specified include path. >> However, the makefile line: >> >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic >> >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. >> >> Reinstate -nostdinc, and add the arch include path to the command line. This >> is a latent bug for now, but it breaks properly with subsequent include >> changes. >> >> Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") > I disagree with this. I'm pretty sure I've check that no command line > have changed. Fine, I'll drop it. > > Also, this commit shows that CFLAGS was only coming from root's > Config.mk: >> diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot >> deleted file mode 100644 >> index e90680cd9f..0000000000 >> --- a/xen/arch/x86/boot/build32.mk >> +++ /dev/null >> @@ -1,40 +0,0 @@ >> -override XEN_TARGET_ARCH=x86_32 >> -CFLAGS = >> -include $(XEN_ROOT)/Config.mk > So, I'm pretty sure, -nostdinc was never used before. But happy to be > told that I've come to the wrong conclusion. (We would need to check by > building with an old version without this commit to be sure.) -nostdinc was added after the fact. Which is fine. But as a consequence of these being unlike the rest of Xen, somehow (and only on FreeBSD it seems), the regular build of Xen is fine, but tools/firmware/xen-dir for the shim is subject to -nostdinc in a way that breaks cmdline.c > "xen/arch/x86/boot" as it's own sets of CFLAGS, which is annoying and I > haven't really tried to change that. This is also why XEN_CFLAGS is been > discarded. This is necessary. We're swapping -m64 for -m32 to build these two objects, and that invalidates a whole bunch of other CFLAGS. ~Andrew
On Tue, Sep 03, 2024 at 12:16:33PM +0100, Andrew Cooper wrote: > On 03/09/2024 12:08 pm, Andrew Cooper wrote: > > On 03/09/2024 11:54 am, Roger Pau Monné wrote: > >> On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: > >>> Most of Xen is build using -nostdinc and a fully specified include path. > >>> However, the makefile line: > >>> > >>> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > >>> > >>> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > >>> > >>> Reinstate -nostdinc, and add the arch include path to the command line. This > >>> is a latent bug for now, but it breaks properly with subsequent include > >>> changes. > >>> > >>> Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") > >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >>> --- > >>> CC: Jan Beulich <JBeulich@suse.com> > >>> CC: Roger Pau Monné <roger.pau@citrix.com> > >>> CC: Anthony PERARD <anthony.perard@vates.tech> > >>> --- > >>> xen/arch/x86/boot/Makefile | 6 +++--- > >>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile > >>> index 03d8ce3a9e48..19eec01e176e 100644 > >>> --- a/xen/arch/x86/boot/Makefile > >>> +++ b/xen/arch/x86/boot/Makefile > >>> @@ -13,11 +13,11 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) > >>> > >>> CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) > >>> $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) > >>> -CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float > >>> +CFLAGS_x86_32 += -Werror -nostdinc -fno-builtin -g0 -msoft-float > >>> ifneq ($(abs_objtree),$(abs_srctree)) > >>> -CFLAGS_x86_32 += -I$(objtree)/include > >>> +CFLAGS_x86_32 += -I$(objtree)/include -I$(objtree)/arch/$(SRCARCH)/include > >>> endif > >>> -CFLAGS_x86_32 += -I$(srctree)/include > >>> +CFLAGS_x86_32 += -I$(srctree)/include -I$(srctree)/arch/$(SRCARCH)/include > >> I think it might be best to just filter out the include paths from > >> XEN_CFLAGS, iow: > >> > >> CFLAGS_x86_32 += $(filter -I%,$(XEN_CFLAGS)) > >> > >> Instead of having to open-code the paths for the include search paths > >> here again. Using the filter leads to the following CC command line: > >> > >> clang -MMD -MP -MF arch/x86/boot/.cmdline.o.d -m32 -march=i686 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Werror -fno-builtin -g0 -msoft-float -mregparm=3 -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -fpic '-D__OBJECT_LABEL__=arch/x86/boot/cmdline.o' -c arch/x86/boot/cmdline.c -o arch/x86/boot/.cmdline.o.tmp -MQ arch/x86/boot/cmdline.o > > FWIW, https://cirrus-ci.com/build/5930269490806784 shows a good build on > > FreeBSD with this patch in place. > > > > I'd be happy with that approach. It's probably less fragile, although > > I'll probably go with: > > > > CFLAGS_x86_32 += -nostdinc $(filter -I%,$(XEN_CFLAGS)) > > > > to handle all the include changes together. Lemme spin a v2. > > Actually, it's not quite that easy. From a regular Xen object file, we > have: > > * -Wa,-I,./include (twice, for some reason). There's a comment next to where this is added: # Set up the assembler include path properly for older toolchains. But won't we also need extra -Wa,-I for the other include paths that are passed on the command line? (./arch/x86/include for example) > * -include ./include/xen/config.h Hm, we could manually add that include option. > > The former can be added to the filter reasonably easily, but the latter > cannot. I guess we've gone this long without it... I've been also thinking, another approach we could take is filtering out what we don't want, but I think that might end up being more fragile. Thanks, Roger.
On Tue, Sep 03, 2024 at 12:51:28PM +0100, Andrew Cooper wrote: > On 03/09/2024 12:20 pm, Anthony PERARD wrote: > > On Tue, Sep 03, 2024 at 11:49:40AM +0100, Andrew Cooper wrote: > >> Most of Xen is build using -nostdinc and a fully specified include path. > >> However, the makefile line: > >> > >> $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic > >> > >> discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. > >> > >> Reinstate -nostdinc, and add the arch include path to the command line. This > >> is a latent bug for now, but it breaks properly with subsequent include > >> changes. > >> > >> Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") > > I disagree with this. I'm pretty sure I've check that no command line > > have changed. > > Fine, I'll drop it. > > > > > Also, this commit shows that CFLAGS was only coming from root's > > Config.mk: > >> diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot > >> deleted file mode 100644 > >> index e90680cd9f..0000000000 > >> --- a/xen/arch/x86/boot/build32.mk > >> +++ /dev/null > >> @@ -1,40 +0,0 @@ > >> -override XEN_TARGET_ARCH=x86_32 > >> -CFLAGS = > >> -include $(XEN_ROOT)/Config.mk > > So, I'm pretty sure, -nostdinc was never used before. But happy to be > > told that I've come to the wrong conclusion. (We would need to check by > > building with an old version without this commit to be sure.) > > -nostdinc was added after the fact. Which is fine. > > But as a consequence of these being unlike the rest of Xen, somehow (and > only on FreeBSD it seems), the regular build of Xen is fine, but > tools/firmware/xen-dir for the shim is subject to -nostdinc in a way > that breaks cmdline.c FWIW, it did break for me on a normal build in xen/ on FreeBSD, no need to run it from tools/firmware/xen-dir. > > "xen/arch/x86/boot" as it's own sets of CFLAGS, which is annoying and I > > haven't really tried to change that. This is also why XEN_CFLAGS is been > > discarded. > > This is necessary. We're swapping -m64 for -m32 to build these two > objects, and that invalidates a whole bunch of other CFLAGS. See my previous reply, I guess figuring out which of those options also need to be dropped as a result of dropping -m64 is likely too complicated and fragile as we add new options. Thanks, Roger.
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index 03d8ce3a9e48..19eec01e176e 100644 --- a/xen/arch/x86/boot/Makefile +++ b/xen/arch/x86/boot/Makefile @@ -13,11 +13,11 @@ $(obj)/head.o: $(head-bin-objs:.o=.bin) CFLAGS_x86_32 := $(subst -m64,-m32 -march=i686,$(XEN_TREEWIDE_CFLAGS)) $(call cc-options-add,CFLAGS_x86_32,CC,$(EMBEDDED_EXTRA_CFLAGS)) -CFLAGS_x86_32 += -Werror -fno-builtin -g0 -msoft-float +CFLAGS_x86_32 += -Werror -nostdinc -fno-builtin -g0 -msoft-float ifneq ($(abs_objtree),$(abs_srctree)) -CFLAGS_x86_32 += -I$(objtree)/include +CFLAGS_x86_32 += -I$(objtree)/include -I$(objtree)/arch/$(SRCARCH)/include endif -CFLAGS_x86_32 += -I$(srctree)/include +CFLAGS_x86_32 += -I$(srctree)/include -I$(srctree)/arch/$(SRCARCH)/include # override for 32bit binaries $(head-bin-objs): CFLAGS_stack_boundary :=
Most of Xen is build using -nostdinc and a fully specified include path. However, the makefile line: $(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic discards XEN_CFLAGS and replaces them with CFLAGS_x86_32. Reinstate -nostdinc, and add the arch include path to the command line. This is a latent bug for now, but it breaks properly with subsequent include changes. Fixes: d58a509e01c4 ("build,x86: remove the need for build32.mk") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Anthony PERARD <anthony.perard@vates.tech> --- xen/arch/x86/boot/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)