Received-SPF: pass (google.com: domain of linux-kernel+bounces-65468-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 14 Feb 2024 16:55:45 +0100
From: Willy Tarreau <w@1wt.eu>
To: Rodrigo Campos <rodrigo@sdfg.com.ar>
Cc: Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] tools/nolibc: Fix strlcpy() return code and size
 usage
Message-ID: <ZczigTyNa5dqtKRy@1wt.eu>
References: <20240129141516.198636-1-rodrigo@sdfg.com.ar>
 <20240129141516.198636-4-rodrigo@sdfg.com.ar>
 <20240211110814.GB19364@1wt.eu>
 <cc997fd5-1478-43fc-8ba0-aba5e7b3bfdc@sdfg.com.ar>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cc997fd5-1478-43fc-8ba0-aba5e7b3bfdc@sdfg.com.ar>

On Wed, Feb 14, 2024 at 12:50:53PM -0300, Rodrigo Campos wrote:
> On 2/11/24 12:08, Willy Tarreau wrote:
> > Hi Rodrigo,
> > 
> > It's good, but for the same reason as the previous one, I'm getting
> > smaller code by doing less in the loop. Also calling strlen() here
> > looks expensive, I'm seeing that the compiler inlined it nevertheless
> > and did it in a dep-optimized way due to the asm statement. That
> > results in 67 bytes total while a simpler version gives 47.
> > 
> > If I explicitly mark strlen() __attribute__((noinline)) that prevents
> > it from doing so starting with gcc-10, where it correctly places a jump
> > from strlcpy() to strlen() and ends up with 50 bytes (vs 44 for the alt
> > one). The other one I can propose is directly derived from the other
> > strlcat() variant, which first performs the copy and starts to count:
> > 
> > size_t strlcpy(char *dst, const char *src, size_t size)
> > {
> >          size_t len;
> > 
> >          for (len = 0; len < size; len++) {
> >                  if (!(dst[len] = src[len]))
> >                          return len;
> >          }
> > 
> >          /* end of src not found before size */
> >          if (size)
> >                  dst[size - 1] = '\0';
> > 
> >          while (src[len])
> >                  len++;
> > 
> >          return len;
> > }
> > 
> > Just let me know what you think.
> 
> This is one is very nice, thanks!
> 
> Sorry I didn't think about the size at all when writing the functions :)

Never be sorry, low-level user code like this is never trivial and
that's the goal of the nolibc-test in the first place ;-)

> We can change the loop to be:
> 
>         for (len = 0; len < size; len++) {
>                 dst[len] = src[len];
>                 if (!dst[len])
>                         break;
>         }
> 
> That IMHO it is slightly more readable and makes it only 2 bytes longer
> here.

It's not exactly the same, it will always write a zero at dst[size-1]
due to the break statement. As much as I hate returns in the middle,
this one made sense for this case. A goto to the final return statement
is fine as well.

> What do you think? I'm fine with both, of course.

I'm fine with the more readable part (I also prefer it) but not the use
of break here.

> If I resend, shall I add a suggested-by or directly you as the author?

No need for either, it's your work, my part was just a review and an
addictive temptation to look at asm code ;-)

Cheers,
Willy