[review] Use ctime_r and localtime_r if available

[review] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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(-)



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';
     }
 }

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

[review] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Thu, 31 Oct 2019 21:15:50 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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

Use ctime_r and localtime_r if available

To make these calls threadsafe.

gdb/ChangeLog:

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

	* config.in: Regenerate.
	* configure: Regenerate.
	* gdbsupport/common.m4: Check for ctime_r.
	* configure.ac: Check for localtime_r.
	* 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.

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/gdbsupport/common.m4
M gdb/maint.c
M gdb/nat/linux-osdata.c
8 files changed, 26 insertions(+), 6 deletions(-)



diff --git a/gdb/config.in b/gdb/config.in
index 5a21fca..9b29196 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
 
diff --git a/gdb/configure b/gdb/configure
index 512f016..f5444df 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 localtime_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"
@@ -13480,7 +13480,7 @@
 
 
   for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask strerror_r
+		  sigprocmask 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/configure.ac b/gdb/configure.ac
index 354bb7b..3d6f763 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 localtime_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
index 2984281..547a942 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
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 3f1f1c1..352836f 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -6822,7 +6822,7 @@
 
 
   for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask strerror_r
+		  sigprocmask 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/gdbsupport/common.m4 b/gdb/gdbsupport/common.m4
index 2e44cf4..81f3fec 100644
--- a/gdb/gdbsupport/common.m4
+++ b/gdb/gdbsupport/common.m4
@@ -33,7 +33,7 @@
 		   dlfcn.h)
 
   AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask strerror_r])
+		  sigprocmask strerror_r ctime_r])
 
   AC_CHECK_DECLS([strerror, strstr])
 
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';
     }
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-MessageType: newpatchset

[review] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 02:53:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Re: [review v2] Use ctime_r and localtime_r if available

[Andreas Schwab]

On Nov 02 2019, Christian Biesinger (Code Review) wrote:

> 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

buf is unused if !HAVE_CTIME_R.

Note that both ctime and ctime_r are obsolescent and should be replaced
by strftime.  gdb currently doesn't setlocale LC_TIME, but if it does it
would make the use of these functions undefined.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[review v3] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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

Use ctime_r and localtime_r if available

To make these calls threadsafe.

gdb/ChangeLog:

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

	* config.in: Regenerate.
	* configure: Regenerate.
	* gdbsupport/common.m4: Check for ctime_r.
	* configure.ac: Check for localtime_r.
	* 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.

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/gdbsupport/common.m4
M gdb/maint.c
M gdb/nat/linux-osdata.c
8 files changed, 26 insertions(+), 6 deletions(-)



diff --git a/gdb/config.in b/gdb/config.in
index 5a21fca..9b29196 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
 
diff --git a/gdb/configure b/gdb/configure
index 512f016..f5444df 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 localtime_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"
@@ -13480,7 +13480,7 @@
 
 
   for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask strerror_r
+		  sigprocmask 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/configure.ac b/gdb/configure.ac
index 354bb7b..3d6f763 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 localtime_r])
 AM_LANGINFO_CODESET
 GDB_AC_COMMON
 
diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
index 2984281..547a942 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
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index 3f1f1c1..352836f 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -6822,7 +6822,7 @@
 
 
   for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask strerror_r
+		  sigprocmask 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/gdbsupport/common.m4 b/gdb/gdbsupport/common.m4
index 2e44cf4..81f3fec 100644
--- a/gdb/gdbsupport/common.m4
+++ b/gdb/gdbsupport/common.m4
@@ -33,7 +33,7 @@
 		   dlfcn.h)
 
   AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask strerror_r])
+		  sigprocmask strerror_r ctime_r])
 
   AC_CHECK_DECLS([strerror, strstr])
 
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..9d6f86a 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);
+#ifdef HAVE_CTIME_R
+      char buf[30];
+      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';
     }
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-MessageType: newpatchset

Re: [review v2] Use ctime_r and localtime_r if available

