Re: [committed 0/2] CRIS: Fix compilation warnings that recent gcc treats as errors

On 2023-12-23 17:44, Hans-Peter Nilsson wrote:
> Half-way through your mail, I thought better put this
> together.  It doesn't address all of it though.
> 
>> Date: Sat, 23 Dec 2023 11:58:42 +0100
>> From: Torbjorn SVENSSON <[email protected]>
> 
>> I'm using a combination of real evaluation boards and the qemu simulator.
> 
> (and a patched qemu, IIUC)

The patch is only to add a machine definition in qemu that has a 
matching address space. It has no impact on the instructions executed.

>> Unfortunately, I cannot share it as it's tied to how the internal
>> systems are setup at ST.
> 
> That's very unfortunate.
> 
>>> To wit, if you use something that gives other results than
>>> when configuring for --target=arm-eabi and testing with
>>> RUNTESTFLAGS=--target_board=arm-sim then your gcc testing
>>> isn't matching the expectations in libstdc++-v3 for a newlib
>>> target (IOW "not correct").
>>
>> I'm pretty sure it didn't match the expectations before
> 
> (Well, it did, for cris-elf+cris-sim.  I still have to check
> whether something was actually missing for
> arm-eabi+arm-sim.)
> 
>> either as it
>> picked up the header definition for _getentropy during the configure
>> stage,
> 
> Confusion.  There was no _getentropy (or getentropy) before
> you added it, so how could it be picked up?  Not ruling out
> bugs in libstdc++-v3 configuration here.  Maybe it didn't
> recognize your mix as a newlib target?  (It should default
> to it from the target tuple, but you can always add a
> --with-newlib.)

Yes there was (pardon the typo in the previous mail as the header only 
included getentropy and not _getentropy): 
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/unistd.h;h=5386bd49dc4f700180c94b4a6ab56c8196e6f77b;hb=70ee6b17df9b055a05cdcc4d3fe1813d7b57e2d8#l107

That prototype is picked up by the configure script in libstdc++ here: 
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/configure;h=bef2b840a595b6ec3a2c24eb3efb04f6835f62b7;hb=refs/heads/master#l51573

As I am cross compiling, it will not try to link and thus, it's 
perfectly legal (from the POV of the C compiler) to have the prototype 
but no implementation and thus, it thinks that getentropy is available 
while it's not.

> If you didn't post your complete configure line and
> config.log in previous reports and conversions to newlib@
> and gcc lists, please do.

