Re: gcc10's -Wreturn-local-addr gives FP warning about lib/careadlinkat

[Kaz Kylheku (Coreutils)]

On 2020-02-06 09:05, Jim Meyering wrote:
> On Thu, Feb 6, 2020 at 6:03 AM Pádraig Brady <address@hidden> wrote:
> > On 06/02/2020 00:27, Jim Meyering wrote:
> > > Building latest latest coreutils using latest-from-git gcc10 evokes
> > > this false positive:
> > >
> > > lib/careadlinkat.c: In function 'careadlinkat':
> > > cc1: error: function may return address of local variable
> > > [-Werror=return-local-addr]
> > > lib/careadlinkat.c:73:8: note: declared here
> > >     73 |   char stack_buf[1024];
> > >
> > > I'm guessing improved flow analysis will eventually suppress this. I
> > > hesitate to turn off the useful and normally-high-S/N
> > > -Wreturn-local-addr globally. Maybe just disable it in that one file,
> > > temporarily?
> >
> > The logic of the function looks fine.
> > Would an `assure (buf != stack_buf)` before the `return buf`
> > indicate that constraint to gcc with minimal runtime overhead?
>
> I would have preferred that, but it has no effect.
> I then tried to suppress that warning in the affected file by adding
> these lines:
>
> /* Without this pragma, gcc 10.0.1 20200205 reports that
>    the "function may return address of local variable".  */
> # pragma GCC diagnostic ignored "-Wreturn-local-addr"
>
> But, surprisingly, that didn't help, either.
> Also tried Kaz Kylheku's return-early suggestion, to no avail.

I have other thoughts about this.

There is a well-known technique for using a array for small
arrays up to a certain size, after which a dynamic array is used.

That technique is useful in cases when dynamic allocation is
avoided entirely.

If the array has to eventually go into
dynamic storage, because it is returned, then it can just
start out that way.

So that is to say, this justification for the stack_buf is
pretty poor:

   /* Allocate the initial buffer on the stack.  This way, in the
      common case of a symlink of small size, we get away with a
      single small malloc() instead of a big malloc() followed by a
      shrinking realloc().  */

The common case is in fact small symlinks; I can't remember
when I've seen a symlink that was anywhere near a kilobyte long.

If you start with, say, a 128 byte malloc buffer, or even a 64
byte one, there is hardly any need to realloc that to a smaller size,
and doing so for chunks of that size might not even actually
make any memory available, depending on the allocator.

The vast majority of symlinks you will ever read will fit into
128 bytes.

Also think about this: depending on the exact filesystem,
small symlinks are stored directly in the inode (or perhaps
even directory entry?)  Whereas large symlinks have to
go to a separate block.

So, okay, there is an overhead *inside* readlink for fetching
a large symlink, and that overhead dwarfs the user-space
concern of whether an extra realloc is called.

readlink may have to read a whole extra data block of storage
containing the symlink, on a cache-cold system. That could
result in a disk seek, bloating up the time into a range
measured in milliseconds.

Basically, the initial guessimate of the required space for
a symlinks should probably be more or less aligned with a
reasonable estimate of the symlink size that is efficiently
handled at the filesystem level.