Sanitizer kernels build.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
\\\\\\\\\\\\\\\\\\
| sys/sys/atomic_san.h | ||
|---|---|---|
| 43 | We always required sys/types.h before machine/atomic.h, and this header claims that machine/atomic.h is the pre-requisite. This is the reason why I did not proposed to include sys/types.h into sys/atomic_san.h IMO it is better to fix sys/systm.h to follow machine/atomic.h requirements (my #3 in the mail) | |
| 46 | sys/cdefs.h inclusion is definitely redundand | |
| sys/sys/atomic_san.h | ||
|---|---|---|
| 43 | In general we seem to be preferring self-contained headers. I cannot see why it is preferable to force consumers to include sys/types.h when the dependency is unconditional. For your #3, shall we simply move the existing sys/param.h include in sys/systm.h? | |
| 46 | If we can assume that sys/types.h includes sys/cdefs.h, then the cdefs include should be removed from atomic_common.h as well. | |
| sys/sys/atomic_san.h | ||
|---|---|---|
| 43 | We prefer self-contained headers, but this header is not supposed to be used. It is internal helper for machine/atomic.h. This is why I do not see much sense in making it more namespace-polluting than the header that utilizes it. I still prefer my #3 as an addition of sys/types.h to sys/systm.h instead of sys/param.h. There are user-mode bits in sys/systm.h. | |
| 46 | sys/cdefs.h is mostly magic, it should not be included in random headers (IMO). | |
| sys/sys/atomic_san.h | ||
|---|---|---|
| 43 | Then machine/atomic.h should include sys/types.h. But this still makes less sense to me, since it's atomic_common.h/atomic_san.h that actually need it.
Not really? There is a forward declaration of struct ucred for some reason, and some macros (__read_mostly etc.) which do not accomplish much in usermode without extra linker directives. In particular, I can't see why systm.h should include machine/atomic.h for userland. Nonetheless, here is a systm.h patch: https://reviews.freebsd.org/D37943 | |
Comment Actions
so, I like this w/o the sys/cdefs.h.
We're including the pre-reqs at the level they are used, so either here or possibly machine/atomic.h (the next layer up) is the right place.
systm.h is fixing it at the wrong level.
We're moving towards self contained files, where possible. Where that's not possible or desirable, we should included at the lowest level that makes sense.
systm.h is absolutely the wrong place, (sorry kib) imho. It move knowledge of how atomic.h is implemented there, and should we ever have to change it, we'll need to change systm.h. I'm having a lot of trouble understanding why that's the right fix.
(ah, between the time I saw this on my phone and when I got a chance to comment, it was committed)