Page MenuHomeFreeBSD
Differential D31056

time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h
ClosedPublic
Actions

Authored by imp on Jul 5 2021, 4:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, May 11, 7:24 PM
Unknown Object (File)
Sat, May 11, 7:23 PM
Unknown Object (File)
Sat, May 11, 7:23 PM
Unknown Object (File)
Sat, May 11, 4:53 PM
Unknown Object (File)
Wed, May 8, 12:42 PM
Unknown Object (File)
Wed, May 8, 4:50 AM
Unknown Object (File)
Fri, May 3, 4:43 PM
Unknown Object (File)
Thu, May 2, 5:45 PM
View All 74 Files
Subscribers
kaktus

Details

Reviewers
kib
Commits
rGd7535fb3e3cb: time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h
rG7b797ba27a71: time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h
Summary

Attempt to comply with the strict namespace pollution requirements of
_POSIX_C_SOURCE. Add guards to limit visitbility of CLOCK_ and TIMER_
defines as appropriate. Only define the CLOCK_ variables relevant to the
specific standards. Move all the sharing to sys/_clock_id.h and make
time.h and sys/time.h both include that rather than copy due to the
now large number of clocks and compat defines.

Please note: The old time.h previously used these newer dates:

CLOCK_REALTIME                  199506
CLOCK_MONOTONIC                 200112
CLOCK_THREAD_CPUTIME_ID         200112
CLOCK_PROCESS_CPUTIME_ID        200112

but glibc defines all of these for 199309. I can't get my hands on an
actual IEEE-1003.1b standard to verify this is correct. There are
several ports that assume this value as well. So, I've taken the path of
least resistance and assumed using the glibc dates to be best.

Test Plan

With these fixes, the next exp run would fail only with one issue (a legit bug in the target port):
http://gohan04.nyi.freebsd.org/data/main-amd64-PR256941-default/2021-07-10_18h40m23s/logs/errors/pocl-1.6_1.log
Since it's being naughty and asking for strict namespace but referencing CLOCK_UPTIME_FAST, which is FreeBSD specific.
It is a bug in the code. It requests a strict namespace:

#ifndef _MSC_VER

  1. define _DEFAULT_SOURCE
  2. define __POSIX_VISIBLE 200112L
  3. ifndef _POSIX_C_SOURCE
  4. define _POSIX_C_SOURCE 200112L
  5. endif

and then uses non-strictly conforming values. Also, it uses CLOCK_UPTIME_FAST for *BSD, even though only FreeBSD and DFBSD define it.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp requested review of this revision.Jul 5 2021, 4:49 PM
imp created this revision.
Harbormaster completed remote builds in B40311: Diff 91809.Jul 5 2021, 4:49 PM
imp edited the test plan for this revision. (Show Details)Jul 5 2021, 4:50 PM
imp added a reviewer: kib.
kib added a comment.Jul 5 2021, 6:20 PM

I wonder if we would be better served by e.g. sys/_clock_ids.h that would be included by both sys/time.h and time.h, with the single copy of this stuff.

sys/sys/time.h
474–475

Please look at include/time.h. There CLOCK_REALTIME is provided for __POSIX_VISIBLE >= 199506

475

Wouldn't this cause redefinition if time.h is included earlier?

476

199506

478

time.h uses 200112, they should be synced

imp added a comment.Jul 5 2021, 6:30 PM
In D31056#698628, @kib wrote:

I wonder if we would be better served by e.g. sys/_clock_ids.h that would be included by both sys/time.h and time.h, with the single copy of this stuff.

I do too... I hadn't noticed the time.h ones for some reason, and nobody mentioned them in the other review I have open... :(...

I'll do that first and just use the stuff in time.h, fixed of course (it defines too much).

imp retitled this revision from time.h: reduce CLOCK_ namespace pollution to time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h.Jul 5 2021, 9:04 PM
imp edited the summary of this revision. (Show Details)
imp edited the test plan for this revision. (Show Details)
imp updated this revision to Diff 91829.Jul 5 2021, 9:04 PM

update, per kib's suggestion

Harbormaster completed remote builds in B40331: Diff 91829.Jul 5 2021, 9:04 PM
imp updated this revision to Diff 91830.Jul 5 2021, 9:06 PM

fix comment

Harbormaster completed remote builds in B40332: Diff 91830.Jul 5 2021, 9:06 PM
imp added inline comments.Jul 5 2021, 9:07 PM
sys/sys/_clock_id.h
5

not at all sure about this copyright, but since I copied from time.h and there's a USL copyright there, I'm erring on the side of caution.

imp added inline comments.Jul 5 2021, 9:10 PM
sys/sys/_clock_id.h
41

I could make all these simpler if I used an include guard. Will do that and respin.

imp updated this revision to Diff 91831.Jul 5 2021, 9:44 PM

update with guard to make things simpler.

Harbormaster completed remote builds in B40333: Diff 91831.Jul 5 2021, 9:44 PM
imp mentioned this in D30988: clock_gettime: Add Linux aliases for CLOCK_*.Jul 5 2021, 10:04 PM
imp added a child revision: D30988: clock_gettime: Add Linux aliases for CLOCK_*.Jul 5 2021, 10:05 PM
imp added a comment.Jul 5 2021, 11:17 PM

