Patchwork [libfortran] PR 47571 Fix static linking regression

login
register
mail settings
Submitter Janne Blomqvist
Date Feb. 1, 2011, 9:12 p.m.
Message ID <AANLkTinb=SiyUezxej5kARB9MxtvA6A0YqZc6JHDBfmk@mail.gmail.com>
Download mbox | patch
Permalink /patch/81377/
State New
Headers show

Comments

Janne Blomqvist - Feb. 1, 2011, 9:12 p.m.
Hi,

my recent patch to use clock_gettime() made static linking break on
Linux,  since the driver didn't link in librt. It was also pointed out
that always requiring librt in libgfortran might in principle reduce
performance for non-threaded programs as librt pulls in libpthread and
thus the weakrefed IO locking stuff becomes real locks.

The attached patch fixes this by not linking in librt (and this
libpthread) in libgfortran.so, and instead calls clock_gettime()
through a weak reference, falling back to gettimeofday()/other
fallbacks in case librt is not explicitly linked in (or implicitly
e.g. via -fopenmp). For platforms that provide clock_gettime() in
libc, clock_gettime() ought to always be used, although I'm not able
to test this.

Regtested on x86_64-unknown-linux-gnu, tested a simple program to
ensure usage of clock_gettime() or gettimeofday() depending on whether
librt is linked. Ok for trunk?

2011-02-01  Janne Blomqvist  <jb@gcc.gnu.org>

	PR libfortran/47571
	* configure: Regenerated.
	* configure.ac: Don't add librt to LIBS.
	* intrinsics/time_1.h (weak_gettime): Weakref trickery for
	clock_gettime().
	(gf_gettime): Use weak_gettime() instead of clock_gettime().
Janne Blomqvist - Feb. 1, 2011, 9:37 p.m.
On Tue, Feb 1, 2011 at 23:58, Dave Korn <dave.korn.cygwin@gmail.com> wrote:
> On 01/02/2011 21:12, Janne Blomqvist wrote:
>
>> my recent patch to use clock_gettime() made static linking break on
>> Linux,  since the driver didn't link in librt. It was also pointed out
>> that always requiring librt in libgfortran might in principle reduce
>> performance for non-threaded programs as librt pulls in libpthread and
>> thus the weakrefed IO locking stuff becomes real locks.
>>
>> The attached patch fixes this by not linking in librt (and this
>> libpthread) in libgfortran.so, and instead calls clock_gettime()
>> through a weak reference, falling back to gettimeofday()/other
>> fallbacks in case librt is not explicitly linked in (or implicitly
>> e.g. via -fopenmp). For platforms that provide clock_gettime() in
>> libc, clock_gettime() ought to always be used, although I'm not able
>> to test this.
>>
>> Regtested on x86_64-unknown-linux-gnu, tested a simple program to
>> ensure usage of clock_gettime() or gettimeofday() depending on whether
>> librt is linked. Ok for trunk?
>
>  Does SUPPORTS_WEAK necessarily imply supports weakref?  I'm not certain
> they're the same thing.

Well, that macro is used in quadmath_weak.h and in gthr-posix.h in a
similar way, so I guess it's ok.
Jakub Jelinek - Feb. 1, 2011, 9:40 p.m.
On Tue, Feb 01, 2011 at 09:58:36PM +0000, Dave Korn wrote:
> > Regtested on x86_64-unknown-linux-gnu, tested a simple program to
> > ensure usage of clock_gettime() or gettimeofday() depending on whether
> > librt is linked. Ok for trunk?
> 
>   Does SUPPORTS_WEAK necessarily imply supports weakref?  I'm not certain
> they're the same thing.

Yes, it does.

	Jakub
Dave Korn - Feb. 1, 2011, 9:58 p.m.
On 01/02/2011 21:12, Janne Blomqvist wrote:

> my recent patch to use clock_gettime() made static linking break on
> Linux,  since the driver didn't link in librt. It was also pointed out
> that always requiring librt in libgfortran might in principle reduce
> performance for non-threaded programs as librt pulls in libpthread and
> thus the weakrefed IO locking stuff becomes real locks.
> 
> The attached patch fixes this by not linking in librt (and this
> libpthread) in libgfortran.so, and instead calls clock_gettime()
> through a weak reference, falling back to gettimeofday()/other
> fallbacks in case librt is not explicitly linked in (or implicitly
> e.g. via -fopenmp). For platforms that provide clock_gettime() in
> libc, clock_gettime() ought to always be used, although I'm not able
> to test this.
> 
> Regtested on x86_64-unknown-linux-gnu, tested a simple program to
> ensure usage of clock_gettime() or gettimeofday() depending on whether
> librt is linked. Ok for trunk?

  Does SUPPORTS_WEAK necessarily imply supports weakref?  I'm not certain
they're the same thing.

    cheers,
      DaveK
Mike Stump - Feb. 1, 2011, 10:34 p.m.
On Feb 1, 2011, at 1:40 PM, Jakub Jelinek wrote:
> On Tue, Feb 01, 2011 at 09:58:36PM +0000, Dave Korn wrote:
>>> Regtested on x86_64-unknown-linux-gnu, tested a simple program to
>>> ensure usage of clock_gettime() or gettimeofday() depending on whether
>>> librt is linked. Ok for trunk?
>> 
>>  Does SUPPORTS_WEAK necessarily imply supports weakref?  I'm not certain
>> they're the same thing.
> 
> Yes, it does.

