Re: [PATCH 0/1] tools/nolibc/string: export strlen()

[Thomas Weißschuh]

Hi!

On 2024-01-26 15:24:10+0100, Rodrigo Campos wrote:
> Hi, while using nolibc on debian testing, I found that compilation fails when using strlcat().
> 
> The compilation fails with:
> 
>             cc -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc  -static -o test test.c
>             /usr/bin/ld: /tmp/cccIasKL.o: in function `main':
>             test.c:(.text.startup+0x1e): undefined reference to `strlen'
>             collect2: error: ld returned 1 exit status
> 
> This is using debian testing, with gcc 13.2.0.

I can reproduce the issue with gcc 13.2.1 on Arch.

> A small repro case that fails with this error on debian is:
> 
> 	int main(void) {
> 		char dst[6] = "12";
> 		char *src = "abc";
> 		strlcat(dst, src, 6);
> 	
> 		printf("dst is: %s\n", dst);
> 	
> 		return 0;
> 	}
> 
> Please note that this code is not using strlen() and strlcat() doesn't seems to use it either.

I think the comment in strlen() explains it:

    Note that gcc12 recognizes an strlen() pattern and replaces it with a
    jump to strlen().

strlcat() indeed contains this pattern.

I was able to fix the issue by replacing the open-coded strlen() in
strlcat() with a call to the function and that also fixed the issue.

It seems nicer to me as a fix, on the other hand the change to a weak
definition will also catch other instances of the issue.
Maybe we do both.

> First I noted that removing the attribute unused in strlen(), the compilation worked fine. And then
> I noticied that other functions had the attribute weak, a custom section and export the function.
> 
> In particular, what happens here seems to be the same as in commit "tools/nolibc/string: export memset() and
> memmove()" (8d304a374023), as removing the -Os or adding the -ffreestanding seem to fix the issue.
> So, I did the same as that commit, for strlen().
> 
> However, I'm not 100% confident on how to check that this is done by the compiler to later replace
> it and provide a builtin. I'm not sure how that was verified for commit 8d304a374023, but if you let
> me know, I can verify it too.
> 
> What do you think?

Personally I don't know how it was verified, we'll have to wait for
Willy on that.

> As a side note, it seems strlcat()/strlcpy() fail to set the terminating null byte on some cases,
> and the return code is not always the same as when using libbsd. It seems to be only on "error"
> cases, and not sure if it's worth fixing all/some of those cases.
> 
> Let me know if you think it is worth adding some _simple_ patches (I don't think it is worth fixing
> all the cases, the code is to fix all of the cases is probably not nice and not worth it).

Souns reasonable to me to fix the return values.
And get some tests for it.

> Best,
> Rodrigo
> 
> ---
> 
> 
> Rodrigo Campos (1):
>   tools/nolibc/string: export strlen()