[Christian Biesinger via gdb-patches]

On Sun, Nov 3, 2019 at 2:20 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Nov 02 2019, Christian Biesinger (Code Review) wrote:
>
> > 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
>
> buf is unused if !HAVE_CTIME_R.

Thanks, fixed.

> Note that both ctime and ctime_r are obsolescent and should be replaced
> by strftime.  gdb currently doesn't setlocale LC_TIME, but if it does it
> would make the use of these functions undefined.

I read through the manpage for ctime/ctime_r and I don't see any note
that the behavior is undefined when setlocale is called, or that it is
obsolete. For completeness, I also looked at the Solaris manpage and
it does not say that either. Instead, it will use a locale-independent
format, which seems desirable here since it is serialized into XML.

Christian

[review v3] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 20:30:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Re: [review v3] Use ctime_r and localtime_r if available

[Pedro Alves]

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

Re: [review v3] Use ctime_r and localtime_r if available

[Christian Biesinger via gdb-patches]

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

Re: [review v3] Use ctime_r and localtime_r if available

[Christian Biesinger via gdb-patches]

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

[review v3] Use ctime_r and localtime_r if available

[Kevin Buettner (Code Review)]

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


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Comment-Date: Sun, 10 Nov 2019 07:38:34 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v3] Use ctime_r and localtime_r if available

[Kevin Buettner (Code Review)]

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';
|      }
|  }
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Comment-Date: Sun, 10 Nov 2019 07:45:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v4] Use ctime_r and localtime_r if available

[Christian Biesinger (Code Review)]

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

Use ctime_r and localtime_r if available

To make these calls threadsafe.

gdb/ChangeLog:

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

	* config.in: Regenerate.
	* configure: Regenerate.
	* gdbsupport/common.m4: Check for ctime_r.
	* maint.c (scoped_command_stats::print_time): Use localtime_r
	instead of localtime (provided through gnulib if necessary).
	* 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.

Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
---
M gdb/config.in
M gdb/configure
M gdb/gdbserver/config.in
M gdb/gdbserver/configure
M gdb/gdbsupport/common.m4
M gdb/maint.c
M gdb/nat/linux-osdata.c
7 files changed, 19 insertions(+), 6 deletions(-)



diff --git a/gdb/config.in b/gdb/config.in
index fc05f15..19b18d1 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
 
diff --git a/gdb/configure b/gdb/configure
index e805903..78c0f34 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -13480,7 +13480,7 @@
 
 
   for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask
+		  sigprocmask 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/config.in b/gdb/gdbserver/config.in
index 0bce18d..efa2ea0 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
 
diff --git a/gdb/gdbserver/configure b/gdb/gdbserver/configure
index e513fc5..0414aa7 100755
--- a/gdb/gdbserver/configure
+++ b/gdb/gdbserver/configure
@@ -6822,7 +6822,7 @@
 
 
   for ac_func in fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask
+		  sigprocmask 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/gdbsupport/common.m4 b/gdb/gdbsupport/common.m4
index 471d705..d86fc92 100644
--- a/gdb/gdbsupport/common.m4
+++ b/gdb/gdbsupport/common.m4
@@ -33,7 +33,7 @@
 		   dlfcn.h)
 
   AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair sigaction \
-		  sigprocmask])
+		  sigprocmask ctime_r])
 
   AC_CHECK_DECLS([strerror, strstr])
 
diff --git a/gdb/maint.c b/gdb/maint.c
index ec9f4ab..a253584 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1039,10 +1039,11 @@
   auto millis = ticks % 1000;
 
   std::time_t as_time = system_clock::to_time_t (now);
-  struct tm *tm = localtime (&as_time);
+  struct tm tm;
+  localtime_r (&as_time, &tm);
 
   char out[100];