darwin is special...  Watch for failures on darwin, if you use weakref and plan on not defining everything that's weak.
Dave Korn - Feb. 1, 2011, 10:54 p.m.
On 01/02/2011 21:40, Jakub Jelinek wrote:
> On Tue, Feb 01, 2011 at 09:58:36PM +0000, Dave Korn wrote:
>>> Regtested on x86_64-unknown-linux-gnu, tested a simple program to
>>> ensure usage of clock_gettime() or gettimeofday() depending on whether
>>> librt is linked. Ok for trunk?
>>   Does SUPPORTS_WEAK necessarily imply supports weakref?  I'm not certain
>> they're the same thing.
> 
> Yes, it does.

  Thanks for the update.  I have no objections to the patch.

    cheers,
      DaveK
Tobias Burnus - Feb. 2, 2011, 8:15 a.m.
On 02/01/2011 10:12 PM, Janne Blomqvist wrote:
> Regtested on x86_64-unknown-linux-gnu, tested a simple program to
> ensure usage of clock_gettime() or gettimeofday() depending on whether
> librt is linked. Ok for trunk?

OK. Thanks for the patch - and watch out for failures on Darwin.

Tobias

> 2011-02-01  Janne Blomqvist<jb@gcc.gnu.org>
>
> 	PR libfortran/47571
> 	* configure: Regenerated.
> 	* configure.ac: Don't add librt to LIBS.
> 	* intrinsics/time_1.h (weak_gettime): Weakref trickery for
> 	clock_gettime().
> 	(gf_gettime): Use weak_gettime() instead of clock_gettime().
>
>
Janne Blomqvist - Feb. 2, 2011, 8:49 a.m.
On Wed, Feb 2, 2011 at 10:15, Tobias Burnus <burnus@net-b.de> wrote:
> On 02/01/2011 10:12 PM, Janne Blomqvist wrote:
>>
>> Regtested on x86_64-unknown-linux-gnu, tested a simple program to
>> ensure usage of clock_gettime() or gettimeofday() depending on whether
>> librt is linked. Ok for trunk?
>
> OK. Thanks for the patch - and watch out for failures on Darwin.

Thanks for the review,

Sending        libgfortran/ChangeLog
Sending        libgfortran/configure
Sending        libgfortran/configure.ac
Sending        libgfortran/intrinsics/time_1.h
Transmitting file data ....
Committed revision 169517.


>
> Tobias
>
>> 2011-02-01  Janne Blomqvist<jb@gcc.gnu.org>
>>
>>        PR libfortran/47571
>>        * configure: Regenerated.
>>        * configure.ac: Don't add librt to LIBS.
>>        * intrinsics/time_1.h (weak_gettime): Weakref trickery for
>>        clock_gettime().
>>        (gf_gettime): Use weak_gettime() instead of clock_gettime().
>>
>>
>
>

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index ed1e2cc..8161659 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -486,11 +486,11 @@  AC_CHECK_LIB([m],[feenableexcept],[have_feenableexcept=yes AC_DEFINE([HAVE_FEENA
 
 # At least for glibc, clock_gettime is in librt.  But don't pull that
 # in if it still doesn't give us the function we want.
-# This test is copied from libgomp.
+# This test is copied from libgomp, and modified to not link in -lrt
+# as libgfortran calls clock_gettime via a weak reference.
 if test $ac_cv_func_clock_gettime = no; then
   AC_CHECK_LIB(rt, clock_gettime,
-    [LIBS="-lrt $LIBS"  
-     AC_DEFINE(HAVE_CLOCK_GETTIME, 1,
+    [AC_DEFINE(HAVE_CLOCK_GETTIME, 1,
                [Define to 1 if you have the `clock_gettime' function.])])
 fi
 
diff --git a/libgfortran/intrinsics/time_1.h b/libgfortran/intrinsics/time_1.h
index 58c51af..86e4331 100644
--- a/libgfortran/intrinsics/time_1.h
+++ b/libgfortran/intrinsics/time_1.h
@@ -189,6 +189,22 @@  gf_cputime (long *user_sec, long *user_usec, long *system_sec, long *system_usec
 #define GF_CLOCK_MONOTONIC GF_CLOCK_REALTIME
 #endif
 
+/* Weakref trickery for clock_gettime().  On Glibc, clock_gettime()
+   requires us to link in librt, which also pulls in libpthread.  In
+   order to avoid this by default, only call clock_gettime() through a
+   weak reference.  */
+#ifdef HAVE_CLOCK_GETTIME
+#ifdef SUPPORTS_WEAK
+static int weak_gettime (clockid_t, struct timespec *) 
+  __attribute__((__weakref__("clock_gettime")));
+#else
+static inline int weak_gettime (clockid_t clk_id, struct timespec *res)
+{
+  return clock_gettime (clk_id, res);
+}
+#endif
+#endif
+
 /* Arguments:
    clock_id - INPUT, must be either GF_CLOCK_REALTIME or GF_CLOCK_MONOTONIC
    secs     - OUTPUT, seconds
@@ -208,14 +224,18 @@  gf_gettime (int clock_id __attribute__((unused)), time_t * secs,
             long * nanosecs)
 {
 #ifdef HAVE_CLOCK_GETTIME
-  struct timespec ts;
-  int err;
-  err = clock_gettime (clock_id, &ts);
-  *secs = ts.tv_sec;
-  if (nanosecs)
-    *nanosecs = ts.tv_nsec;
-  return err;
-#elif HAVE_GETTIMEOFDAY
+  if (weak_gettime)
+    {
+      struct timespec ts;
+      int err;
+      err = weak_gettime (clock_id, &ts);
+      *secs = ts.tv_sec;
+      if (nanosecs)
+	*nanosecs = ts.tv_nsec;
+      return err;
+    }
+#endif
+#ifdef HAVE_GETTIMEOFDAY
   struct timeval tv;
   int err;
   err = gettimeofday (&tv, NULL);