[review] Use strerror_r in safe_strerror if available

Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed

* [review] Use strerror_r in safe_strerror if available
@ 2019-10-31 19:44 Christian Biesinger (Code Review)
  2019-10-31 19:53 ` Simon Marchi (Code Review)
                   ` (10 more replies)
  0 siblings, 11 replies; 13+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-31 19:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Christian Biesinger

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................

Use strerror_r in safe_strerror if available

Also stores the result in a thread-local static variable.

This is already important because Guile creates threads and
Python can create threads, but with the patch series here:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
GDB itself will create threads, too.

gdb/ChangeLog:

2019-10-31  Christian Biesinger  <cbiesinger@google.com>

	* configure: Regenerate.
	* configure.ac: Check for strerror_r.
	* gdbsupport/posix-strerror.c (safe_strerror): Make buf
	thread_local and call strerror_r, if available.

Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
---
M gdb/configure
M gdb/configure.ac
M gdb/gdbsupport/posix-strerror.c
3 files changed, 15 insertions(+), 5 deletions(-)



diff --git a/gdb/configure b/gdb/configure
index e805903..018cc4b 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -13073,7 +13073,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors
+		ptrace64 sigaltstack setns use_default_colors strerror_r
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 354bb7b..987507a 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1318,7 +1318,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors])
+		ptrace64 sigaltstack setns use_default_colors strerror_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/gdbsupport/posix-strerror.c b/gdb/gdbsupport/posix-strerror.c
index a8651b7..727eb7b 100644
--- a/gdb/gdbsupport/posix-strerror.c
+++ b/gdb/gdbsupport/posix-strerror.c
@@ -24,12 +24,22 @@
 char *
 safe_strerror (int errnum)
 {
-  char *msg;
+  static thread_local char buf[1024];
 
+  char *msg = nullptr;
+#ifdef HAVE_STRERROR_R
+#if !__GLIBC__ || ((_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE)
+  /* Glibc has two different, incompatible versions of strerror_r.  */
+  if (strerror_r (errnum, buf, sizeof (buf)) == 0)
+    msg = buf;
+#else
+  msg = strerror_r (errnum, buf, sizeof (buf));
+#endif
+#else
   msg = strerror (errnum);
-  if (msg == NULL)
+#endif
+  if (msg == nullptr)
     {
-      static char buf[32];
 
       xsnprintf (buf, sizeof buf, "(undocumented errno %d)", errnum);
       msg = buf;

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-MessageType: newchange


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
@ 2019-10-31 19:53 ` Simon Marchi (Code Review)
  2019-10-31 20:05 ` [review v2] " Christian Biesinger (Code Review)
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Simon Marchi (Code Review) @ 2019-10-31 19:53 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 1:

(2 comments)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/1/gdb/gdbsupport/posix-strerror.c 
File gdb/gdbsupport/posix-strerror.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/1/gdb/gdbsupport/posix-strerror.c@24 
PS1, Line 24: 
19 | 
20 | #include "common-defs.h"
21 | 
22 | /* Implementation of safe_strerror as defined in common-utils.h.  */
23 | 
24 > char *
25 | safe_strerror (int errnum)
26 | {
27 |   static thread_local char buf[1024];
28 | 
29 |   char *msg = nullptr;

Just wondering, maybe we could make safe_strerror return a `const char *`?  I suppose we never want the caller to modify what we return, since it could point to some immutable string.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/1/gdb/gdbsupport/posix-strerror.c@31 
PS1, Line 31: 
26 | {
27 |   static thread_local char buf[1024];
28 | 
29 |   char *msg = nullptr;
30 | #ifdef HAVE_STRERROR_R
31 > #if !__GLIBC__ || ((_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE)
32 |   /* Glibc has two different, incompatible versions of strerror_r.  */
33 |   if (strerror_r (errnum, buf, sizeof (buf)) == 0)
34 |     msg = buf;
35 | #else
36 |   msg = strerror_r (errnum, buf, sizeof (buf));

Just a nit, can you "indent" the preprocessor declarations, like

 #ifdef HAVE_STRERROR_R
 # if ...

?  I think it makes it easier to read.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 31 Oct 2019 19:53:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v2] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
  2019-10-31 19:53 ` Simon Marchi (Code Review)
@ 2019-10-31 20:05 ` Christian Biesinger (Code Review)
  2019-10-31 20:05 ` Christian Biesinger (Code Review)
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-31 20:05 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches; +Cc: Simon Marchi

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Uploaded patch set 2.

(2 comments)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/1/gdb/gdbsupport/posix-strerror.c 
File gdb/gdbsupport/posix-strerror.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/1/gdb/gdbsupport/posix-strerror.c@24 
PS1, Line 24: 
19 | 
20 | #include "common-defs.h"
21 | 
22 | /* Implementation of safe_strerror as defined in common-utils.h.  */
23 | 
24 > char *
25 | safe_strerror (int errnum)
26 | {
27 |   static thread_local char buf[1024];
28 | 
29 |   char *msg = nullptr;

> Just wondering, maybe we could make safe_strerror return a `const char *`?  I suppose we never want  [â€¦]

Done, only two callers needed changes.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/1/gdb/gdbsupport/posix-strerror.c@31 
PS1, Line 31: 
26 | {
27 |   static thread_local char buf[1024];
28 | 
29 |   char *msg = nullptr;
30 | #ifdef HAVE_STRERROR_R
31 > #if !__GLIBC__ || ((_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE)
32 |   /* Glibc has two different, incompatible versions of strerror_r.  */
33 |   if (strerror_r (errnum, buf, sizeof (buf)) == 0)
34 |     msg = buf;
35 | #else
36 |   msg = strerror_r (errnum, buf, sizeof (buf));

> Just a nit, can you "indent" the preprocessor declarations, like [â€¦]

Good idea, done.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 31 Oct 2019 20:05:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v2] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
  2019-10-31 19:53 ` Simon Marchi (Code Review)
  2019-10-31 20:05 ` [review v2] " Christian Biesinger (Code Review)
@ 2019-10-31 20:05 ` Christian Biesinger (Code Review)
  2019-10-31 20:10 ` Simon Marchi (Code Review)
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-31 20:05 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches; +Cc: Simon Marchi

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................

Use strerror_r in safe_strerror if available

Also stores the result in a thread-local static variable and
changes the return value to a const char*.

This is already important because Guile creates threads and
Python can create threads, but with the patch series here:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
GDB itself will create threads, too.

gdb/ChangeLog:

2019-10-31  Christian Biesinger  <cbiesinger@google.com>

	* configure: Regenerate.
	* configure.ac: Check for strerror_r.
	* gdbsupport/common-utils.h (safe_strerror): Change return value
	to const char * and document that this function is now threadsafe.
	* gdbsupport/posix-strerror.c (safe_strerror): Make buf
	thread_local and call strerror_r, if available.
	* utils.c (perror_string): Update.
	(print_sys_errmsg): Update.

Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
---
M gdb/configure
M gdb/configure.ac
M gdb/gdbsupport/common-utils.h
M gdb/gdbsupport/posix-strerror.c
M gdb/utils.c
5 files changed, 21 insertions(+), 16 deletions(-)



diff --git a/gdb/configure b/gdb/configure
index e805903..018cc4b 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -13073,7 +13073,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors
+		ptrace64 sigaltstack setns use_default_colors strerror_r
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 354bb7b..987507a 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1318,7 +1318,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors])
+		ptrace64 sigaltstack setns use_default_colors strerror_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/gdbsupport/common-utils.h b/gdb/gdbsupport/common-utils.h
index 23bf354..89868fc 100644
--- a/gdb/gdbsupport/common-utils.h
+++ b/gdb/gdbsupport/common-utils.h
@@ -108,9 +108,9 @@
 
 /* The strerror() function can return NULL for errno values that are
    out of range.  Provide a "safe" version that always returns a
-   printable string.  */
+   printable string.  This version is also thread-safe.  */
 
-extern char *safe_strerror (int);
+extern const char *safe_strerror (int);
 
 /* Return true if the start of STRING matches PATTERN, false otherwise.  */
 
diff --git a/gdb/gdbsupport/posix-strerror.c b/gdb/gdbsupport/posix-strerror.c
index a8651b7..34420cf 100644
--- a/gdb/gdbsupport/posix-strerror.c
+++ b/gdb/gdbsupport/posix-strerror.c
@@ -21,15 +21,25 @@
 
 /* Implementation of safe_strerror as defined in common-utils.h.  */
 
-char *
+const char *
 safe_strerror (int errnum)
 {
-  char *msg;
+  static thread_local char buf[1024];
 
+  char *msg = nullptr;
+#ifdef HAVE_STRERROR_R
+#  if !__GLIBC__ || ((_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE)
+  /* Glibc has two different, incompatible versions of strerror_r.  */
+  if (strerror_r (errnum, buf, sizeof (buf)) == 0)
+    msg = buf;
+#  else
+  msg = strerror_r (errnum, buf, sizeof (buf));
+#  endif
+#else
   msg = strerror (errnum);
-  if (msg == NULL)
+#endif
+  if (msg == nullptr)
     {
-      static char buf[32];
 
       xsnprintf (buf, sizeof buf, "(undocumented errno %d)", errnum);
       msg = buf;
diff --git a/gdb/utils.c b/gdb/utils.c
index 0c133d1..e06eedd 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -582,9 +582,7 @@
 static std::string
 perror_string (const char *prefix)
 {
-  char *err;
-
-  err = safe_strerror (errno);
+  const char *err = safe_strerror (errno);
   return std::string (prefix) + ": " + err;
 }
 
@@ -630,11 +628,8 @@
 void
 print_sys_errmsg (const char *string, int errcode)
 {
-  char *err;
-  char *combined;
-
-  err = safe_strerror (errcode);
-  combined = (char *) alloca (strlen (err) + strlen (string) + 3);
+  const char *err = safe_strerror (errcode);
+  char *combined = (char *) alloca (strlen (err) + strlen (string) + 3);
   strcpy (combined, string);
   strcat (combined, ": ");
   strcat (combined, err);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v2] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (2 preceding siblings ...)
  2019-10-31 20:05 ` Christian Biesinger (Code Review)
@ 2019-10-31 20:10 ` Simon Marchi (Code Review)
  2019-10-31 20:10 ` Simon Marchi (Code Review)
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Simon Marchi (Code Review) @ 2019-10-31 20:10 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 2: Code-Review+2


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 31 Oct 2019 20:10:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v2] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (3 preceding siblings ...)
  2019-10-31 20:10 ` Simon Marchi (Code Review)
@ 2019-10-31 20:10 ` Simon Marchi (Code Review)
  2019-10-31 20:15 ` [pushed] " Sourceware to Gerrit sync (Code Review)
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Simon Marchi (Code Review) @ 2019-10-31 20:10 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 2:

Thanks!


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 31 Oct 2019 20:10:18 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [pushed] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (4 preceding siblings ...)
  2019-10-31 20:10 ` Simon Marchi (Code Review)
@ 2019-10-31 20:15 ` Sourceware to Gerrit sync (Code Review)
  2019-10-31 20:15 ` Sourceware to Gerrit sync (Code Review)
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-10-31 20:15 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches; +Cc: Simon Marchi

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................

Use strerror_r in safe_strerror if available

Also stores the result in a thread-local static variable and
changes the return value to a const char*.

This is already important because Guile creates threads and
Python can create threads, but with the patch series here:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
GDB itself will create threads, too.

gdb/ChangeLog:

2019-10-31  Christian Biesinger  <cbiesinger@google.com>

	* configure: Regenerate.
	* configure.ac: Check for strerror_r.
	* gdbsupport/common-utils.h (safe_strerror): Change return value
	to const char * and document that this function is now threadsafe.
	* gdbsupport/posix-strerror.c (safe_strerror): Make buf
	thread_local and call strerror_r, if available.
	* utils.c (perror_string): Update.
	(print_sys_errmsg): Update.

Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
---
M gdb/ChangeLog
M gdb/configure
M gdb/configure.ac
M gdb/gdbsupport/common-utils.h
M gdb/gdbsupport/posix-strerror.c
M gdb/utils.c
6 files changed, 32 insertions(+), 16 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7abe20e..b7a7d98 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2019-10-31  Christian Biesinger  <cbiesinger@google.com>
+
+	* configure: Regenerate.
+	* configure.ac: Check for strerror_r.
+	* gdbsupport/common-utils.h (safe_strerror): Change return value
+	to const char * and document that this function is now threadsafe.
+	* gdbsupport/posix-strerror.c (safe_strerror): Make buf
+	thread_local and call strerror_r, if available.
+	* utils.c (perror_string): Update.
+	(print_sys_errmsg): Update.
+
 2019-10-31  Luis Machado  <luis.machado@linaro.org>
 
 	* arm-tdep.c (arm_exidx_data_key): Use bfd_key instead of
diff --git a/gdb/configure b/gdb/configure
index e805903..018cc4b 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -13073,7 +13073,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors
+		ptrace64 sigaltstack setns use_default_colors strerror_r
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 354bb7b..987507a 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1318,7 +1318,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors])
+		ptrace64 sigaltstack setns use_default_colors strerror_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/gdbsupport/common-utils.h b/gdb/gdbsupport/common-utils.h
index 23bf354..89868fc 100644
--- a/gdb/gdbsupport/common-utils.h
+++ b/gdb/gdbsupport/common-utils.h
@@ -108,9 +108,9 @@
 
 /* The strerror() function can return NULL for errno values that are
    out of range.  Provide a "safe" version that always returns a
-   printable string.  */
+   printable string.  This version is also thread-safe.  */
 
