1244320 - Switch BSDs from gettimeofday() to clock_gettime(CLOCK_MONOTONIC)

Status: REOPENED

(NSPR :: NSPR, defect, P2)

Description

[Jan Beich]

Discrepancy between Core configure and pr/include/_<platform>.h may lead to hard to notice bugs. So, define HAVE_CLOCK_MONOTONIC on BSDs per POSIX.

Comment 1

[Jan Beich]

Attachment: Enable CLOCK_MONOTONIC on BSDs

Comment 2

[Wan-Teh Chang]

Comment on attachment 8713852 [details] [diff] [review]
Enable CLOCK_MONOTONIC on BSDs

Review of attachment 8713852 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

Patch checked in to the NSPR trunk:
https://hg.mozilla.org/projects/nspr/rev/ed39f339dc1e

Comment 3

[Wan-Teh Chang]

Jan: I don't understand your comment 0 (I guess it was directed
at Landry).

Comment 4

[Landry Breuil (:gaston)]

Wasn't HAVE_CLOCK_MONOTONIC already defined by the env/another header already ? Given that the codepaths were already taken....

Comment 5

[Landry Breuil (:gaston)]

Yeah, i think this patch is wrong, because HAVE_CLOCK_MONOTONIC is checked/set by configure, at least in m-c context:

 9:14.23 In file included from /home/othersrc/mozilla-central/nsprpub/pr/src/md/unix/openbsd.c:6:
 9:14.23 In file included from /home/othersrc/mozilla-central/nsprpub/pr/include/private/primpl.h:44:
 9:14.23 In file included from /home/othersrc/mozilla-central/nsprpub/pr/include/md/prosdep.h:44:
 9:14.23 /home/othersrc/mozilla-central/nsprpub/pr/include/md/_openbsd.h:195:9: warning: 'HAVE_CLOCK_MONOTONIC' macro redefined
 9:14.23 #define HAVE_CLOCK_MONOTONIC
 9:14.23         ^
 9:14.23 /home/obj/m-c/mozilla-config.h:44:9: note: previous definition is here
 9:14.23 #define HAVE_CLOCK_MONOTONIC 1

http://mxr.mozilla.org/mozilla-central/source/configure.in#2989

So either just remove _MD_INTERVAL_USE_GTOD or add some #ifndef guards..

Comment 6

[Jan Beich]

Attachment: Add _MD_ prefix

NSPR should stick to its own namespace for hard-coded stuff rather than sprinkle ugly |#ifdef MOZILLA_CLIENT|.

Comment 7

[Landry Breuil (:gaston)]

Comment on attachment 8713970 [details] [diff] [review]
Add _MD_ prefix

But then, shouldnt the code in pr/src/md/unix/unix.c check for defined(HAVE_CLOCK_MONOTONIC) || defined (_MD_HAVE_CLOCK_MONOTONIC) ?

Comment 8

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #7)
> But then, shouldnt the code in pr/src/md/unix/unix.c check for
> defined(HAVE_CLOCK_MONOTONIC) || defined (_MD_HAVE_CLOCK_MONOTONIC) ?

No. HAVE_CLOCK_MONOTONIC is currently out of scope because it's not detected by NSPR's own configure.in. 

However, it maybe possible to copy clock_gettime(CLOCK_MONOTONIC) check from Core's configure.in and avoid hardcoding in pr/include/_<platform>.h.

Comment 9

[Landry Breuil (:gaston)]

But in the context of building nspr within m-c HAVE_CLOCK_MONOTONIC is in scope since it's in mozilla-config.h.. oh well. Now i see where you're going :)

Comment 10

[Jan Beich]

Attachment: Detect CLOCK_MONOTONIC like Core

