Re: blowfish failure to compile

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2025 19:32, Andrew Cooper wrote:
> On 13/02/2025 10:06 am, Jan Beulich wrote:
>> On 12.02.2025 18:20, Andrew Cooper wrote:
>>> 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`.

This or make the 64-bit case work in tools/firmware/include/stdint.h.
At some point that may end up being necessary anyway.

Jan