sleep(9), sleepqueue(9): const'ify wchan pointers
ClosedPublic
Actions

Authored by cem on Dec 23 2019, 10:51 PM.

Details

Reviewers

kib
mjg
jhb
markj
rlibby
brooks

Group Reviewers

manpages

Commits

rS356057: sleep(9), sleepqueue(9): const'ify wchan pointers

Summary

_sleep(9), wakeup(9), sleepqueue(9), et al do not dereference or modify the
channel pointers provided in any way; they are merely used as intptrs into a
dictionary structure to match waiters with wakers. Correctly annotate this
such that _sleep() and wakeup() may be used on const pointers without
invoking ugly patterns like __DECONST(). Plumb const through all of the
underlying sleepqueue bits.

No functional change.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem created this revision.Dec 23 2019, 10:51 PM

Harbormaster completed remote builds in B28297: Diff 65939.Dec 23 2019, 10:51 PM

cem added a child revision: D22512: random(4): Simplify RANDOM_LOADABLE.Dec 24 2019, 12:13 AM

I do not object but I also do not see any advantage of this change. Was it done only because you did not liked __DECONST somewhere ?

sys/kern/kern_synch.c
173 ↗	(On Diff #65939)	BTW, this is undefined behavior. Both sides of comparisons should be cast to uintptr_t.
sys/sys/user.h
131 ↗	(On Diff #65939)	This might be user-breaking change.

@kib, Yes. I dislike using __DECONST() when invoking APIs that could be const, but are not by historical accident.

sys/kern/kern_synch.c
173 ↗	(On Diff #65939)	It's only UB if the condition is false ;-). And the UB predates this change. But sure, I am happy to fix it while I'm making changes in the area.
sys/sys/user.h
131 ↗	(On Diff #65939)	I'm not sure how. The pointer is nominally KVA and cannot possibly be meaningful to dereference and mutate by userspace. The size of `void` and `const void` are identical on all FreeBSD arch. Possibly the type of `ki_wchan` should just be `uintptr_t` instead, similar to how `COMPAT_FREEBSD32` uses the integral `uint32_t` type for `ki_wchan`?

kib added inline comments.Dec 24 2019, 1:41 AM

sys/kern/kern_synch.c
173 ↗	(On Diff #65939)	I am not saying that you added UB, just noted that it is there. Probably worth fixing it in prep commit.
sys/sys/user.h
131 ↗	(On Diff #65939)	Using uintptr_t would be also user-breaking change. I mean that a user code that does something with ki_wchan would break. I am not sure that such code exists, which is why I said 'might'. Still, it is outside the actual scope of your change, so I would just leave this place as is.

Looks good. Recommend building at least INVARIANTS and non-INVARIANTS kernels before commit.

sys/kern/kern_synch.c
173 ↗	(On Diff #65939)	While you're changing these lines and the decl, you might also change the type to char, there's no need for uint8_t.

This revision is now accepted and ready to land.Dec 24 2019, 2:08 AM

Rebase over pre-commit changes in rS356049.

This revision now requires review to proceed.Dec 24 2019, 8:14 AM

Harbormaster completed remote builds in B28301: Diff 65949.Dec 24 2019, 8:14 AM

Rebase LGTM.

This revision is now accepted and ready to land.Dec 24 2019, 8:16 AM

Seems reasonable to me.

sys/sys/user.h
131 ↗	(On Diff #65939)	A program doesn't need to dereference or mutate the wchan to be broken by the change. A simple `void *wchan = kp->ki_wchan` won't compile anymore. You could try an exp-run to see the extent of it. I see however that libkvm won't compile if you simply revert this fragment of the patch.