-  strftime (out, sizeof (out), "%F %H:%M:%S", tm);
+  strftime (out, sizeof (out), "%F %H:%M:%S", &tm);
 
   printf_unfiltered ("%s.%03d - %s\n", out, (int) millis, msg);
 }
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 84357e2..cfa245a 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -913,7 +913,13 @@
     {
       time_t t = (time_t) seconds;
 
-      strncpy (time, ctime (&t), maxlen);
+#ifdef HAVE_CTIME_R
+      char buf[30];
+      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';
     }
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 4
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-MessageType: newpatchset

[review v5] Use ctime_r and localtime_r for threadsafety

[Christian Biesinger (Code Review)]

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

Use ctime_r and localtime_r for threadsafety

To make these calls threadsafe. localtime_r is provided by gnulib if
necessary, and for ctime_r we can just use it because it is in a linux-
specific file.

gdb/ChangeLog:

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

	* maint.c (scoped_command_stats::print_time): Use localtime_r
	instead of localtime (provided through gnulib if necessary).
	* nat/linux-osdata.c (time_from_time_t): Use ctime_r instead
	of ctime.

Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
---
M gdb/maint.c
M gdb/nat/linux-osdata.c
2 files changed, 8 insertions(+), 3 deletions(-)



diff --git a/gdb/maint.c b/gdb/maint.c
index ec9f4ab..a253584 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1039,10 +1039,11 @@
   auto millis = ticks % 1000;
 
   std::time_t as_time = system_clock::to_time_t (now);
-  struct tm *tm = localtime (&as_time);
+  struct tm tm;
+  localtime_r (&as_time, &tm);
 
   char out[100];
-  strftime (out, sizeof (out), "%F %H:%M:%S", tm);
+  strftime (out, sizeof (out), "%F %H:%M:%S", &tm);
 
   printf_unfiltered ("%s.%03d - %s\n", out, (int) millis, msg);
 }
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 84357e2..a0b43c6 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -913,7 +913,11 @@
     {
       time_t t = (time_t) seconds;
 
-      strncpy (time, ctime (&t), maxlen);
+      /* Per the ctime_r manpage, this buffer needs to be at least 26
+         characters long.  */
+      char buf[30];
+      const char *time_str = ctime_r (&t, buf);
+      strncpy (time, time_str, maxlen);
       time[maxlen - 1] = '\0';
     }
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-MessageType: newpatchset

[review v5] Use ctime_r and localtime_r for threadsafety

[Christian Biesinger (Code Review)]

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.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Comment-Date: Mon, 11 Nov 2019 22:29:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v5] Use ctime_r and localtime_r for threadsafety

[Kevin Buettner (Code Review)]

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


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Comment-Date: Tue, 12 Nov 2019 20:21:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v6] Use ctime_r and localtime_r for threadsafety

[Christian Biesinger (Code Review)]

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

Use ctime_r and localtime_r for threadsafety

To make these calls threadsafe. localtime_r is provided by gnulib if
necessary, and for ctime_r we can just use it because it is in a linux-
specific file.

gdb/ChangeLog:

2019-11-15  Christian Biesinger  <cbiesinger@google.com>

	* maint.c (scoped_command_stats::print_time): Use localtime_r
	instead of localtime (provided through gnulib if necessary).
	* nat/linux-osdata.c (time_from_time_t): Use ctime_r instead
	of ctime.

Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
---
M gdb/ChangeLog
M gdb/maint.c
M gdb/nat/linux-osdata.c
3 files changed, 15 insertions(+), 3 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f31552b..f727aa4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2019-11-15  Christian Biesinger  <cbiesinger@google.com>
 
+	* maint.c (scoped_command_stats::print_time): Use localtime_r
+	instead of localtime (provided through gnulib if necessary).
+	* nat/linux-osdata.c (time_from_time_t): Use ctime_r instead
+	of ctime.
+
+2019-11-15  Christian Biesinger  <cbiesinger@google.com>
+
 	* gdbsupport/common-defs.h: Include time.h before pathmax.h to
 	avoid compile errors.
 
