Patchwork revised fix for nanosleep check in GLIBCXX_ENABLE_LIBSTDCXX_TIME for darwin

login
register
mail settings
Submitter Jack Howarth
Date Oct. 9, 2012, 12:39 a.m.
Message ID <20121009003921.GA25678@bromo.med.uc.edu>
Download mbox | patch
Permalink /patch/190167/
State New
Headers show

Comments

Jack Howarth - Oct. 9, 2012, 12:39 a.m.
The --enable-libstdcxx-time=yes configure option fails to validate the
presence of a usable nanosleep() call on darwin due to its use of pre-2008
POSIX timers. As both nanosleep() and sched_yield() have always been available
on darwin, the attached patch simply defines _GLIBCXX_USE_NANOSLEEP and
_GLIBCXX_USE_SCHED_YIELD in config/os/bsd/darwin/os_defines.h. This also has
the advantage of effectively making --enable-libstdcxx-time=yes the default
on darwin. Regression tested on x86_64-apple-darwin12.
   Okay for gcc trunk as well as gcc-4_7-branch/gcc-4_6-branch/gcc-4_5-branch?
                Jack

libstdc++-v3/

2012-10-08  Jack Howarth  <howarth@bromo.med.uc.edu>

	PR libstdc++/54847
	* config/os/bsd/darwin/os_defines.h (_GLIBCXX_USE_NANOSLEEP): Define.
	(_GLIBCXX_USE_SCHED_YIELD): Likewise.
Jonathan Wakely - Oct. 9, 2012, 8:21 a.m.
On 9 October 2012 01:39, Jack Howarth wrote:
>    The --enable-libstdcxx-time=yes configure option fails to validate the
> presence of a usable nanosleep() call on darwin due to its use of pre-2008
> POSIX timers. As both nanosleep() and sched_yield() have always been available
> on darwin, the attached patch simply defines _GLIBCXX_USE_NANOSLEEP and
> _GLIBCXX_USE_SCHED_YIELD in config/os/bsd/darwin/os_defines.h. This also has
> the advantage of effectively making --enable-libstdcxx-time=yes the default
> on darwin. Regression tested on x86_64-apple-darwin12.
>    Okay for gcc trunk as well as gcc-4_7-branch/gcc-4_6-branch/gcc-4_5-branch?

The 4.5 branch is closed, so definitely not alright there.

I don't like the sched_yield macro being set there because it's
detected correctly by configure anyway, but I'm not going to labour
that point any more.

OK for trunk, it's not a regression so I'm not sure about the
branches. If it doesn't cause problems on the trunk we can decide
whether to apply it to the 4.7 branch later.
Jack Howarth - Oct. 9, 2012, 1:11 p.m.
On Tue, Oct 09, 2012 at 09:21:25AM +0100, Jonathan Wakely wrote:
> On 9 October 2012 01:39, Jack Howarth wrote:
> >    The --enable-libstdcxx-time=yes configure option fails to validate the
> > presence of a usable nanosleep() call on darwin due to its use of pre-2008
> > POSIX timers. As both nanosleep() and sched_yield() have always been available
> > on darwin, the attached patch simply defines _GLIBCXX_USE_NANOSLEEP and
> > _GLIBCXX_USE_SCHED_YIELD in config/os/bsd/darwin/os_defines.h. This also has
> > the advantage of effectively making --enable-libstdcxx-time=yes the default
> > on darwin. Regression tested on x86_64-apple-darwin12.
> >    Okay for gcc trunk as well as gcc-4_7-branch/gcc-4_6-branch/gcc-4_5-branch?
> 
> The 4.5 branch is closed, so definitely not alright there.
> 
> I don't like the sched_yield macro being set there because it's
> detected correctly by configure anyway, but I'm not going to labour
> that point any more.