-extern char *safe_strerror (int);
+extern const char *safe_strerror (int);
 
 /* Return true if the start of STRING matches PATTERN, false otherwise.  */
 
diff --git a/gdb/gdbsupport/posix-strerror.c b/gdb/gdbsupport/posix-strerror.c
index a8651b7..34420cf 100644
--- a/gdb/gdbsupport/posix-strerror.c
+++ b/gdb/gdbsupport/posix-strerror.c
@@ -21,15 +21,25 @@
 
 /* Implementation of safe_strerror as defined in common-utils.h.  */
 
-char *
+const char *
 safe_strerror (int errnum)
 {
-  char *msg;
+  static thread_local char buf[1024];
 
+  char *msg = nullptr;
+#ifdef HAVE_STRERROR_R
+#  if !__GLIBC__ || ((_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE)
+  /* Glibc has two different, incompatible versions of strerror_r.  */
+  if (strerror_r (errnum, buf, sizeof (buf)) == 0)
+    msg = buf;
+#  else
+  msg = strerror_r (errnum, buf, sizeof (buf));
+#  endif
+#else
   msg = strerror (errnum);
-  if (msg == NULL)
+#endif
+  if (msg == nullptr)
     {
-      static char buf[32];
 
       xsnprintf (buf, sizeof buf, "(undocumented errno %d)", errnum);
       msg = buf;
diff --git a/gdb/utils.c b/gdb/utils.c
index 0c133d1..e06eedd 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -582,9 +582,7 @@
 static std::string
 perror_string (const char *prefix)
 {
-  char *err;
-
-  err = safe_strerror (errno);
+  const char *err = safe_strerror (errno);
   return std::string (prefix) + ": " + err;
 }
 
@@ -630,11 +628,8 @@
 void
 print_sys_errmsg (const char *string, int errcode)
 {
-  char *err;
-  char *combined;
-
-  err = safe_strerror (errcode);
-  combined = (char *) alloca (strlen (err) + strlen (string) + 3);
+  const char *err = safe_strerror (errcode);
+  char *combined = (char *) alloca (strlen (err) + strlen (string) + 3);
   strcpy (combined, string);
   strcat (combined, ": ");
   strcat (combined, err);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: merged


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [pushed] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (5 preceding siblings ...)
  2019-10-31 20:15 ` [pushed] " Sourceware to Gerrit sync (Code Review)
@ 2019-10-31 20:15 ` Sourceware to Gerrit sync (Code Review)
  2019-11-01 15:21 ` [review v3] " Hannes Domani (Code Review)
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-10-31 20:15 UTC (permalink / raw)
  To: Christian Biesinger, Simon Marchi, gdb-patches

The original change was created by Christian Biesinger.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................

Use strerror_r in safe_strerror if available

Also stores the result in a thread-local static variable and
changes the return value to a const char*.

This is already important because Guile creates threads and
Python can create threads, but with the patch series here:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/176
GDB itself will create threads, too.

gdb/ChangeLog:

2019-10-31  Christian Biesinger  <cbiesinger@google.com>

	* configure: Regenerate.
	* configure.ac: Check for strerror_r.
	* gdbsupport/common-utils.h (safe_strerror): Change return value
	to const char * and document that this function is now threadsafe.
	* gdbsupport/posix-strerror.c (safe_strerror): Make buf
	thread_local and call strerror_r, if available.
	* utils.c (perror_string): Update.
	(print_sys_errmsg): Update.

Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
---
M gdb/ChangeLog
M gdb/configure
M gdb/configure.ac
M gdb/gdbsupport/common-utils.h
M gdb/gdbsupport/posix-strerror.c
M gdb/utils.c
6 files changed, 32 insertions(+), 16 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 7abe20e..b7a7d98 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2019-10-31  Christian Biesinger  <cbiesinger@google.com>
+
+	* configure: Regenerate.
+	* configure.ac: Check for strerror_r.
+	* gdbsupport/common-utils.h (safe_strerror): Change return value
+	to const char * and document that this function is now threadsafe.
+	* gdbsupport/posix-strerror.c (safe_strerror): Make buf
+	thread_local and call strerror_r, if available.
+	* utils.c (perror_string): Update.
+	(print_sys_errmsg): Update.
+
 2019-10-31  Luis Machado  <luis.machado@linaro.org>
 
 	* arm-tdep.c (arm_exidx_data_key): Use bfd_key instead of
diff --git a/gdb/configure b/gdb/configure
index e805903..018cc4b 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -13073,7 +13073,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors
+		ptrace64 sigaltstack setns use_default_colors strerror_r
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 354bb7b..987507a 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1318,7 +1318,7 @@
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
 		setrlimit getrlimit posix_madvise waitpid \
-		ptrace64 sigaltstack setns use_default_colors])
+		ptrace64 sigaltstack setns use_default_colors strerror_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/gdbsupport/common-utils.h b/gdb/gdbsupport/common-utils.h
index 23bf354..89868fc 100644
--- a/gdb/gdbsupport/common-utils.h
+++ b/gdb/gdbsupport/common-utils.h
@@ -108,9 +108,9 @@
 
 /* The strerror() function can return NULL for errno values that are
    out of range.  Provide a "safe" version that always returns a
-   printable string.  */
+   printable string.  This version is also thread-safe.  */
 
-extern char *safe_strerror (int);
+extern const char *safe_strerror (int);
 
 /* Return true if the start of STRING matches PATTERN, false otherwise.  */
 
diff --git a/gdb/gdbsupport/posix-strerror.c b/gdb/gdbsupport/posix-strerror.c
index a8651b7..34420cf 100644
--- a/gdb/gdbsupport/posix-strerror.c
+++ b/gdb/gdbsupport/posix-strerror.c
@@ -21,15 +21,25 @@
 
 /* Implementation of safe_strerror as defined in common-utils.h.  */
 
-char *
+const char *
 safe_strerror (int errnum)
 {
-  char *msg;
+  static thread_local char buf[1024];
 
+  char *msg = nullptr;
+#ifdef HAVE_STRERROR_R
+#  if !__GLIBC__ || ((_POSIX_C_SOURCE >= 200112L) && !  _GNU_SOURCE)
+  /* Glibc has two different, incompatible versions of strerror_r.  */
+  if (strerror_r (errnum, buf, sizeof (buf)) == 0)
+    msg = buf;
+#  else
+  msg = strerror_r (errnum, buf, sizeof (buf));
+#  endif
+#else
   msg = strerror (errnum);
-  if (msg == NULL)
+#endif
+  if (msg == nullptr)
     {
-      static char buf[32];
 
       xsnprintf (buf, sizeof buf, "(undocumented errno %d)", errnum);
       msg = buf;
diff --git a/gdb/utils.c b/gdb/utils.c
index 0c133d1..e06eedd 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -582,9 +582,7 @@
 static std::string
 perror_string (const char *prefix)
 {
-  char *err;
-
-  err = safe_strerror (errno);
+  const char *err = safe_strerror (errno);
   return std::string (prefix) + ": " + err;
 }
 
@@ -630,11 +628,8 @@
 void
 print_sys_errmsg (const char *string, int errcode)
 {
-  char *err;
-  char *combined;
-
-  err = safe_strerror (errcode);
-  combined = (char *) alloca (strlen (err) + strlen (string) + 3);
+  const char *err = safe_strerror (errcode);
+  char *combined = (char *) alloca (strlen (err) + strlen (string) + 3);
   strcpy (combined, string);
   strcat (combined, ": ");
   strcat (combined, err);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v3] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (6 preceding siblings ...)
  2019-10-31 20:15 ` Sourceware to Gerrit sync (Code Review)
@ 2019-11-01 15:21 ` Hannes Domani (Code Review)
  2019-11-01 15:22 ` Christian Biesinger (Code Review)
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 13+ messages in thread
From: Hannes Domani (Code Review) @ 2019-11-01 15:21 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches; +Cc: Simon Marchi

Hannes Domani has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 3:

(1 comment)

> Patch Set 1:
> 
> (2 comments)

New build failure for windows.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/common-utils.h 
File gdb/gdbsupport/common-utils.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/common-utils.h@113 
PS3, Line 113: 
108 | 
109 | /* The strerror() function can return NULL for errno values that are
110 |    out of range.  Provide a "safe" version that always returns a
111 |    printable string.  This version is also thread-safe.  */
112 | 
113 > extern const char *safe_strerror (int);
114 | 
115 | /* Return true if the start of STRING matches PATTERN, false otherwise.  */
116 | 
117 | static inline bool
118 | startswith (const char *string, const char *pattern)

The windows build now fails, because safe_strerror() of mingw-strerror.c doesn't match this prototype anymore:

C:/src/repos/binutils-gdb.git/gdb/gdbsupport/mingw-strerror.c:32:1: error: ambiguating new declaration of 'char* safe_strerror(int)'
   32 | safe_strerror (int errnum)
      | ^~~~~~~~~~~~~
In file included from C:/src/repos/binutils-gdb.git/gdb/gdbsupport/common-defs.h:122,
                 from C:/src/repos/binutils-gdb.git/gdb/gdbsupport/mingw-strerror.c:20:
C:/src/repos/binutils-gdb.git/gdb/gdbsupport/common-utils.h:113:20: note: old declaration 'const char* safe_strerror(int)'
  113 | extern const char *safe_strerror (int);
      |                    ^~~~~~~~~~~~~



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Hannes Domani <ssbssa@yahoo.de>
Gerrit-Comment-Date: Fri, 01 Nov 2019 15:21:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v3] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (7 preceding siblings ...)
  2019-11-01 15:21 ` [review v3] " Hannes Domani (Code Review)
@ 2019-11-01 15:22 ` Christian Biesinger (Code Review)
  2019-11-01 17:10   ` Tom Tromey
  2019-11-06  1:08 ` Pedro Alves (Code Review)
  2019-11-06 19:37 ` Christian Biesinger (Code Review)
  10 siblings, 1 reply; 13+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-11-01 15:22 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey, Hannes Domani, Simon Marchi

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 3:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/common-utils.h 
File gdb/gdbsupport/common-utils.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/common-utils.h@113 
PS3, Line 113: 
108 | 
109 | /* The strerror() function can return NULL for errno values that are
110 |    out of range.  Provide a "safe" version that always returns a
111 |    printable string.  This version is also thread-safe.  */
112 | 
113 > extern const char *safe_strerror (int);
114 | 
115 | /* Return true if the start of STRING matches PATTERN, false otherwise.  */
116 | 
117 | static inline bool
118 | startswith (const char *string, const char *pattern)

> The windows build now fails, because safe_strerror() of mingw-strerror. [â€¦]

Thanks; tromey has a patch for that at https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/478



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Hannes Domani <ssbssa@yahoo.de>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 01 Nov 2019 15:22:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hannes Domani <ssbssa@yahoo.de>
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [review v3] Use strerror_r in safe_strerror if available
  2019-11-01 15:22 ` Christian Biesinger (Code Review)
@ 2019-11-01 17:10   ` Tom Tromey
  0 siblings, 0 replies; 13+ messages in thread
From: Tom Tromey @ 2019-11-01 17:10 UTC (permalink / raw)
  To: Christian Biesinger (Code Review)
  Cc: Christian Biesinger, gdb-patches, gnutoolchain-gerrit,
	Tom Tromey, Hannes Domani, Simon Marchi

>>>>> "Christian" == Christian Biesinger (Code Review) <gerrit@gnutoolchain-gerrit.osci.io> writes:

>> The windows build now fails, because safe_strerror() of mingw-strerror. […]

Christian> Thanks; tromey has a patch for that at https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/478

I pushed it a little while ago.

Tom


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v3] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (8 preceding siblings ...)
  2019-11-01 15:22 ` Christian Biesinger (Code Review)
@ 2019-11-06  1:08 ` Pedro Alves (Code Review)
  2019-11-06 19:37 ` Christian Biesinger (Code Review)
  10 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves (Code Review) @ 2019-11-06  1:08 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey, Hannes Domani, Simon Marchi

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 3:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/posix-strerror.c 
File gdb/gdbsupport/posix-strerror.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/posix-strerror.c@37 
PS3, Line 37: 
32 |   /* Glibc has two different, incompatible versions of strerror_r.  */
33 |   if (strerror_r (errnum, buf, sizeof (buf)) == 0)
34 |     msg = buf;
35 | #  else
36 |   msg = strerror_r (errnum, buf, sizeof (buf));
37 > #  endif
38 | #else
39 |   msg = strerror (errnum);
40 | #endif
41 |   if (msg == nullptr)
42 |     {

Any reason we don't use strerror_r from gnulib instead?

https://www.gnu.org/software/gnulib/manual/html_node/strerror_005fr.html

We don't import that module currently, but I'd assume there'd be no blocker.

I looked at the lib/strerror_r.c in gnulib's sources, and checked that it handles Windows' socket error codes too.  Though it does it differently.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Hannes Domani <ssbssa@yahoo.de>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Wed, 06 Nov 2019 01:08:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [review v3] Use strerror_r in safe_strerror if available
  2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
                   ` (9 preceding siblings ...)
  2019-11-06  1:08 ` Pedro Alves (Code Review)
@ 2019-11-06 19:37 ` Christian Biesinger (Code Review)
  10 siblings, 0 replies; 13+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-11-06 19:37 UTC (permalink / raw)
  To: Christian Biesinger, gdb-patches
  Cc: Pedro Alves, Tom Tromey, Hannes Domani, Simon Marchi

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474
......................................................................


Patch Set 3:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/posix-strerror.c 
File gdb/gdbsupport/posix-strerror.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/474/3/gdb/gdbsupport/posix-strerror.c@37 
PS3, Line 37: 
32 |   /* Glibc has two different, incompatible versions of strerror_r.  */
33 |   if (strerror_r (errnum, buf, sizeof (buf)) == 0)
34 |     msg = buf;
35 | #  else
36 |   msg = strerror_r (errnum, buf, sizeof (buf));
37 > #  endif
38 | #else
39 |   msg = strerror (errnum);
40 | #endif
41 |   if (msg == nullptr)
42 |     {

> Any reason we don't use strerror_r from gnulib instead? [â€¦]

Thanks for the suggestion, uploaded a patch for that at https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/514



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I81048fbaf148035c221c528727f7efe58ba528eb
Gerrit-Change-Number: 474
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-CC: Hannes Domani <ssbssa@yahoo.de>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Wed, 06 Nov 2019 19:36:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-11-06 19:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 19:44 [review] Use strerror_r in safe_strerror if available Christian Biesinger (Code Review)
2019-10-31 19:53 ` Simon Marchi (Code Review)
2019-10-31 20:05 ` [review v2] " Christian Biesinger (Code Review)
2019-10-31 20:05 ` Christian Biesinger (Code Review)
2019-10-31 20:10 ` Simon Marchi (Code Review)
2019-10-31 20:10 ` Simon Marchi (Code Review)
2019-10-31 20:15 ` [pushed] " Sourceware to Gerrit sync (Code Review)
2019-10-31 20:15 ` Sourceware to Gerrit sync (Code Review)
2019-11-01 15:21 ` [review v3] " Hannes Domani (Code Review)
2019-11-01 15:22 ` Christian Biesinger (Code Review)
2019-11-01 17:10   ` Tom Tromey
2019-11-06  1:08 ` Pedro Alves (Code Review)
2019-11-06 19:37 ` Christian Biesinger (Code Review)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox