Page MenuHomeFreeBSD

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

Authored by imp on Jul 5 2021, 4:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 1:07 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Unknown Object (File)
Sat, Nov 2, 1:03 AM
Subscribers

Details

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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40358
Build 37247: arc lint + arc unit

Event Timeline

imp requested review of this revision.Jul 5 2021, 4:49 PM
imp created this revision.
imp added a reviewer: kib.

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

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)

update, per kib's suggestion

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.

sys/sys/_clock_id.h
41

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

update with guard to make things simpler.

tinderboxed... likely final version.

imp marked 3 inline comments as done.Jul 5 2021, 11:17 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

update with kib's suggestions

This revision now requires review to proceed.Jul 6 2021, 11:46 PM
This revision is now accepted and ready to land.Jul 7 2021, 12:00 PM
This revision now requires review to proceed.Jul 12 2021, 3:32 PM
imp edited the test plan for this revision. (Show Details)
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.

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.

sys/sys/_clock_id.h
71

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

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

sys/sys/_clock_id.h
71

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

This revision is now accepted and ready to land.Jul 23 2021, 2:43 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?

sys/sys/_clock_id.h
5

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