diff mbox

Default to --enable-libstdcxx-time=auto

Message ID ydd38tcpkks.fsf@lokon.CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth May 24, 2013, 1:35 p.m. UTC
Jakub Jelinek <jakub@redhat.com> writes:

>> It occured to me that there might be a far less intrusive option to still
>> allow a Solaris backport: instead of going the libstdc++.spec route
>> (which I still think is the correct way forward), statically handle -lrt
>> addition in g++spec.c, controlled by a macro defined only in config/sol2.h.
>> 
>> Such a patch could be added to mainline and 4.8 branch now, and mainline
>> later changed to use libstdc++.spec instead.
>
> I think a switch from not linking libstdc++ against -lrt to linking against
> that is quite heavy change for the branch.  Do you see why the latest patch

I'm not so sure: to me, that's just an implementation detail.

> I've posted today wouldn't work well on Solaris?

It should work on the 4.8 branch (I'll include it in my bootstraps this
weekend), but on mainline the failures due to missing nanosleep/-lrt
will remain.

I've just successfully bootstrapped the following patch on
i386-pc-solaris2.10 and x86_64-unknown-linux-gnu.  It fixes the
libstdc++ testsuite failures I'd reported before.

	Rainer


2013-05-24  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	gcc/cp:
	* g++spec.c (TIMELIB): Define.
	(WITHLIBC, SKIPOPT): Adjust values.
	(lang_specific_driver): Add TIME_LIBRARY if not passed explicitly.

	gcc:
	* config/sol2.h (TIME_LIBRARY): Define.

Comments

Jakub Jelinek May 24, 2013, 1:40 p.m. UTC | #1
On Fri, May 24, 2013 at 03:35:15PM +0200, Rainer Orth wrote:
> > I've posted today wouldn't work well on Solaris?
> 
> It should work on the 4.8 branch (I'll include it in my bootstraps this
> weekend), but on mainline the failures due to missing nanosleep/-lrt
> will remain.

Thanks.  Please make sure to use
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01456.html
rather than the earlier broken patch.

Yeah, the patch isn't meant to fix the pre-existing issues on the trunk
on Solaris, it is just about the ABI thing plus enablement to do the
real thing on Linux using syscall function; but, with the ABI change
either the =auto changes for Solaris can be safely reverted, or
your patch applied, whatever else.

Note that for 4.8.1 this is kind of urgent, because it is blocking 4.8.1-rc2
and thus also 4.8.1 release, so the sooner this gets resolved, the better.

Jonathan/Benjamin, could you please review the patch in the mean time,
so that if it works well for Rainer, it can be applied immediately and
4.8.1-rc2 rolled, even during the weekend?

	Jakub
Rainer Orth May 24, 2013, 1:51 p.m. UTC | #2
Jakub Jelinek <jakub@redhat.com> writes:

> On Fri, May 24, 2013 at 03:35:15PM +0200, Rainer Orth wrote:
>> > I've posted today wouldn't work well on Solaris?
>> 
>> It should work on the 4.8 branch (I'll include it in my bootstraps this
>> weekend), but on mainline the failures due to missing nanosleep/-lrt
>> will remain.
>
> Thanks.  Please make sure to use
> http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01456.html
> rather than the earlier broken patch.

Yep, I'd seen that.

> Yeah, the patch isn't meant to fix the pre-existing issues on the trunk
> on Solaris, it is just about the ABI thing plus enablement to do the
> real thing on Linux using syscall function; but, with the ABI change
> either the =auto changes for Solaris can be safely reverted, or
> your patch applied, whatever else.
>
> Note that for 4.8.1 this is kind of urgent, because it is blocking 4.8.1-rc2
> and thus also 4.8.1 release, so the sooner this gets resolved, the better.

I've now fired off the various Solaris bootstraps.  It may take until
tomorrow that I can report final results.

	Rainer
Jonathan Wakely May 24, 2013, 1:56 p.m. UTC | #3
On 24 May 2013 14:40, Jakub Jelinek wrote:
>
> Note that for 4.8.1 this is kind of urgent, because it is blocking 4.8.1-rc2
> and thus also 4.8.1 release, so the sooner this gets resolved, the better.

Sorry about that, I didn't realise the trunk change would affect 4.8.1

> Jonathan/Benjamin, could you please review the patch in the mean time,
> so that if it works well for Rainer, it can be applied immediately and
> 4.8.1-rc2 rolled, even during the weekend?


In the fallback for steady_clock::now() would it be easier to just
call system_clock::now() instead of duplicating its logic?

i.e .replace:

+#elif defined(_GLIBCXX_USE_CLOCK_REALTIME)
+      timespec tp;
+      // -EINVAL, -EFAULT
+      clock_gettime(CLOCK_REALTIME, &tp);
+      return time_point(duration(chrono::seconds(tp.tv_sec)
+ + chrono::nanoseconds(tp.tv_nsec)));
+#elif defined(_GLIBCXX_USE_GETTIMEOFDAY)
+      timeval tv;
+      // EINVAL, EFAULT
+      gettimeofday(&tv, 0);
+      return time_point(duration(chrono::seconds(tv.tv_sec)
+ + chrono::microseconds(tv.tv_usec)));
+#else
+      std::time_t __sec = std::time(0);
+      return system_clock::from_time_t(__sec);
 #endif

