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

[Rodrigo Campos]

On 1/27/24 17:24, Willy Tarreau wrote:
> On Sat, Jan 27, 2024 at 03:53:32PM +0100, Thomas Weißschuh wrote:
> > On 2024-01-26 15:24:10+0100, Rodrigo Campos wrote:
> > > 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.
>
> Yes, once we have the proof that the compiler may produce such a call, it
> can also happen in whatever user code so we need to export the function,
> there's no other solution.

Makes sense, thanks!

> > > 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.
>
> Oh it's very simple, just build a small code that doesn't contain any
> such explicit nor implicit call and check that it doesn't contain the
> function.
>
> E.g >
>    $ printf "int main(void) { }\n" | gcc -nostdlib -static -Isysroot/x86/include -include nolibc.h -Os -Wl,--gc-sections -xc -
>    $ nm --size a.out

Oh, cool. I can confirm that gcc does indeed add the strlen call
(note I had to remove the "--size" param to nm, as the symbol is 
undefined and not shown otherwise)

I wonder if there is an easy way to check for which functions gcc/clang 
do this...

> > > As a side note, it seems strlcat()/strlcpy() fail to set the terminating null byte on some cases,
>
> Indeed I've just checked and you're right, that defeats their purpose!

Cool.

> > > 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.
>
> Seconded!

Thanks, I'll see how to improve that too :)



Best,
Rodrigo