Since we are defining _GLIBCXX_USE_NANOSLEEP in os_defines.h and effectively
implementing half of the behavior of --enable-libstdcxx-time=yes, it seemed
odd to not complete the process and define _GLIBCXX_USE_SCHED_YIELD as well.
The usage is not as straight-forward as many other configure options 
(especially in light of the absence of rt timer support on darwin).

> 
> OK for trunk, it's not a regression so I'm not sure about the
> branches. If it doesn't cause problems on the trunk we can decide
> whether to apply it to the 4.7 branch later.

I guess the question is which branches have enough C++11 standard support to
make the change meaningful to the end user.
Jonathan Wakely - Oct. 9, 2012, 3:33 p.m.
On 9 October 2012 14:11, Jack Howarth wrote:
> On Tue, Oct 09, 2012 at 09:21:25AM +0100, Jonathan Wakely wrote:
>> I don't like the sched_yield macro being set there because it's
>> detected correctly by configure anyway, but I'm not going to labour
>> that point any more.
>
> Since we are defining _GLIBCXX_USE_NANOSLEEP in os_defines.h and effectively
> implementing half of the behavior of --enable-libstdcxx-time=yes, it seemed
> odd to not complete the process and define _GLIBCXX_USE_SCHED_YIELD as well.
> The usage is not as straight-forward as many other configure options
> (especially in light of the absence of rt timer support on darwin).

Why does that absence affect the usage of the option?

For darwin there is no difference between --enable-libstdcxx-time=yes
and --enable-libstdcxx-time=rt, which should make it easier to use,
not harder, because there's no need to choose between the two.

>>
>> OK for trunk, it's not a regression so I'm not sure about the
>> branches. If it doesn't cause problems on the trunk we can decide
>> whether to apply it to the 4.7 branch later.
>
> I guess the question is which branches have enough C++11 standard support to
> make the change meaningful to the end user.

Surely the question is the usual one of whether to make a change to a
release branch if it doesn't fix a regression.
Benjamin Kosnik - Oct. 9, 2012, 5:49 p.m.
> I don't like the sched_yield macro being set there because it's
> detected correctly by configure anyway, but I'm not going to labour
> that point any more.

Indeed. Then somebody will waste hours in the future wondering why
configure says no but their TU says yes. 

At least a comment in the configure bits admitting defeat, people.

-benjamin
Jack Howarth - Oct. 9, 2012, 6:23 p.m.
On Tue, Oct 09, 2012 at 10:49:28AM -0700, Benjamin De Kosnik wrote:
> 
> > I don't like the sched_yield macro being set there because it's
> > detected correctly by configure anyway, but I'm not going to labour
> > that point any more.
> 
> Indeed. Then somebody will waste hours in the future wondering why
> configure says no but their TU says yes. 
 
    Since darwin has always supported nanosleep() and sched_yield() 
and the atypical behavior of the -enable-libstdcxx-time configure option
obviously confuses some end-users, it seems rational to just default to
--enable-libstdcxx-time=yes on for darwin. Is there another way to
achieve this short of defining both _GLIBCXX_USE_NANOSLEEP and
_GLIBCXX_USE_SCHED_YIELD in config/os/bsd/darwin/os_defines.h? If so,
I'll revise the patch to use that approach.
           Jack

> 
> At least a comment in the configure bits admitting defeat, people.
> 
> -benjamin

Patch

Index: libstdc++-v3/config/os/bsd/darwin/os_defines.h
===================================================================
--- libstdc++-v3/config/os/bsd/darwin/os_defines.h	(revision 192222)
+++ libstdc++-v3/config/os/bsd/darwin/os_defines.h	(working copy)
@@ -42,4 +42,9 @@ 
 // Static initializer macro is buggy in darwin, see libstdc++/51906
 #define _GTHREAD_USE_RECURSIVE_MUTEX_INIT_FUNC
 
+// Use nanosleep and sched_yield in libc for time.clock and 
+// thread.thread.this in C++11 standard.
+#define _GLIBCXX_USE_NANOSLEEP 1
+#define _GLIBCXX_USE_SCHED_YIELD 1
+
 #endif