Re: [PATCH] careadlinkat: fix GCC 10 workaround

[Bruno Haible]

Paul Eggert wrote:
> What's wrong with avoiding the problem via GCC_LINT?

Some people, especially those that build distros, want to get warnings about
the exact code that they ship in the distro, not about some different code.

> I suppose you could add -Wno-return-local-addr to your CFLAGS, but I don't 
> recommend this as it's normally a useful warning.

Right.

How about this modification? It splits the function into two functions, and
unless the first function is inlined, the warning disappears.
- No use of GCC_LINT.
- The runtime cost is smaller: It still uses a stack-allocated buffer.
  Just an additional function call.


diff --git a/lib/careadlinkat.c b/lib/careadlinkat.c
index 1aa0436..67ea7d7 100644
--- a/lib/careadlinkat.c
+++ b/lib/careadlinkat.c
@@ -60,45 +60,24 @@
    If successful, return the buffer address; otherwise return NULL and
    set errno.  */
 
-char *
-careadlinkat (int fd, char const *filename,
-              char *buffer, size_t buffer_size,
-              struct allocator const *alloc,
-              ssize_t (*preadlinkat) (int, char const *, char *, size_t))
+/* A broken gcc -Wreturn-local-addr would cry wolf about returning the address
+   of a stack-allocated buffer, if this function gets inlined.  See:
+   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95044
+   When the GCC bug is fixed this workaround should be limited to the broken
+   GCC versions.  */
+#if _GL_GNUC_PREREQ (10, 1) && ! defined __clang__
+__attribute__ ((__noinline__))
+#endif
+static char *
+careadlinkat_internal (int fd, char const *filename,
+                       char *buffer, size_t buffer_size,
+                       struct allocator const *alloc,
+                       ssize_t (*preadlinkat) (int, char const *, char *, 
size_t))
 {
   char *buf;
   size_t buf_size;
   size_t buf_size_max =
     SSIZE_MAX < SIZE_MAX ? (size_t) SSIZE_MAX + 1 : SIZE_MAX;
-  char stack_buf[1024];
-
-#if (defined GCC_LINT || defined lint) && _GL_GNUC_PREREQ (10, 1)
-  /* Pacify preadlinkat without creating a pointer to the stack
-     that a broken gcc -Wreturn-local-addr would cry wolf about.  See:
-     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95044
-     This workaround differs from the mainline code, but
-     no other way to pacify GCC 10.1.0 is known; even an explicit
-     #pragma does not pacify GCC.  When the GCC bug is fixed this
-     workaround should be limited to the broken GCC versions.  */
-# define WORK_AROUND_GCC_BUG_95044
-#endif
-
-  if (! alloc)
-    alloc = &stdlib_allocator;
-
-  if (!buffer)
-    {
-#ifdef WORK_AROUND_GCC_BUG_95044
-      buffer = alloc->allocate (sizeof stack_buf);
-#else
-      /* 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().  */
-      buffer = stack_buf;
-#endif
-      buffer_size = sizeof stack_buf;
-    }
 
   buf = buffer;
   buf_size = buffer_size;
@@ -130,15 +109,6 @@ careadlinkat (int fd, char const *filename,
         {
           buf[link_size++] = '\0';
 
-          if (buf == stack_buf)
-            {
-              char *b = alloc->allocate (link_size);
-              buf_size = link_size;
-              if (! b)
-                break;
-              return memcpy (b, buf, link_size);
-            }
-
           if (link_size < buf_size && buf != buffer && alloc->reallocate)
             {
               /* Shrink BUF before returning it.  */
@@ -172,3 +142,44 @@ careadlinkat (int fd, char const *filename,
   errno = ENOMEM;
   return NULL;
 }
+
+char *
+careadlinkat (int fd, char const *filename,
+              char *buffer, size_t buffer_size,
+              struct allocator const *alloc,
+              ssize_t (*preadlinkat) (int, char const *, char *, size_t))
+{
+  char stack_buf[1024];
+
+  if (! alloc)
+    alloc = &stdlib_allocator;
+
+  if (!buffer)
+    {
+      /* 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().  */
+      buffer = stack_buf;
+      buffer_size = sizeof stack_buf;
+    }
+
+  char *buf = careadlinkat_internal (fd, filename, buffer, buffer_size, alloc,
+                                     preadlinkat);
+
+  if (buf == stack_buf)
+    {
+      size_t link_size = strlen (buf) + 1;
+      char *b = alloc->allocate (link_size);
+      if (b)
+        return memcpy (b, buf, link_size);
+      else
+        {
+          if (alloc->die)
+            alloc->die (link_size);
+          errno = ENOMEM;
+          return NULL;
+        }
+    }
+  return buf;
+}