(In reply to Jan Beich from comment #8)
> it maybe possible to copy clock_gettime(CLOCK_MONOTONIC) check from Core's configure.in

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb649e51a74d

Comment 11

[Jan Beich]

Comment on attachment 8713974 [details] [diff] [review]
Detect CLOCK_MONOTONIC like Core

How to test NSPR standalone build on Tier1 platforms? Would a Try push to any branch that lacks bug 1230117 be enough?

mozilla-release Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=719817aa850b

Comment 12

[Wan-Teh Chang]

Comment on attachment 8713974 [details] [diff] [review]
Detect CLOCK_MONOTONIC like Core

Review of attachment 8713974 [details] [diff] [review]:
-----------------------------------------------------------------

Jan: thanks for the patch. I think I understand the issue now.

I suggest we just rename HAVE_CLOCK_MONOTONIC to
_PR_HAVE_CLOCK_MONOTONIC and continue to manually define the
macro in _xxx.h.

If we want to use a configure test to define the macro, the
configure test should only run on Unix platforms and should
define _MD_INTERVAL_USE_GTOD if clock_gettime(CLOCK_MONOTONIC)
is not available (except for Mac OS X). I think it's not worth
the trouble.

Comment 13

[Jan Beich]

Comment on attachment 8713974 [details] [diff] [review]
Detect CLOCK_MONOTONIC like Core

(In reply to Jan Beich from comment #11)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=719817aa850b

Try build failed on Linux but due to lack -Wl,-z,defs during NSPR build the error is caught too late. My guess, $(REALTIME_LIBS) for some reason expands to an empty string.

  nsprpub> checking for clock_gettime(CLOCK_MONOTONIC)... -lrt
  [...]
  python2.7 gcc -o shlibsign ... -Ldist/lib -lplc4 -lplds4 -lnspr4  -lpthread  -ldl -lc
  dist/lib/libnspr4.so: undefined reference to `clock_gettime'
  collect2: error: ld returned 1 exit status

However, my local cross-build for CentOS 7 succeeded because clock_gettime() is in libc.

Comment 14

[Jan Beich]

Comment on attachment 8713970 [details] [diff] [review]
Add _MD_ prefix

(In reply to Wan-Teh Chang from comment #12)
> rename HAVE_CLOCK_MONOTONIC to _PR_HAVE_CLOCK_MONOTONIC

OK. I don't enjoy hacking on NSPR due to a lot of cruft that looks like from pre-autotools era. ;\

Comment 15

[Landry Breuil (:gaston)]

Comment on attachment 8713970 [details] [diff] [review]
Add _MD_ prefix

This version makes sense to me, albeit you're prefixing the defines by _MD and not _PR? _MD looks consistent to me

Comment 16

[Wan-Teh Chang]

Attachment: Add _PR_ prefix, by Jan Beich

For NSPR's feature test macros we should use the PR_ or
_PR_ prefix. I made that change and checked this into
the NSPR trunk:
https://hg.mozilla.org/projects/nspr/rev/091cfd3c8c13

Comment 17

[Wan-Teh Chang]

Attachment: Detect CLOCK_MONOTONIC like Core, by Jan Beich

Jan: I updated your configure patch (attachment 8713974 [details] [diff] [review])
to the current NSPR trunk. If you are interested in
completing this work, you can request a review from
Ted Mielczarek and Mike Hommey, who know autoconf much
better than I do.

As I noted earlier, this patch still needs work because we
need to handle three cases on Unix platforms.

1. Special case: Darwin (Mac OS X) uses Mach functions
rather than gettimeofday() or clock_gettime(CLOCK_MONOTONIC).
Right now neither _PR_HAVE_ CLOCK_MONOTONIC nor
_MD_INTERVAL_USE_GTOD is defined in
pr/include/md/_darwin.h.

2. If clock_gettime(CLOCK_MONOTONIC) is available, use
it. Right now this is controlled by the
_PR_HAVE_ CLOCK_MONOTONIC macro, manually defined in
pr/include/md/_<platform>.h.

3. Otherwise, use gettimeofday(). Right now this is controlled
by the _MD_INTERVAL_USE_GTOD macro, manually defined in
pr/include/md/_<platform>.h.

Comment 18

[Wan-Teh Chang]

(In reply to Jan Beich from comment #14)
> 
> ... I don't enjoy hacking on NSPR due to a lot of cruft that looks like from
> pre-autotools era. ;\

Sorry about that. I really appreciate your help to improve the code.
FYI, I spent several weekends on updating NSPR's configure.in script
to autoconf 2.69. So at least I tried hard :-)

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:KaiE, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 20

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.