[review] Use ctime_r and localtime_r if available

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

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.
  

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.
  

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.
  

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
  

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
  

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
  

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
  

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

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.
  

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
  

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