I have not, but it's more or less the same flags that are used in the 
official arm-none-eabi build from Arm 
(https://developer.arm.com/downloads/-/arm-gnu-toolchain-downloads).

newlib:
.../src/newlib/configure --build=x86_64-linux-gnu 
--host=x86_64-linux-gnu --target=arm-none-eabi 
--prefix=.../install-native 
--infodir=.../install-native/share/doc/gcc-arm-none-eabi/info 
--mandir=.../install-native/share/doc/gcc-arm-none-eabi/man 
--htmldir=.../install-native/share/doc/gcc-arm-none-eabi/html 
--pdfdir=.../install-native/share/doc/gcc-arm-none-eabi/pdf 
--enable-newlib-io-long-long --enable-newlib-io-c99-formats 
--enable-newlib-reent-check-verify --enable-newlib-register-fini 
--enable-newlib-retargetable-locking --disable-newlib-supplied-syscalls 
--disable-nls

GCC:
.../src/gcc/configure --target=arm-none-eabi --prefix=.../install-native 
--libexecdir=.../install-native/lib 
--infodir=.../install-native/share/doc/gcc-arm-none-eabi/info 
--mandir=.../install-native/share/doc/gcc-arm-none-eabi/man 
--htmldir=.../install-native/share/doc/gcc-arm-none-eabi/html 
--pdfdir=.../install-native/share/doc/gcc-arm-none-eabi/pdf 
--enable-languages=c,c++ --enable-plugins --disable-decimal-float 
--disable-libffi --disable-libgomp --disable-libmudflap 
--disable-libquadmath --disable-libssp --disable-libstdcxx-pch 
--disable-nls --disable-shared --disable-threads --disable-tls 
--with-gnu-as --with-gnu-ld --with-newlib --with-headers=yes 
--with-python-dir=share/gcc-arm-none-eabi 
--with-sysroot=.../install-native/arm-none-eabi --build=x86_64-linux-gnu 
--host=x86_64-linux-gnu --with-gmp=.../build-native/host-libs/usr 
--with-mpfr=.../build-native/host-libs/usr 
--with-mpc=.../build-native/host-libs/usr 
--with-isl=.../build-native/host-libs/usr 
'--with-host-libstdcxx=-static-libgcc -Wl,-Bstatic,-lstdc++,-Bdynamic 
-lm' --with-multilib-list=rmprofile,aprofile

> 
>> but when linking, there was no such function defined in the
>> newlib provided archives, thus the following error (taken from the
>> commit message of b9e867d088935d9f0bf312e6dbf3e4976850dfd3) was raised
>> during testing of the g++ tool:
>>
>>   
>> /build/gcc-13-2709-g9ac9fde961f/bin/../lib/gcc/arm-none-eabi/13.0.0/../../../../arm-none-eabi/bin/ld:
>> /build/gcc-13-2709-g9ac9fde961f/bin/../lib/gcc/arm-none-eabi/13.0.0/../../../../arm-none-eabi/lib/thumb/v6-m/nofp/libstdc++.a(random.o):
>> in function `std::(anonymous namespace)::__libc_getentropy(void*)':
>>       (.text._ZNSt12_GLOBAL__N_117__libc_getentropyEPv+0x8): undefined
>> reference to `getentropy'
> 
> Right; that means it was *wrongly* identified as present at
> configure-time.  The solution is then not to add a stub, but
> to fix the wrong present-identification.

Well, arc4random, that is part of newlib, also assumes that getentropy 
exists.
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/stdlib/arc4random.c;h=09a134c3b9dabec8ae414223f082e9dfdcaaeb3a;hb=refs/heads/main#l101

This part was added by Sebastian Huber back in 2016: 
https://sourceware.org/git/?p=newlib-cygwin.git;a=commit;f=newlib/libc/stdlib/arc4random.c;h=f74cf1350e4633892b6ee6db3664eec38579a709

>> In my case (cross building and cross testing) is that without my patch,
>> the failure happens in the testsuite.
> 
> Odd; I'll re-read that conversion.  The lack of _getentropy
> (and getentropy) before, *should* not have posed any problem.

Well it does. :)
As there are other things in newlib that depend on this symbol, I though 
it was the right move to add the stub for _getentropy (and the wrapping 
getentropy function) as it "completes" the chain for other parts of 
newlib as well as allowing the proper handling in libstdc++.
If you think the correct approach is to rip out getentropy, _getentropy 
and the arc4random stuff from newlib, then I suppose you should supply a 
patch for it.

>> So, in that case, just add the following to the library? It should be
>> enough to make your simulator happy and it can still be overridden when
>> using newlib built for the cris architecture in real applications.
>>
>> int _getentropy(void *buf, size_t buflen)
>> {
>>     errno = ENOSYS;
>>     return -1;
>> }
>>
> 
> That won't fly.  I mentioned that pruning the stub warning
> exposes runtime errors, so that'll lead to *regressions for
> those libstdc++ tests that expect randomness support to
> work, compared to before the _getentropy stub*.  (Before,
> libstdc++ fell back to to another randomness
> implementation.)

No it does not.
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/src/c%2B%2B11/random.cc;h=75989bd33375800d04a5ca5788f99c103c199ea2;hb=refs/heads/master#l468
I interpret this as the program can be compiled to support a certain 
random source and then at runtime, it's verified that said random source 
is available or not.
Do you make a different interpretation?

>> Looking forward to your "proper solution" as this appears to work fine
>> if you just define the low level implementation for your architecture.
> 
> A proper solution is one that matches what's actually
> implemented in newlib and libgloss.
> 
> It worked before the _getentropy stub, it fails afterwards.
> 
> IMHO it's preferable to target off-the-shelf buildable
> toolchain combinations rather than homegrown collections; if
> the latter work, then fine, but if they break what's already
> there, then that collection should be adjusted, not the rest
> of newlib+libgloss.

I see no reason why the set of newlib, libgloss or GCC matters for this.
When doing a cross compile, the header file is all that matters for the 
support to be picked up by libstdc++ configure script. As such, it will 
later fail to link with said library as the implementation is missing.

As I said before, either we need to rip out *everything* that loops back 
to getentropy from newlib or there needs to be something that provides a 
working implementation for your use-case. As you appear to be running 
the cris target on linux, I suggest that you simply wrap it to the 
getentropy call on the host for your simulation.

Keep in mind that I don't really care about what works with a simulator 
or not, what I focus on is if real applications will work or not. My way 
of testing the arm-none-eabi target is as close as I can get to real 
application scenarios as it more or less matches how our customers will 
use the toolchain. This is the important part!
Also, I'm not just testing a particular Cortex-M target, I'm testing all 
of the Cortex-M targets that are available in the STM32 portfolio with 
and without FPU. As I'm verifying that all these combinations are 
correct (according to the dejagnu suite), you see why arm-sim will not 
be enough for me.


Kind regards,
Torbjörn