| Message ID | 65338578-dd6c-4f01-807e-da389cc60cb8@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | blowfish failure to compile | expand |
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
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
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
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=" 3dnow-vecs := 8 3dnow-ints :=