Replace the remaining uses of strerror with safe_strerror
Commit Message
To do that, this patch makes IPA compile safe-strerror as well. Because
it doesn't use Gnulib, it calls the Glibc version of strerror_r directly.
As I understand it, IPA only needs to work on Linux, so this should be safe.
Consequently this patch also removes the configure checks for strerror.
Depends on https://sourceware.org/ml/gdb-patches/2019-11/msg00912.html
gdb/ChangeLog:
2019-11-26 Christian Biesinger <cbiesinger@google.com>
* config.in: Regenerate.
* configure: Regenerate.
* gdbsupport/agent.c (gdb_connect_sync_socket): Call
safe_strerror instead of strerror.
* gdbsupport/common.m4: Don't check for strerror.
* gdbsupport/safe-strerror.c: If IN_PROCESS_AGENT is defined,
call the glibc version of strerror_r.
gdb/gdbserver/ChangeLog:
2019-11-26 Christian Biesinger <cbiesinger@google.com>
* Makefile.in: Add safe-strerror.c to gdbreplay and IPA.
* config.in: Regenerate.
* configure: Regenerate.
* configure.ac: Don't check for strerror.
* linux-i386-ipa.c (initialize_fast_tracepoint_trampoline_buffer):
Call safe_strerror instead of strerror.
* server.h (strerror): Remove this now-unnecessary declaration.
* tracepoint.c (init_named_socket): Call safe_strerror instead of
strerror.
(gdb_agent_helper_thread): Likewise.
* utils.c (perror_with_name): Likewise.
Change-Id: I74848f072dcde75cb55c435ef9398dc8f958cd73
---
gdb/config.in | 4 ----
gdb/configure | 12 +-----------
gdb/gdbserver/Makefile.in | 2 ++
gdb/gdbserver/config.in | 4 ----
gdb/gdbserver/configure | 22 +---------------------
gdb/gdbserver/configure.ac | 2 +-
gdb/gdbserver/linux-i386-ipa.c | 4 ++--
gdb/gdbserver/server.h | 6 ------
gdb/gdbserver/tracepoint.c | 12 ++++++------
gdb/gdbserver/utils.c | 2 +-
gdb/gdbsupport/agent.c | 4 ++--
gdb/gdbsupport/common.m4 | 2 +-
gdb/gdbsupport/safe-strerror.c | 7 +++++++
13 files changed, 24 insertions(+), 59 deletions(-)
Comments
On 11/26/19 7:59 PM, Christian Biesinger via gdb-patches wrote:
> index c37db579f7..724a1d24db 100644
> --- a/gdb/gdbsupport/safe-strerror.c
> +++ b/gdb/gdbsupport/safe-strerror.c
> @@ -27,11 +27,18 @@ safe_strerror (int errnum)
> {
> static thread_local char buf[1024];
>
> +#ifdef IN_PROCESS_AGENT
> + /* IPA does not use Gnulib, but only supports Linux, so we can safely
> + call the GNU version of strerror_r here. It is documented not to
> + return NULL. */
> + return strerror_r (errnum, buf, sizeof (buf));
GDBserver and (I assume) the IPA can be built with other C runtimes on Linux,
like musl and others. Do you know how musl behaves?
Thanks,
Pedro Alves
On Fri, Dec 6, 2019 at 2:06 PM Pedro Alves <palves@redhat.com> wrote:
>
> On 11/26/19 7:59 PM, Christian Biesinger via gdb-patches wrote:
>
> > index c37db579f7..724a1d24db 100644
> > --- a/gdb/gdbsupport/safe-strerror.c
> > +++ b/gdb/gdbsupport/safe-strerror.c
> > @@ -27,11 +27,18 @@ safe_strerror (int errnum)
> > {
> > static thread_local char buf[1024];
> >
> > +#ifdef IN_PROCESS_AGENT
> > + /* IPA does not use Gnulib, but only supports Linux, so we can safely
> > + call the GNU version of strerror_r here. It is documented not to
> > + return NULL. */
> > + return strerror_r (errnum, buf, sizeof (buf));
>
> GDBserver and (I assume) the IPA can be built with other C runtimes on Linux,
> like musl and others. Do you know how musl behaves?
Oof, looks like they return int :(
http://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c
I guess I'll change to #if defined(IN_PROCESS_AGENT) && defined(__GLIBC__)?
Christian
On 12/6/19 8:36 PM, Christian Biesinger wrote:
> On Fri, Dec 6, 2019 at 2:06 PM Pedro Alves <palves@redhat.com> wrote:
>> GDBserver and (I assume) the IPA can be built with other C runtimes on Linux,
>> like musl and others. Do you know how musl behaves?
>
> Oof, looks like they return int :(
> http://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c
>
> I guess I'll change to #if defined(IN_PROCESS_AGENT) && defined(__GLIBC__)?
>
Crazy thought --- use C++ overload resolution to pick the right thing
automatically:
/* Called if we have a XSI-compliant strerror_r. */
static char *select_strerror_r (int, char *buf) { return buf; }
/* Called if we have a GNU strerror_r. */
static char *select_strerror_r (char *res, char *) { return res; }
#ifdef IN_PROCESS_AGENT
return select_strerror_r (strerror_r (errnum, buf, buflen), buf);
#else ...
Thanks,
Pedro Alves
On Fri, Dec 6, 2019 at 4:46 PM Pedro Alves <palves@redhat.com> wrote:
>
> On 12/6/19 8:36 PM, Christian Biesinger wrote:
> > On Fri, Dec 6, 2019 at 2:06 PM Pedro Alves <palves@redhat.com> wrote:
>
> >> GDBserver and (I assume) the IPA can be built with other C runtimes on Linux,
> >> like musl and others. Do you know how musl behaves?
> >
> > Oof, looks like they return int :(
> > http://git.musl-libc.org/cgit/musl/tree/src/string/strerror_r.c
> >
> > I guess I'll change to #if defined(IN_PROCESS_AGENT) && defined(__GLIBC__)?
> >
>
> Crazy thought --- use C++ overload resolution to pick the right thing
> automatically:
>
> /* Called if we have a XSI-compliant strerror_r. */
> static char *select_strerror_r (int, char *buf) { return buf; }
>
> /* Called if we have a GNU strerror_r. */
> static char *select_strerror_r (char *res, char *) { return res; }
>
> #ifdef IN_PROCESS_AGENT
> return select_strerror_r (strerror_r (errnum, buf, buflen), buf);
> #else ...
Ah, great idea. That way I don't even need the #ifdef. Done, will send
a new version now.
Christian
@@ -114,10 +114,6 @@
don't. */
#undef HAVE_DECL_SNPRINTF
-/* Define to 1 if you have the declaration of `strerror', and to 0 if you
- don't. */
-#undef HAVE_DECL_STRERROR
-
/* Define to 1 if you have the declaration of `strstr', and to 0 if you don't.
*/
#undef HAVE_DECL_STRSTR
@@ -13489,17 +13489,7 @@ fi
done
- ac_fn_c_check_decl "$LINENO" "strerror" "ac_cv_have_decl_strerror" "$ac_includes_default"
-if test "x$ac_cv_have_decl_strerror" = xyes; then :
- ac_have_decl=1
-else
- ac_have_decl=0
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define HAVE_DECL_STRERROR $ac_have_decl
-_ACEOF
-ac_fn_c_check_decl "$LINENO" "strstr" "ac_cv_have_decl_strstr" "$ac_includes_default"
+ ac_fn_c_check_decl "$LINENO" "strstr" "ac_cv_have_decl_strstr" "$ac_includes_default"
if test "x$ac_cv_have_decl_strstr" = xyes; then :
ac_have_decl=1
else
@@ -301,6 +301,7 @@ GDBREPLAY_OBS = \
gdbsupport/errors.o \
gdbsupport/netstuff.o \
gdbsupport/print-utils.o \
+ gdbsupport/safe-strerror.o \
gdbreplay.o \
utils.o \
version.o
@@ -427,6 +428,7 @@ IPA_OBJS = \
gdbsupport/format-ipa.o \
gdbsupport/print-utils-ipa.o \
gdbsupport/rsp-low-ipa.o \
+ gdbsupport/safe-strerror-ipa.o \
gdbsupport/tdesc-ipa.o \
regcache-ipa.o \
remote-utils-ipa.o \
@@ -51,10 +51,6 @@
don't. */
#undef HAVE_DECL_SNPRINTF
-/* Define to 1 if you have the declaration of `strerror', and to 0 if you
- don't. */
-#undef HAVE_DECL_STRERROR
-
/* Define to 1 if you have the declaration of `strstr', and to 0 if you don't.
*/
#undef HAVE_DECL_STRSTR
@@ -6835,17 +6835,7 @@ fi
done
- ac_fn_c_check_decl "$LINENO" "strerror" "ac_cv_have_decl_strerror" "$ac_includes_default"
-if test "x$ac_cv_have_decl_strerror" = xyes; then :
- ac_have_decl=1
-else
- ac_have_decl=0
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define HAVE_DECL_STRERROR $ac_have_decl
-_ACEOF
-ac_fn_c_check_decl "$LINENO" "strstr" "ac_cv_have_decl_strstr" "$ac_includes_default"
+ ac_fn_c_check_decl "$LINENO" "strstr" "ac_cv_have_decl_strstr" "$ac_includes_default"
if test "x$ac_cv_have_decl_strstr" = xyes; then :
ac_have_decl=1
else
@@ -7531,16 +7521,6 @@ _ACEOF
-ac_fn_c_check_decl "$LINENO" "strerror" "ac_cv_have_decl_strerror" "$ac_includes_default"
-if test "x$ac_cv_have_decl_strerror" = xyes; then :
- ac_have_decl=1
-else
- ac_have_decl=0
-fi
-
-cat >>confdefs.h <<_ACEOF
-#define HAVE_DECL_STRERROR $ac_have_decl
-_ACEOF
ac_fn_c_check_decl "$LINENO" "perror" "ac_cv_have_decl_perror" "$ac_includes_default"
if test "x$ac_cv_have_decl_perror" = xyes; then :
ac_have_decl=1
@@ -158,7 +158,7 @@ LIBS="$old_LIBS"
libiberty_INIT
-AC_CHECK_DECLS([strerror, perror, vasprintf, vsnprintf])
+AC_CHECK_DECLS([perror, vasprintf, vsnprintf])
AC_CHECK_MEMBERS([struct stat.st_blocks, struct stat.st_blksize])
@@ -210,7 +210,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
if (!f)
{
snprintf (buf, sizeof (buf), "mmap_min_addr open failed: %s",
- strerror (errno));
+ safe_strerror (errno));
set_trampoline_buffer_space (0, 0, buf);
return;
}
@@ -233,7 +233,7 @@ initialize_fast_tracepoint_trampoline_buffer (void)
else
{
snprintf (buf, IPA_BUFSIZ, "low-64K-buffer mmap() failed: %s",
- strerror (errno));
+ safe_strerror (errno));
set_trampoline_buffer_space (0, 0, buf);
}
}
@@ -29,12 +29,6 @@ gdb_static_assert (sizeof (CORE_ADDR) >= sizeof (void *));
#include "gdbsupport/version.h"
-#if !HAVE_DECL_STRERROR
-#ifndef strerror
-extern char *strerror (int); /* X3.159-1989 4.11.6.2 */
-#endif
-#endif
-
#if !HAVE_DECL_PERROR
#ifndef perror
extern void perror (const char *);
@@ -6879,7 +6879,7 @@ init_named_socket (const char *name)
result = fd = socket (PF_UNIX, SOCK_STREAM, 0);
if (result == -1)
{
- warning ("socket creation failed: %s", strerror (errno));
+ warning ("socket creation failed: %s", safe_strerror (errno));
return -1;
}
@@ -6895,7 +6895,7 @@ init_named_socket (const char *name)
result = unlink (name);
if (result == -1)
{
- warning ("unlink failed: %s", strerror (errno));
+ warning ("unlink failed: %s", safe_strerror (errno));
close (fd);
return -1;
}
@@ -6905,7 +6905,7 @@ init_named_socket (const char *name)
result = bind (fd, (struct sockaddr *) &addr, sizeof (addr));
if (result == -1)
{
- warning ("bind failed: %s", strerror (errno));
+ warning ("bind failed: %s", safe_strerror (errno));
close (fd);
return -1;
}
@@ -6913,7 +6913,7 @@ init_named_socket (const char *name)
result = listen (fd, 1);
if (result == -1)
{
- warning ("listen: %s", strerror (errno));
+ warning ("listen: %s", safe_strerror (errno));
close (fd);
return -1;
}
@@ -7219,7 +7219,7 @@ gdb_agent_helper_thread (void *arg)
if (fd < 0)
{
warning ("Accept returned %d, error: %s",
- fd, strerror (errno));
+ fd, safe_strerror (errno));
break;
}
@@ -7231,7 +7231,7 @@ gdb_agent_helper_thread (void *arg)
if (ret == -1)
{
warning ("reading socket (fd=%d) failed with %s",
- fd, strerror (errno));
+ fd, safe_strerror (errno));
close (fd);
break;
}
@@ -47,7 +47,7 @@ perror_with_name (const char *string)
const char *err;
char *combined;
- err = strerror (errno);
+ err = safe_strerror (errno);
if (err == NULL)
err = "unknown error";
@@ -149,7 +149,7 @@ gdb_connect_sync_socket (int pid)
res = fd = gdb_socket_cloexec (PF_UNIX, SOCK_STREAM, 0);
if (res == -1)
{
- warning (_("error opening sync socket: %s"), strerror (errno));
+ warning (_("error opening sync socket: %s"), safe_strerror (errno));
return -1;
}
@@ -168,7 +168,7 @@ gdb_connect_sync_socket (int pid)
{
warning (_("error connecting sync socket (%s): %s. "
"Make sure the directory exists and that it is writable."),
- path, strerror (errno));
+ path, safe_strerror (errno));
close (fd);
return -1;
}
@@ -35,7 +35,7 @@ AC_DEFUN([GDB_AC_COMMON], [
AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair sigaction \
sigprocmask])
- AC_CHECK_DECLS([strerror, strstr])
+ AC_CHECK_DECLS([strstr])
dnl Check if sigsetjmp is available. Using AC_CHECK_FUNCS won't
dnl do since sigsetjmp might only be defined as a macro.
@@ -27,11 +27,18 @@ safe_strerror (int errnum)
{
static thread_local char buf[1024];
+#ifdef IN_PROCESS_AGENT
+ /* IPA does not use Gnulib, but only supports Linux, so we can safely
+ call the GNU version of strerror_r here. It is documented not to
+ return NULL. */
+ return strerror_r (errnum, buf, sizeof (buf));
+#else
/* Assign the return value to an int, so we get an error if we accidentally
get the wrong version of this function (glibc has two of them...). */
int ret = strerror_r (errnum, buf, sizeof (buf));
if (ret == 0)
return buf;
+#endif
xsnprintf (buf, sizeof buf, "(undocumented errno %d)", errnum);
return buf;