>From 2a3468c9f263596815a3383c0157ba9a81cf2d24 Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 + 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 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() - - 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() + + 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