with

#else
    return system_clock::now();
#endif

I'm also wondering if

+  AC_DEFINE(_GLIBCXX_USE_CLOCK_MONOTONIC_SYSCALL, 1,
+  [ Defined if clock_gettime syscall has monotonic clock support. ])

should be renamed to _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL because it is
defined when the syscall has CLOCK_REALTIME and clock_MONOTONIC
support.

Otherwise the patch is OK.
Jakub Jelinek May 24, 2013, 2:07 p.m. UTC | #4
On Fri, May 24, 2013 at 02:56:24PM +0100, Jonathan Wakely wrote:
> > Jonathan/Benjamin, could you please review the patch in the mean time,
> > so that if it works well for Rainer, it can be applied immediately and
> > 4.8.1-rc2 rolled, even during the weekend?
> 
> 
> In the fallback for steady_clock::now() would it be easier to just
> call system_clock::now() instead of duplicating its logic?

Yeah, that is what I've tried in
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01440.html
but it failed horribly, because the return type from both the static methods
is different (system_clock::time_point (aka chrono::time_point<system_clock,
duration>) in the first case,
steady_clock::time_point (aka chrono::time_point<steady_clock, duration>)
in the latter).  And there is no conversion in between the two.

Perhaps I'd have to do something like:
  return time_point(system_clock::now().time_since_epoch());
?  Can try that.

> I'm also wondering if
> 
> +  AC_DEFINE(_GLIBCXX_USE_CLOCK_MONOTONIC_SYSCALL, 1,
> +  [ Defined if clock_gettime syscall has monotonic clock support. ])
> 
> should be renamed to _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL because it is
> defined when the syscall has CLOCK_REALTIME and clock_MONOTONIC
> support.

Yeah, that is a good suggestion, will change it.

	Jakub
Jonathan Wakely May 24, 2013, 2:15 p.m. UTC | #5
On 24 May 2013 15:07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, May 24, 2013 at 02:56:24PM +0100, Jonathan Wakely wrote:
>> > Jonathan/Benjamin, could you please review the patch in the mean time,
>> > so that if it works well for Rainer, it can be applied immediately and
>> > 4.8.1-rc2 rolled, even during the weekend?
>>
>>
>> In the fallback for steady_clock::now() would it be easier to just
>> call system_clock::now() instead of duplicating its logic?
>
> Yeah, that is what I've tried in
> http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01440.html
> but it failed horribly, because the return type from both the static methods
> is different (system_clock::time_point (aka chrono::time_point<system_clock,
> duration>) in the first case,
> steady_clock::time_point (aka chrono::time_point<steady_clock, duration>)
> in the latter).  And there is no conversion in between the two.

Ah of course, sorry.

> Perhaps I'd have to do something like:
>   return time_point(system_clock::now().time_since_epoch());
> ?  Can try that.

Yes, they share the same epoch so that should work.
Jakub Jelinek May 24, 2013, 2:16 p.m. UTC | #6
On Fri, May 24, 2013 at 04:07:48PM +0200, Jakub Jelinek wrote:
> On Fri, May 24, 2013 at 02:56:24PM +0100, Jonathan Wakely wrote:
> > > Jonathan/Benjamin, could you please review the patch in the mean time,
> > > so that if it works well for Rainer, it can be applied immediately and
> > > 4.8.1-rc2 rolled, even during the weekend?
> > 
> > 
> > In the fallback for steady_clock::now() would it be easier to just
> > call system_clock::now() instead of duplicating its logic?
> 
> Yeah, that is what I've tried in
> http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01440.html
> but it failed horribly, because the return type from both the static methods
> is different (system_clock::time_point (aka chrono::time_point<system_clock,
> duration>) in the first case,
> steady_clock::time_point (aka chrono::time_point<steady_clock, duration>)
> in the latter).  And there is no conversion in between the two.
> 
> Perhaps I'd have to do something like:
>   return time_point(system_clock::now().time_since_epoch());
> ?  Can try that.

Ah, but the duration can be different depending on macros.

	Jakub
Jonathan Wakely May 24, 2013, 2:29 p.m. UTC | #7
On 24 May 2013 15:16, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, May 24, 2013 at 04:07:48PM +0200, Jakub Jelinek wrote:
>> On Fri, May 24, 2013 at 02:56:24PM +0100, Jonathan Wakely wrote:
>> > > Jonathan/Benjamin, could you please review the patch in the mean time,
>> > > so that if it works well for Rainer, it can be applied immediately and
>> > > 4.8.1-rc2 rolled, even during the weekend?
>> >
>> >
>> > In the fallback for steady_clock::now() would it be easier to just
>> > call system_clock::now() instead of duplicating its logic?
>>
>> Yeah, that is what I've tried in
>> http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01440.html
>> but it failed horribly, because the return type from both the static methods
>> is different (system_clock::time_point (aka chrono::time_point<system_clock,
>> duration>) in the first case,
>> steady_clock::time_point (aka chrono::time_point<steady_clock, duration>)
>> in the latter).  And there is no conversion in between the two.
>>
>> Perhaps I'd have to do something like:
>>   return time_point(system_clock::now().time_since_epoch());
>> ?  Can try that.
>
> Ah, but the duration can be different depending on macros.

