On 8/17/20 10:10 AM, Bruno Haible wrote:
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.
Those people can build and ship with CFLAGS='-DGCC_LINT'. This may hurt
performance on their platforms a bit, but if they're willing to pay that price
to pacify bogus warnings then that's OK. Conversely, if they want to avoid the
minor performance hits without generating this particular bogus warning, they
can build and ship with CFLAGS='-Wno-return-local-addr'.
How about this modification? It splits the function into two functions, and
unless the first function is inlined, the warning disappears.
Thanks, that's a good idea for GCC 10 since it lets the code allocate the first
buffer on the stack too, even if warnings are enabled. However, I would rather
have the code use __attribute__ ((__noinline__)) only if GCC_LINT is defined.
The point of GCC_LINT is to make it clear to readers how code is written a
particular way merely to pacify bogus compiler warnings. I'd rather do this
systematically, rather than use GCC_LINT in some places where the problem occurs
and not use it in others. With that in mind, I installed the attached patch.
>From 2a3468c9f263596815a3383c0157ba9a81cf2d24 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 17 Aug 2020 12:39:48 -0700
Subject: [PATCH] careadlinkat: speedup for GCC 10 with GCC_LINT
Inspired by a suggestion by Bruno Haible in:
https://lists.gnu.org/r/bug-gnulib/2020-08/msg00155.html
* lib/careadlinkat.c (STACK_BUF_SIZE): New constant.
(readlink_stk): New function, with most of the old careadlinkat
contents and with a new STACK_BUF arg. Inline it in GCC 10
if GCC_LINT.
(careadlinkat): Use the new function for everything but the
stack buffer.
---
ChangeLog | 10 +++++
lib/careadlinkat.c | 108 ++++++++++++++++++++++++++-------------------
2 files changed, 72 insertions(+), 46 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index a5cd6e316..cd00997a9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,15 @@
2020-08-17 Paul Eggert <egg...@cs.ucla.edu>
+ careadlinkat: speedup for GCC 10 with GCC_LINT
+ Inspired by a suggestion by Bruno Haible in:
+ https://lists.gnu.org/r/bug-gnulib/2020-08/msg00155.html
+ * lib/careadlinkat.c (STACK_BUF_SIZE): New constant.
+ (readlink_stk): New function, with most of the old careadlinkat
+ contents and with a new STACK_BUF arg. Inline it in GCC 10
+ if GCC_LINT.
+ (careadlinkat): Use the new function for everything but the
+ stack buffer.
+
* build-aux/gcc-warning.spec: Update comments.
2020-08-17 Bruno Haible <br...@clisp.org>
diff --git a/lib/careadlinkat.c b/lib/careadlinkat.c
index 1aa04363d..e43aa42d5 100644
--- a/lib/careadlinkat.c
+++ b/lib/careadlinkat.c
@@ -38,66 +38,41 @@
#include "allocator.h"
-/* Assuming the current directory is FD, get the symbolic link value
- of FILENAME as a null-terminated string and put it into a buffer.
- If FD is AT_FDCWD, FILENAME is interpreted relative to the current
- working directory, as in openat.
-
- If the link is small enough to fit into BUFFER put it there.
- BUFFER's size is BUFFER_SIZE, and BUFFER can be null
- if BUFFER_SIZE is zero.
-
- If the link is not small, put it into a dynamically allocated
- buffer managed by ALLOC. It is the caller's responsibility to free
- the returned value if it is nonnull and is not BUFFER. A null
- ALLOC stands for the standard allocator.
-
- The PREADLINKAT function specifies how to read links. It operates
- like POSIX readlinkat()
- <https://pubs.opengroup.org/onlinepubs/9699919799/functions/readlink.html>
- but can assume that its first argument is the same as FD.
-
- If successful, return the buffer address; otherwise return NULL and
- set errno. */
-
-char *
-careadlinkat (int fd, char const *filename,
+enum { STACK_BUF_SIZE = 1024 };
+
+/* Act like careadlinkat (see below), with an additional argument
+ STACK_BUF that can be used as temporary storage.
+
+ If GCC_LINT is defined, do not inline this function with GCC 10.1
+ and later, to avoid creating a pointer to the stack that GCC
+ -Wreturn-local-addr incorrectly complains about. See:
+ https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644
+ Although the noinline attribute can hurt performance a bit, no better way
+ to pacify GCC 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. */
+#if (defined GCC_LINT || defined lint) && _GL_GNUC_PREREQ (10, 1)
+__attribute__ ((__noinline__))
+#endif
+static char *
+readlink_stk (int fd, char const *filename,
char *buffer, size_t buffer_size,
struct allocator const *alloc,
- ssize_t (*preadlinkat) (int, char const *, char *, size_t))
+ ssize_t (*preadlinkat) (int, char const *, char *, size_t),
+ char stack_buf[STACK_BUF_SIZE])
{
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;
+ buffer_size = STACK_BUF_SIZE;
}
buf = buffer;
@@ -172,3 +147,44 @@ careadlinkat (int fd, char const *filename,
errno = ENOMEM;
return NULL;
}
+
+
+/* Assuming the current directory is FD, get the symbolic link value
+ of FILENAME as a null-terminated string and put it into a buffer.
+ If FD is AT_FDCWD, FILENAME is interpreted relative to the current
+ working directory, as in openat.
+
+ If the link is small enough to fit into BUFFER put it there.
+ BUFFER's size is BUFFER_SIZE, and BUFFER can be null
+ if BUFFER_SIZE is zero.
+
+ If the link is not small, put it into a dynamically allocated
+ buffer managed by ALLOC. It is the caller's responsibility to free
+ the returned value if it is nonnull and is not BUFFER. A null
+ ALLOC stands for the standard allocator.
+
+ The PREADLINKAT function specifies how to read links. It operates
+ like POSIX readlinkat()
+ <https://pubs.opengroup.org/onlinepubs/9699919799/functions/readlink.html>
+ but can assume that its first argument is the same as FD.
+
+ 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))
+{
+ /* 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.
+
+ If GCC -Wreturn-local-addr warns about this buffer, the warning
+ is bogus; see readlink_stk. */
+ char stack_buf[STACK_BUF_SIZE];
+ return readlink_stk (fd, filename, buffer, buffer_size, alloc,
+ preadlinkat, stack_buf);
+}
--
2.17.1
