[PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Andrew Cooper]

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
 
 # override for 32bit binaries
 $(head-bin-objs): CFLAGS_stack_boundary :=
-- 
2.39.2

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Roger Pau Monné]

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.

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Andrew Cooper]

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

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Andrew Cooper]

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

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Anthony PERARD]

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,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Andrew Cooper]

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

[PATCH v2] x86/boot: Fix include paths for 32bit objects

[Andrew Cooper]

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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.

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>

v2:
 * Copy all -I from XEN_CFLAGS

https://cirrus-ci.com/build/6740464739549184

Note that this misses the overall '-include ./include/xen/config.h' but it was
missing before as well.
---
 xen/arch/x86/boot/Makefile | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 03d8ce3a9e48..1ec2b123305d 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -14,10 +14,7 @@ $(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
-ifneq ($(abs_objtree),$(abs_srctree))
-CFLAGS_x86_32 += -I$(objtree)/include
-endif
-CFLAGS_x86_32 += -I$(srctree)/include
+CFLAGS_x86_32 += -nostdinc $(filter -I% -Wa$(comma)-I%,$(XEN_CFLAGS))
 
 # override for 32bit binaries
 $(head-bin-objs): CFLAGS_stack_boundary :=
-- 
2.39.2

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Roger Pau Monné]

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.

Re: [PATCH] x86/boot: Reinstate -nostdinc for CFLAGS_x86_32

[Roger Pau Monné]

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.

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 12:53:34PM +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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I wouldn't mind if you also open-coded the config.h -include addition
to CFLAGS_x86_32, regardless:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I do wonder however whether the explicit assembler includes parameters
(-Wa,-I) are actually required, seeing as we only provide include/ to
the assembler, but not the arch-specific include paths.

This is from XSA-254, which used the '.include' asm directive, but
that was ultimately removed by:

762c3890c89f x86: fold indirect_thunk_asm.h into asm-defns.h

So maybe the -Wa,-I is no longer needed?

Thanks, Roger.

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Andrew Cooper]

On 03/09/2024 1:43 pm, Roger Pau Monné wrote:
> On Tue, Sep 03, 2024 at 12:53:34PM +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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I wouldn't mind if you also open-coded the config.h -include addition
> to CFLAGS_x86_32, regardless:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

TBH, I'm going to put it in as is and unblock the fixes behind it.

We can adjust the others in due course.

Given the other shuffling of headers we've done recently, I'm starting
to think that the -include isn't really as needed as it might once have
been.

> I do wonder however whether the explicit assembler includes parameters
> (-Wa,-I) are actually required, seeing as we only provide include/ to
> the assembler, but not the arch-specific include paths.
>
> This is from XSA-254, which used the '.include' asm directive, but
> that was ultimately removed by:
>
> 762c3890c89f x86: fold indirect_thunk_asm.h into asm-defns.h
>
> So maybe the -Wa,-I is no longer needed?

Perhaps, although I'm struggling to find where it's declared today.

~Andrew

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Roger Pau Monné]

On Tue, Sep 03, 2024 at 02:01:45PM +0100, Andrew Cooper wrote:
> On 03/09/2024 1:43 pm, Roger Pau Monné wrote:
> > On Tue, Sep 03, 2024 at 12:53:34PM +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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > I wouldn't mind if you also open-coded the config.h -include addition
> > to CFLAGS_x86_32, regardless:
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> TBH, I'm going to put it in as is and unblock the fixes behind it.
> 
> We can adjust the others in due course.

Sure, if that allows you to unblock the rest.

> Given the other shuffling of headers we've done recently, I'm starting
> to think that the -include isn't really as needed as it might once have
> been.
> 
> > I do wonder however whether the explicit assembler includes parameters
> > (-Wa,-I) are actually required, seeing as we only provide include/ to
> > the assembler, but not the arch-specific include paths.
> >
> > This is from XSA-254, which used the '.include' asm directive, but
> > that was ultimately removed by:
> >
> > 762c3890c89f x86: fold indirect_thunk_asm.h into asm-defns.h
> >
> > So maybe the -Wa,-I is no longer needed?
> 
> Perhaps, although I'm struggling to find where it's declared today.

It's in xen/arch/x86/arch.mk.

Thanks, Roger.

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Jan Beulich]

On 03.09.2024 13:53, 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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.

And the files in boot/ indeed need the -Wa,-I? I'd favor us not propagating
such without actually being required for a particular reason.

Jan

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Andrew Cooper]

On 03/09/2024 3:32 pm, Jan Beulich wrote:
> On 03.09.2024 13:53, 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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
> And the files in boot/ indeed need the -Wa,-I? I'd favor us not propagating
> such without actually being required for a particular reason.

Hmm ok fine, I'll strip that back out as both you and Roger have asked
for it.

I'm still waiting on Gitlab CI, so haven't pushed yet.

~Andrew

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Andrew Cooper]

On 03/09/2024 3:33 pm, Andrew Cooper wrote:
> On 03/09/2024 3:32 pm, Jan Beulich wrote:
>> On 03.09.2024 13:53, 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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
>> And the files in boot/ indeed need the -Wa,-I? I'd favor us not propagating
>> such without actually being required for a particular reason.
> Hmm ok fine, I'll strip that back out as both you and Roger have asked
> for it.
>
> I'm still waiting on Gitlab CI, so haven't pushed yet.

Eclair has just said no, because the -include is needed to pull in
Kconfig, ahead of compiler.h using CONFIG_GCC_* symbols

I'm retesting with:

@@ -14,10 +14,8 @@ $(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
-ifneq ($(abs_objtree),$(abs_srctree))
-CFLAGS_x86_32 += -I$(objtree)/include
-endif
-CFLAGS_x86_32 += -I$(srctree)/include
+CFLAGS_x86_32 += -nostdinc -include $(filter
%/include/xen/config.h,$(XEN_CFLAGS))
+CFLAGS_x86_32 += $(filter -I%,$(XEN_CFLAGS))
 
 # override for 32bit binaries
 $(head-bin-objs): CFLAGS_stack_boundary :=

which is the best I can think of to find the path of config.h in
XEN_CFLAGS.  This is the same pattern we use in filter-out elsewhere.

~Andrew

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Jan Beulich]

On 03.09.2024 13:53, 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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Despite the title saying "Fix" I take the absence of a Fixes: tag as meaning
that this won't need backporting, and is rather only needed for what went on
top.

Jan

Re: [PATCH v2] x86/boot: Fix include paths for 32bit objects

[Andrew Cooper]

On 04/09/2024 7:54 am, Jan Beulich wrote:
> On 03.09.2024 13:53, 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 copy all -I (and -Wa,-I) arguments from XEN_CFLAGS.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Despite the title saying "Fix" I take the absence of a Fixes: tag as meaning
> that this won't need backporting, and is rather only needed for what went on
> top.

Oh sorry.  v1 had a Fixes tag, and then Anthony objected.

"x86/boot: Use <xen/types.h>" is where it breaks properly without this
patch, so I don't think there's a specific need to backport.

~Andrew