You can convert between durations: with duration_cast:

return time_point(duration_cast<duration>(system_clock::now().time_since_epoch()));
diff mbox

Patch

# HG changeset patch
# Parent d6881ec042d3a6328b763cbf0f38e61bdbb64d79
Add -lrt on Solaris

diff --git a/gcc/config/sol2.h b/gcc/config/sol2.h
--- a/gcc/config/sol2.h
+++ b/gcc/config/sol2.h
@@ -194,6 +194,9 @@  along with GCC; see the file COPYING3.  
 #endif /* HAVE_LD_EH_FRAME && TARGET_DL_ITERATE_PHDR */
 #endif
 
+/* C++11 programs need -lrt for nanosleep.  */
+#define TIME_LIBRARY "rt"
+
 #ifndef USE_GLD
 /* The default MFLIB_SPEC is GNU ld specific.  */
 #define MFLIB_SPEC ""
diff --git a/gcc/cp/g++spec.c b/gcc/cp/g++spec.c
--- a/gcc/cp/g++spec.c
+++ b/gcc/cp/g++spec.c
@@ -28,10 +28,12 @@  along with GCC; see the file COPYING3.  
 #define LANGSPEC	(1<<1)
 /* This bit is set if they did `-lm' or `-lmath'.  */
 #define MATHLIB		(1<<2)
+/* This bit is set if they did `-lrt' or equivalent.  */
+#define TIMELIB		(1<<3)
 /* This bit is set if they did `-lc'.  */
-#define WITHLIBC	(1<<3)
+#define WITHLIBC	(1<<4)
 /* Skip this option.  */
-#define SKIPOPT		(1<<4)
+#define SKIPOPT		(1<<5)
 
 #ifndef MATH_LIBRARY
 #define MATH_LIBRARY "m"
@@ -40,6 +42,10 @@  along with GCC; see the file COPYING3.  
 #define MATH_LIBRARY_PROFILE MATH_LIBRARY
 #endif
 
+#ifndef TIME_LIBRARY
+#define TIME_LIBRARY ""
+#endif
+
 #ifndef LIBSTDCXX
 #define LIBSTDCXX "stdc++"
 #endif
@@ -83,16 +89,22 @@  lang_specific_driver (struct cl_decoded_
   /* "-lm" or "-lmath" if it appears on the command line.  */
   const struct cl_decoded_option *saw_math = NULL;
 
+  /* "-lrt" or eqivalent if it appears on the command line.  */
+  const struct cl_decoded_option *saw_time = NULL;
+
   /* "-lc" if it appears on the command line.  */
   const struct cl_decoded_option *saw_libc = NULL;
 
   /* An array used to flag each argument that needs a bit set for
-     LANGSPEC, MATHLIB, or WITHLIBC.  */
+     LANGSPEC, MATHLIB, TIMELIB, or WITHLIBC.  */
   int *args;
 
   /* By default, we throw on the math library if we have one.  */
   int need_math = (MATH_LIBRARY[0] != '\0');
 
+  /* By default, we throw on the time library if we have one.  */
+  int need_time = (TIME_LIBRARY[0] != '\0');
+
   /* True if we saw -static.  */
   int static_link = 0;
 
@@ -136,6 +148,11 @@  lang_specific_driver (struct cl_decoded_
 	      args[i] |= MATHLIB;
 	      need_math = 0;
 	    }
+	  else if (strcmp (arg, TIME_LIBRARY) == 0)
+	    {
+	      args[i] |= TIMELIB;
+	      need_time = 0;
+	    }
 	  else if (strcmp (arg, "c") == 0)
 	    args[i] |= WITHLIBC;
 	  else
@@ -268,6 +285,12 @@  lang_specific_driver (struct cl_decoded_
 	  saw_math = &decoded_options[i];
 	}
 
+      if (!saw_time && (args[i] & TIMELIB) && library > 0)
+	{
+	  --j;
+	  saw_time = &decoded_options[i];
+	}
+
       if (!saw_libc && (args[i] & WITHLIBC) && library > 0)
 	{
 	  --j;
@@ -352,6 +375,15 @@  lang_specific_driver (struct cl_decoded_
       added_libraries++;
       j++;
     }
+  if (saw_time)
+    new_decoded_options[j++] = *saw_time;
+  else if (library > 0 && need_time)
+    {
+      generate_option (OPT_l, TIME_LIBRARY, 1, CL_DRIVER,
+		       &new_decoded_options[j]);
+      added_libraries++;
+      j++;
+    }
   if (saw_libc)
     new_decoded_options[j++] = *saw_libc;
   if (shared_libgcc && !static_link)