tinderboxed... likely final version.

imp marked 3 inline comments as done.Jul 5 2021, 11:17 PM
kib accepted this revision.Jul 6 2021, 6:18 PM

For me, it all looks great. I believe that this change requires exp run, unfortunately, and I expect bad surprises.

This revision is now accepted and ready to land.Jul 6 2021, 6:18 PM
imp updated this revision to Diff 91915.Jul 6 2021, 11:46 PM

update with kib's suggestions

This revision now requires review to proceed.Jul 6 2021, 11:46 PM
Harbormaster completed remote builds in B40358: Diff 91915.Jul 6 2021, 11:47 PM
kib accepted this revision.Jul 7 2021, 12:00 PM
This revision is now accepted and ready to land.Jul 7 2021, 12:00 PM
imp updated this revision to Diff 92102.Jul 12 2021, 3:32 PM

update after exp run

This revision now requires review to proceed.Jul 12 2021, 3:32 PM
Harbormaster completed remote builds in B40446: Diff 92102.Jul 12 2021, 3:32 PM
imp edited the summary of this revision. (Show Details)Jul 12 2021, 3:33 PM
imp edited the test plan for this revision. (Show Details)
imp edited the test plan for this revision. (Show Details)Jul 12 2021, 3:42 PM
kib added inline comments.Jul 12 2021, 7:59 PM
sys/sys/_clock_id.h
71

There were no anything related to threads in 1993 edition, I believe. References show that first threading supplement was 1003.1c-1995.

I think it is better to somewhat pollute environment if limiting visibility causes troubles, instead of putting arbitrary versioning. Note that sys/time.h as of now defines symbols unconditionally.

I think that restricting CLOCK_UPTIME etc from the block above, and moving all the stuff to common header to avoid duplication, already makes the significant step forward there. And ultimate cleanup is probably not achievable simply due to compat constraints.

imp added inline comments.Jul 12 2021, 8:16 PM
sys/sys/_clock_id.h
71

This is a 'bug compatibility with glibc' decision. It limits things more than we do today while at the same time as not breaking software needlessly.

Having said that, there were about two or three ports that this affects. I can dig into whether or not upstream is open to what I'm sure are the small tweaks needed to fix this there.

I'm having trouble understanding what course of cation you are recommending, though.

imp added inline comments.Jul 12 2021, 8:25 PM
sys/sys/_clock_id.h
71

"course of action." I can't type today. I'm not sure what you are suggesting I do.

kib added inline comments.Jul 12 2021, 8:41 PM
sys/sys/_clock_id.h
71

I suggest to not add POSIX_VISIBLE checks at all in places where you have to put POSIX_VISIBLE >= 1993. This would give some namespace pollution but overall state is still better than the current.

If you want to go with 1993 for bug compatibility with glibc, perhaps add a comment there stating the reason. Otherwise it is quite confusing, I must admit.

imp updated this revision to Diff 92667.Jul 23 2021, 4:47 AM
imp edited the summary of this revision. (Show Details)
imp edited the test plan for this revision. (Show Details)

Update based on kib's feedback.
Also move CLOCK_MONOTONIC_FAST to allow lang/pocl to compile until it can
be updated upstream.

Harbormaster completed remote builds in B40633: Diff 92667.Jul 23 2021, 4:48 AM
imp added inline comments.Jul 23 2021, 4:49 AM
sys/sys/_clock_id.h
71

I opted for the bug compatibility with comments. Please let me know what you think of the result.

kib accepted this revision.Jul 23 2021, 2:43 PM
This revision is now accepted and ready to land.Jul 23 2021, 2:43 PM
Closed by commit rG7b797ba27a71: time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h (authored by imp). · Explain WhyJul 31 2021, 12:04 AM
This revision was automatically updated to reflect the committed changes.
imp added a commit: rG7b797ba27a71: time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h.
kaktus added a subscriber: kaktus.Jul 31 2021, 10:30 PM
kaktus added inline comments.
sys/sys/_clock_id.h
5

I found no trace of CLOCK_* in 4.3 nor in 4.4-Alpha (based on sources fetched from TUHS) and I believe these were added after the lawsuit. Can we check this with more eyes and in case it's not needed can we remove the USL copyright instead of proliferating it?

imp added inline comments.Jul 31 2021, 11:24 PM
sys/sys/_clock_id.h
5

Let's continue this discussion in D31369. It has more detailed analysis and has my proposed change.

imp added a commit: rGd7535fb3e3cb: time.h: reduce CLOCK_ namespace pollution, move to _clock_id.h.Sep 12 2021, 4:36 PM

Revision Contents
Changeset List

PathSize
include/
30 lines
sys/
sys/
95 lines
28 lines

Diff 93014

View Options

include/time.h

Loading...
View Options

sys/sys/_clock_id.h

Loading...
View Options

sys/sys/time.h

Loading...