Re: blowfish failure to compile

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.02.2025 18:20, Andrew Cooper wrote:
> I've noticed the following failure in XenServer's build environment
> 
>> make[6]: Leaving directory
>> '/builddir/build/BUILD/xen-4.19.1/tools/tests/x86_emulator'
>> In file included from /usr/include/features.h:535,
>>                  from /usr/include/bits/libc-header-start.h:33,
>>                  from /usr/include/stdint.h:26,
>>                  from
>> /usr/lib/gcc/x86_64-xenserver-linux/12/include/stdint.h:9,
>>                  from blowfish.c:18:
>> /usr/include/gnu/stubs.h:7:11: fatal error: gnu/stubs-32.h: No such
>> file or directory
>>     7 | # include <gnu/stubs-32.h>
>>       |           ^~~~~~~~~~~~~~~~
>> compilation terminated.
>> make[6]: *** [testcase.mk:15: blowfish.bin] Error 1
> 
> It's non-fatal, but it reduces the content in test_x86_emulator which we
> do care about running.

Hmm, yes, I did see such in the past, and solved it by putting the seemingly
missing header in place on the distro.

> Elsewhere in the tree we fix this with -ffreestanding -nostdinc
> -I$(XEN_ROOT)/tools/firmware/include but that isn't an option for
> test_x86_emulator in general which is hosted.
> 
> However, it is an option for blowfish.c specifically which is
> freestanding, and for which we build a 32bit form in an otherwise 64bit
> build.
> 
> Therefore, it stands to reason that:
> 
> diff --git a/tools/tests/x86_emulator/Makefile
> b/tools/tests/x86_emulator/Makefile
> index 294d27ebaa08..e46fd8becb96 100644
> --- a/tools/tests/x86_emulator/Makefile
> +++ b/tools/tests/x86_emulator/Makefile
> @@ -33,8 +33,8 @@ HOSTCFLAGS += -m32 -I..
>  
>  else
>  
> -blowfish-cflags := ""
> -blowfish-cflags-x86_32 := "-mno-accumulate-outgoing-args -Dstatic="
> +blowfish-cflags := "-ffreestanding -nostdinc
> -I$(XEN_ROOT)/tools/firmware/include "
> +blowfish-cflags-x86_32 := "$(blowfish-cflags)
> -mno-accumulate-outgoing-args -Dstatic="

What this does is request the shared (between 32- and 64-bit)) flavor to
be built differently, with the options "-ffreestanding -nostdinc
-I$(XEN_ROOT)/tools/firmware/include". And then the (kind of) nested use
of double quotes in blowfish-cflags-x86_32 ends up asking for several
32-bit flavors: One with -ffreestanding, one with -nostdinc, one with
-I$(XEN_ROOT)/tools/firmware/include (which is what causes the
strangeness you saw), and the pre-existing one with
"-mno-accumulate-outgoing-args -Dstatic=".

Every set of options grouped together by double quotes (or any unquoted
option) designates a flavor (while the quotation isn't meaningful to
make aiui, its use is in a shell construct, where those quotes play
their usual role). That is,

blowfish-cflags := ""

designates a flavor without any special options. What I understand you
want, though, is to have these flags passed to all of the blowfish
flavors.

What complicates things slightly is that the first of the options names
the flavor (i.e. prior to your change, but with my APX changes in place,
we have

blowfish_x86_32[]
blowfish_x86_32_mno_accumulate_outgoing_args[]
blowfish_x86_64[]
blowfish_x86_64_DREX2[]
blowfish_x86_64_mapxf[]

resulting from

blowfish-cflags := ""
blowfish-cflags-x86_32 := "-mno-accumulate-outgoing-args -Dstatic="
blowfish-cflags-x86_64 := "-DREX2 -Dstatic=" "-mapxf -Dstatic="

. I think you can see now how the compiler ends up choking on

blowfish_x86_32_I/local/xen.spec/scm/tools/tests/x86_emulator/../../../tools/firmware/include[]

.) Surely we could accommodate for the added options by changing the
references from test_x86_emulator.c, but maybe there's a better way
(and also potentially useful for other test blobs going forward),
modifying the .h generator rule(s):

                $(MAKE) -f testcase.mk TESTCASE=$* XEN_TARGET_ARCH=$(arch) 
$*-cflags="$$cflags $($*-cflags-common)" all; \

and then the needed addition simply being

blowfish-cflags-common := -ffreestanding -nostdinc 
-I$(XEN_ROOT)/tools/firmware/include

Entirely untested, though, for now.

However, further: The freestanding-ness does apply to all of the test
blobs, doesn't it? Why don't we alter

CFLAGS += -fno-builtin -g0 $($(TESTCASE)-cflags) $(CFLAGS-VSZ)

in testcase.mk to become

CFLAGS += -ffreestanding -nostdinc -I$(XEN_ROOT)/tools/firmware/include
CFLAGS += -g0 $($(TESTCASE)-cflags) $(CFLAGS-VSZ)

(which doesn't appear to become dependent upon anything we don't already
have available in this file, i.e. in particular $(XEN_ROOT) is already
used elsewhere), seeing that -ffreestanding implies -fno-builtin?

>> blowfish.h:617:99: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
>> ‘__attribute__’ before ‘/’ token
>>   617 | static const unsigned int __attribute__((section(".test,
>> \"ax\", @progbits #")))
>> blowfish_x86_32_I/local/xen.spec/scm/tools/tests/x86_emulator/../../../tools/firmware/include[]
>> = {
>>      
>> |                                                                            
>>                       
>> ^
> 
> and at this point I've got completely lost in this build system.  The .h
> generation seems to loop over each cflag, and while that looks plausible
> for vector generation, I can't see how it works (except by accident) for
> blowfish.
> 
> The problem is the generation of $flavor, but this logic is completely
> opaque.

Can you suggest how to achieve the same in a less opaque way? (Surely it
having grown over time has made quite a bit worse what may have been
okay-ish in the beginning.)

Jan