Re: blowfish failure to compile

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/02/2025 10:06 am, Jan Beulich wrote:
> 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.

It's awkward.  With glibc, it's the glibc-i386-devel headers you need,
but this doesn't work in clean-64bit-only build environments, or in
Alpine where Musl has Opinions on how glibc works, and they go as far as
patching gcc to behave differently.

Alpine is what caused us to finally stop trying to use the compiler stdint.h

>
>> 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?

-ffreestanding seems fine.

And while -nostdinc -I... works for the 32bit builds, they break the
64bit builds.

> In file included from blowfish.c:18:
> /builddir/build/BUILD/xen-4.20.0/tools/tests/x86_emulator/../../../tools/firmware/include/stdint.h:5:2:
> error: #error "32bit only header"
>     5 | #error "32bit only header"
>       |  ^~~~~
> make[6]: *** [testcase.mk:16: blowfish.bin] Error 1

which is because we've only provided half a stdint.h

I think that means we only want the -nostdinc -I... in the cross-build
case, which I guess means searching CFLAGS for `-m32`.




>
>>> 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.)

I don't have a good suggestion.  More an observation than this is too
complicated for me to figure out how it works with half an hour of trying?

~Andrew