Re: [PATCH] newlib: riscv: Fix build

On 3/26/25 4:22 PM, Eric Salem wrote:
> On 3/26/25 8:28 AM, Eric Salem wrote:
>> On 3/25/25 9:15 PM, Eric Salem wrote:
>>> Hi Kito,
>>>
>>> On 3/25/25 8:39 PM, Kito Cheng wrote:
>>>> Just one more comment, don't put xlenint.h in
>>>> newlib/libc/machine/riscv/sys, please move that into
>>>> newlib/libc/machine/riscv
>>>> The difference between the two is that the former will be installed
>>>> into the toolchain, but the latter one won't.
>>>>
>>>> Otherwise LGTM, also verified on my side :)
>>>
>>> I'm trying to move the file to that directory, but now the problem I'm
>>> facing is sys/string.h in riscv can no longer find the file. Newlib
>>> copies the header files to the targ-include directory when building,
>>> but only those that are in the machine and sys directories. So when I go
>>> to build, it can't find xlenint.h anymore.
>>>
>>> Any suggestions? Perhaps I'm doing something wrong, but looking at the
>>> file layout of other ports, this seems intentional by Newlib, where the
>>> libc/machine/<port> directories contain C and assembly files, and all
>>> header files go in subdirectories.
>>>
>>> Eric
>>
>> It looks like if I add the path to Makefile.am, this could work. I'll dig
>> into it today.
> 
> I'm a little concerned about making this change. If you run the following
> find command in newlib/libc/machine:
> 
> find -maxdepth 2 -iname "*.h"
> 
> You'll see all the header files contained in each newlib/libc/machine/<port>
> directory:
> 
> ./aarch64/asmdefs.h
> ./amdgcn/exit-value.h
> ./arc/asm.h
> ./arm/arm_asm.h
> ./h8300/defines.h
> ./h8300/setarch.h
> ./hppa/DEFS.h
> ./i386/i386mach.h
> ./m68k/m68kasm.h
> ./microblaze/mb_endian.h
> ./powerpc/fix64.h
> ./sh/asm.h
> ./spu/c99ppe.h
> ./spu/ea_internal.h
> ./spu/spu_timer_internal.h
> ./spu/straddr.h
> ./spu/strcpy.h
> ./spu/strncmp.h
> ./spu/vec_literal.h
> ./visium/memcpy.h
> ./visium/memset.h
> ./x86_64/x86_64mach.h
> ./xtensa/xtensa-asm.h
> ./z8k/args.h
> 
> If Makefile.am is updated to copy all header files in <port> to targ-include,
> I don't know what the consequences would be for each port, and there's no
> practical way for me to thoroughly test this.
> 
> To reiterate, it's not a C or assembly file in <port> that's including a header
> file that would also be located in <port>. It's a header file (in this case)
> in <port>/sys that needs to also include a header file currently located in
> <port>/sys (currently how the patch would work).
> 
> What does everyone think? I think keeping the new xlenint.h header file in
> <port>/sys would be the safer option.
> 
> Eric

Doing some more digging, when I run:

find -ipath "*/sys/*.h"

in newlib/lib/machine, I get some interesting results for spu:

./spu/sys/custom_file.h
./spu/sys/linux_syscalls.h

I don't think either of these are supposed to be in the sys directory.
However, Christian pointed out:

https://sourceware.org/pipermail/libc-alpha/2017-June/081918.html

mentions __libc_detect_null() shouldn't be present in a public header. So
what I'm going to do is move both sys/string.h and sys/xlenint.h one
directory above, update the includes for the various C and assembly files,
and send a v2 patch.

Jeff, if you don't mind, could you test the patch once it's ready? Just
want to ensure it doesn't break any of your builds.

Thanks,

Eric