diff --git a/gdb/maint.c b/gdb/maint.c
index ec9f4ab..a253584 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1039,10 +1039,11 @@
   auto millis = ticks % 1000;
 
   std::time_t as_time = system_clock::to_time_t (now);
-  struct tm *tm = localtime (&as_time);
+  struct tm tm;
+  localtime_r (&as_time, &tm);
 
   char out[100];
-  strftime (out, sizeof (out), "%F %H:%M:%S", tm);
+  strftime (out, sizeof (out), "%F %H:%M:%S", &tm);
 
   printf_unfiltered ("%s.%03d - %s\n", out, (int) millis, msg);
 }
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index ca6acd3..d82c062 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -916,7 +916,11 @@
     {
       time_t t = (time_t) seconds;
 
-      strncpy (time, ctime (&t), maxlen);
+      /* Per the ctime_r manpage, this buffer needs to be at least 26
+         characters long.  */
+      char buf[30];
+      const char *time_str = ctime_r (&t, buf);
+      strncpy (time, time_str, maxlen);
       time[maxlen - 1] = '\0';
     }
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 6
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-MessageType: newpatchset

[pushed] Use ctime_r and localtime_r for threadsafety

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

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

Use ctime_r and localtime_r for threadsafety

To make these calls threadsafe. localtime_r is provided by gnulib if
necessary, and for ctime_r we can just use it because it is in a linux-
specific file.

gdb/ChangeLog:

2019-11-15  Christian Biesinger  <cbiesinger@google.com>

	* maint.c (scoped_command_stats::print_time): Use localtime_r
	instead of localtime (provided through gnulib if necessary).
	* nat/linux-osdata.c (time_from_time_t): Use ctime_r instead
	of ctime.

Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
---
M gdb/ChangeLog
M gdb/maint.c
M gdb/nat/linux-osdata.c
3 files changed, 15 insertions(+), 3 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f31552b..f727aa4 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2019-11-15  Christian Biesinger  <cbiesinger@google.com>
 
+	* maint.c (scoped_command_stats::print_time): Use localtime_r
+	instead of localtime (provided through gnulib if necessary).
+	* nat/linux-osdata.c (time_from_time_t): Use ctime_r instead
+	of ctime.
+
+2019-11-15  Christian Biesinger  <cbiesinger@google.com>
+
 	* gdbsupport/common-defs.h: Include time.h before pathmax.h to
 	avoid compile errors.
 
diff --git a/gdb/maint.c b/gdb/maint.c
index ec9f4ab..a253584 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -1039,10 +1039,11 @@
   auto millis = ticks % 1000;
 
   std::time_t as_time = system_clock::to_time_t (now);
-  struct tm *tm = localtime (&as_time);
+  struct tm tm;
+  localtime_r (&as_time, &tm);
 
   char out[100];
-  strftime (out, sizeof (out), "%F %H:%M:%S", tm);
+  strftime (out, sizeof (out), "%F %H:%M:%S", &tm);
 
   printf_unfiltered ("%s.%03d - %s\n", out, (int) millis, msg);
 }
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index ca6acd3..d82c062 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -916,7 +916,11 @@
     {
       time_t t = (time_t) seconds;
 
-      strncpy (time, ctime (&t), maxlen);
+      /* Per the ctime_r manpage, this buffer needs to be at least 26
+         characters long.  */
+      char buf[30];
+      const char *time_str = ctime_r (&t, buf);
+      strncpy (time, time_str, maxlen);
       time[maxlen - 1] = '\0';
     }
 }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 6
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-MessageType: merged

[review v6] Use ctime_r and localtime_r for threadsafety

[Christian Biesinger (Code Review)]

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';
|      }
|  }
|  

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I329bbdc39d5b576f51859ba00f1617e024c30cbd
Gerrit-Change-Number: 475
Gerrit-PatchSet: 6
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Kevin Buettner <kevinb@redhat.com>
Gerrit-Comment-Date: Sat, 16 Nov 2019 22:07:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Buettner <kevinb@redhat.com>
Gerrit-MessageType: comment