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

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

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

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..

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

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

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

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

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

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

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.

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