diff mbox

[review] Use ctime_r and localtime_r if available

Message ID gerrit.1572555982000.I329bbdc39d5b576f51859ba00f1617e024c30cbd@gnutoolchain-gerrit.osci.io
State New
Headers show

Commit Message

Simon Marchi (Code Review) Oct. 31, 2019, 9:06 p.m. UTC
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/475
......................................................................

Use ctime_r and localtime_r if available

This requires definining _POSIX_C_SOURCE for glibc. I don't know if there
are any undesired side-effects from that.

I am also adding the strerror_r check to gdbserver's configure.ac
that I previously forgot (but gdbserver doesn't use threads as
much, so that's less critical)

gdb/ChangeLog:

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

	* config.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Check for localtime_r and ctime_r, and define
	_POSIX_C_SOURCE so that we actually find them.
	* maint.c (scoped_command_stats::print_time): Use localtime_r
	when available instead of localtime.
	* nat/linux-osdata.c (time_from_time_t): Use ctime_r if available
	instead of ctime.

gdb/gdbserver/ChangeLog:

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

	* config.in: Regenerate.
	* configure: Regenerate.
	* configure.ac: Define _POSIX_C_SOURCE and search for strerror_r
	and ctime_r.

Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
---
M gdb/config.in
M gdb/configure
M gdb/configure.ac
M gdb/gdbserver/config.in
M gdb/gdbserver/configure
M gdb/gdbserver/configure.ac
M gdb/maint.c
M gdb/nat/linux-osdata.c
8 files changed, 42 insertions(+), 5 deletions(-)

Comments

Simon Marchi (Code Review) Oct. 31, 2019, 9:15 p.m. UTC | #1
Christian Biesinger has posted comments on this change.

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


Patch Set 1:

OK, this fails on Solaris:
https://gdb-buildbot.osci.io/#builders/11/builds/1069

I am not planning to work further on this. Someone more familiar with autoconf and/or Solaris can pick this up.
Simon Marchi (Code Review) Nov. 3, 2019, 2:53 a.m. UTC | #2
Christian Biesinger has posted comments on this change.

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


Patch Set 1:

I must've tested this incorrectly before; the previous version was overly complicated. Will upload a new version in a second that works fine.
Simon Marchi (Code Review) Nov. 6, 2019, 8:30 p.m. UTC | #3
Christian Biesinger has posted comments on this change.

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


Patch Set 3:

I looked at gnulib's time_r module but it turns out that its localtime_r implementation is not in any way threadsafe.
Pedro Alves Nov. 8, 2019, 2:08 p.m. UTC | #4
On 11/6/19 8:30 PM, Christian Biesinger (Code Review) wrote:
> Patch Set 3:
> 
> I looked at gnulib's time_r module but it turns out that its localtime_r implementation is not in any way threadsafe.

It does seemingly look that way:

 struct tm *
 localtime_r (time_t const * restrict t, struct tm * restrict tp)
 {
   return copy_tm_result (tp, localtime (t));
 }

But that doesn't seem worse than the #ifdef fallback that you're leaving
in place.  I'd argue that it'd be better to use the gnulib version.

But read on, please.  Looking at:

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

Of the problems that are relevant on our supported hosts, we see:

Portability problems fixed by Gnulib:

    This function is missing on some platforms: mingw, MSVC 14. 

And, note that localtime, like other C functions that use global
state, are thread safe on Windows, because the C run time stores
the global buffers in thread local storage.  So the gnulib
implementation ends up being thread safe there, even though
it doesn't look like it is.

Thanks,
Pedro Alves
Doug Evans via gdb-patches Nov. 8, 2019, 5:10 p.m. UTC | #5
On Fri, Nov 8, 2019 at 8:08 AM Pedro Alves <palves@redhat.com> wrote:
> And, note that localtime, like other C functions that use global
> state, are thread safe on Windows, because the C run time stores
> the global buffers in thread local storage.  So the gnulib
> implementation ends up being thread safe there, even though
> it doesn't look like it is.

OK, I'll change this to use the time_r module.

I'll make it depend on
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/514 since that
one also imports a new module.

Christian
Doug Evans via gdb-patches Nov. 9, 2019, 8:17 p.m. UTC | #6
On Fri, Nov 8, 2019 at 11:10 AM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> On Fri, Nov 8, 2019 at 8:08 AM Pedro Alves <palves@redhat.com> wrote:
> > And, note that localtime, like other C functions that use global
> > state, are thread safe on Windows, because the C run time stores
> > the global buffers in thread local storage.  So the gnulib
> > implementation ends up being thread safe there, even though
> > it doesn't look like it is.
>
> OK, I'll change this to use the time_r module.
>
> I'll make it depend on
> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/514 since that
> one also imports a new module.

So I tried gnulib's time_r module but it failed to compile on the
mingw buildbot:
https://gdb-buildbot.osci.io/#builders/23/builds/934

Any ideas?
Christian
Simon Marchi (Code Review) Nov. 10, 2019, 7:38 a.m. UTC | #7
Kevin Buettner has posted comments on this change.

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


Patch Set 3: Code-Review+2
Simon Marchi (Code Review) Nov. 10, 2019, 7:45 a.m. UTC | #8
Kevin Buettner has posted comments on this change.

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


Patch Set 3:

(1 comment)

> Patch Set 3: Code-Review+2

Only one small nit in gdb/nat/linux-osdata.c - I suggest adding a comment. Aside from that, it looks good to me.

| --- gdb/nat/linux-osdata.c
| +++ gdb/nat/linux-osdata.c
| @@ -908,12 +908,18 @@ time_from_time_t (char *time, int maxlen, TIME_T seconds)
|  {
|    if (!seconds)
|      time[0] = '\0';
|    else
|      {
|        time_t t = (time_t) seconds;
|  
| -      strncpy (time, ctime (&t), maxlen);
| +#ifdef HAVE_CTIME_R
| +      char buf[30];

PS3, Line 916:

Perhaps add a comment here stating that the ctime_r man page states
that the user supplied buffer needs to be 26 bytes or larger?

| +      const char *time_str = ctime_r (&t, buf);
| +#else
| +      const char *time_str = ctime (&t);
| +#endif
| +      strncpy (time, time_str, maxlen);
|        time[maxlen - 1] = '\0';
|      }
|  }
|
Simon Marchi (Code Review) Nov. 11, 2019, 10:29 p.m. UTC | #9
Christian Biesinger has posted comments on this change.

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


Patch Set 5:

Hi Kevin,

thanks for the review! I've simplified this now to rely on gnulib for localtime_r. I also realized that we can just unconditionally use ctime_r because it's a linux-specific like. Could you take another look (also on the dependencies)?

I did add a comment as you suggested.
Simon Marchi (Code Review) Nov. 12, 2019, 8:21 p.m. UTC | #10
Kevin Buettner has posted comments on this change.

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


Patch Set 5: Code-Review+2
Simon Marchi (Code Review) Nov. 16, 2019, 10:07 p.m. UTC | #11
Christian Biesinger has posted comments on this change.

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


Patch Set 6:

(1 comment)

| --- gdb/nat/linux-osdata.c
| +++ gdb/nat/linux-osdata.c
| @@ -908,12 +908,18 @@ time_from_time_t (char *time, int maxlen, TIME_T seconds)
|  {
|    if (!seconds)
|      time[0] = '\0';
|    else
|      {
|        time_t t = (time_t) seconds;
|  
| -      strncpy (time, ctime (&t), maxlen);
| +#ifdef HAVE_CTIME_R
| +      char buf[30];

PS3, Line 916:

Done.

| +      const char *time_str = ctime_r (&t, buf);
| +#else
| +      const char *time_str = ctime (&t);
| +#endif
| +      strncpy (time, time_str, maxlen);
|        time[maxlen - 1] = '\0';
|      }
|  }
|
diff mbox

Patch

diff --git a/gdb/config.in b/gdb/config.in
index 5a21fca..a676b47 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -78,6 +78,9 @@ 
 /* Define to 1 if you have the `btowc' function. */
 #undef HAVE_BTOWC
 
+/* Define to 1 if you have the `ctime_r' function. */
+#undef HAVE_CTIME_R
+
 /* Define to 1 if you have the <cursesX.h> header file. */
 #undef HAVE_CURSESX_H
 
@@ -261,6 +264,9 @@ 
 /* Define to 1 if you have the <locale.h> header file. */
 #undef HAVE_LOCALE_H
 
+/* Define to 1 if you have the `localtime_r' function. */
+#undef HAVE_LOCALTIME_R
+
 /* Define to 1 if the compiler supports long double. */
 #undef HAVE_LONG_DOUBLE
 
@@ -780,6 +786,9 @@ 
    this defined. */
 #undef _POSIX_1_SOURCE
 
+/* We want the latest POSIX standard */
+#undef _POSIX_C_SOURCE
+
 /* Define to 1 if you need to in order for `stat' and other things to work. */
 #undef _POSIX_SOURCE
 
diff --git a/gdb/configure b/gdb/configure
index 018cc4b..42cfb5b 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -4408,6 +4408,9 @@ 
   $as_echo "#define _TANDEM_SOURCE 1" >>confdefs.h
 
 
+
+$as_echo "#define _POSIX_C_SOURCE 200809L" >>confdefs.h
+
 ac_aux_dir=
 for ac_dir in "$srcdir" "$srcdir/.." "$srcdir/../.."; do
   if test -f "$ac_dir/install-sh"; then
@@ -13072,7 +13075,7 @@ 
 		sbrk getpgid setpgid setpgrp setsid \
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
-		setrlimit getrlimit posix_madvise waitpid \
+		setrlimit getrlimit posix_madvise waitpid ctime_r localtime_r \
 		ptrace64 sigaltstack setns use_default_colors strerror_r
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 987507a..6a6ec5b 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -29,6 +29,7 @@ 
 AC_PROG_CXX
 
 AC_USE_SYSTEM_EXTENSIONS
+AC_DEFINE([_POSIX_C_SOURCE], [200809L], [We want the latest POSIX standard])
 ACX_LARGEFILE
 AM_PROG_CC_STDC
 AM_PROG_INSTALL_STRIP
@@ -1317,7 +1318,7 @@ 
 		sbrk getpgid setpgid setpgrp setsid \
 		sigaction sigsetmask socketpair \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
-		setrlimit getrlimit posix_madvise waitpid \
+		setrlimit getrlimit posix_madvise waitpid ctime_r localtime_r \
 		ptrace64 sigaltstack setns use_default_colors strerror_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
index 0bce18d..78fbf26 100644
--- a/gdb/gdbserver/config.in
+++ b/gdb/gdbserver/config.in
@@ -21,6 +21,9 @@ 
 /* Define to 1 if you have the <arpa/inet.h> header file. */
 #undef HAVE_ARPA_INET_H
 
+/* Define to 1 if you have the `ctime_r' function. */
+#undef HAVE_CTIME_R
+
 /* define if the compiler supports basic C++11 syntax */
 #undef HAVE_CXX11
 
@@ -229,6 +232,9 @@ 
 /* Define to 1 if you have the <stdlib.h> header file. */
 #undef HAVE_STDLIB_H
 
+/* Define to 1 if you have the `strerror_r' function. */
+#undef HAVE_STRERROR_R
+
 /* Define to 1 if you have the <strings.h> header file. */
 #undef HAVE_STRINGS_H
 
@@ -420,6 +426,9 @@ 
    this defined. */
 #undef _POSIX_1_SOURCE
 
+/* We want the latest POSIX standard */
+#undef _POSIX_C_SOURCE
+
 /* Define to 1 if you need to in order for `stat' and other things to work. */
 #undef _POSIX_SOURCE
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index e513fc5..8dd13b0 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -4094,6 +4094,9 @@ 
 
 
 
+
+$as_echo "#define _POSIX_C_SOURCE 200809L" >>confdefs.h
+
 # Check whether --enable-largefile was given.
 if test "${enable_largefile+set}" = set; then :
   enableval=$enable_largefile;
@@ -6448,7 +6451,7 @@ 
 
 fi
 
-for ac_func in getauxval pread pwrite pread64 setns
+for ac_func in getauxval pread pwrite pread64 setns strerror_r ctime_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/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
index 7ebc9c3..972a17d 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -26,6 +26,7 @@ 
 AC_PROG_CC
 AC_PROG_CXX
 AC_GNU_SOURCE
+AC_DEFINE([_POSIX_C_SOURCE], [200809L], [We want the latest POSIX standard])
 AC_SYS_LARGEFILE
 
 AC_CANONICAL_SYSTEM
@@ -90,7 +91,7 @@ 
 		 sys/ioctl.h netinet/in.h sys/socket.h netdb.h dnl
 		 netinet/tcp.h arpa/inet.h)
 AC_FUNC_FORK
-AC_CHECK_FUNCS(getauxval pread pwrite pread64 setns)
+AC_CHECK_FUNCS(getauxval pread pwrite pread64 setns strerror_r ctime_r)
 
 GDB_AC_COMMON
 
diff --git a/gdb/maint.c b/gdb/maint.c
index ec9f4ab..36ab716 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1039,7 +1039,12 @@ 
   auto millis = ticks % 1000;
 
   std::time_t as_time = system_clock::to_time_t (now);
+#ifdef HAVE_LOCALTIME_R
+  struct tm buf;
+  struct tm *tm = localtime_r (&as_time, &buf);
+#else
   struct tm *tm = localtime (&as_time);
+#endif
 
   char out[100];
   strftime (out, sizeof (out), "%F %H:%M:%S", tm);
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 67f9f3a..e0bad81 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -912,7 +912,13 @@ 
     {
       time_t t = (time_t) seconds;
 
-      strncpy (time, ctime (&t), maxlen);
+      char buf[30];
+#ifdef HAVE_CTIME_R
+      const char *time_str = ctime_r (&t, buf);
+#else
+      const char *time_str = ctime (&t);
+#endif
+      strncpy (time, time_str, maxlen);
       time[maxlen - 1] = '\0';
     }
 }