Re: git: 68912701700c - main - ffs_suspend.c: clean up includes

On 1/3/23 3:53 AM, Konstantin Belousov wrote:
> On Mon, Jan 02, 2023 at 06:29:57PM +0100, Mateusz Guzik wrote:
>> On 12/29/22, Konstantin Belousov <kib@freebsd.org> wrote:
>>> The branch main has been updated by kib:
>>>
>>> URL:
>>> https://cgit.FreeBSD.org/src/commit/?id=68912701700ca3230f3e2d4b7858a038f884a327
>>>
>>> commit 68912701700ca3230f3e2d4b7858a038f884a327
>>> Author:     Konstantin Belousov <kib@FreeBSD.org>
>>> AuthorDate: 2022-12-28 18:17:53 +0000
>>> Commit:     Konstantin Belousov <kib@FreeBSD.org>
>>> CommitDate: 2022-12-29 20:55:39 +0000
>>>
>>>      ffs_suspend.c: clean up includes
>>>
>>>      Order includes alphabetically.
>>>      Remove unneeded sys/param.h, it is already included by sys/systm.h.
>>>
>>>      Reviewed by:    mckusick
>>>      Sponsored by:   The FreeBSD Foundation
>>>      MFC after:      1 week
>>>      Differential revision:  https://reviews.freebsd.org/D37896
>>> ---
>>>   sys/ufs/ffs/ffs_suspend.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/sys/ufs/ffs/ffs_suspend.c b/sys/ufs/ffs/ffs_suspend.c
>>> index d13097109758..e7c976b6e921 100644
>>> --- a/sys/ufs/ffs/ffs_suspend.c
>>> +++ b/sys/ufs/ffs/ffs_suspend.c
>>> @@ -33,15 +33,14 @@
>>>   #include <sys/cdefs.h>
>>>   __FBSDID("$FreeBSD$");
>>>
>>> -#include <sys/param.h>
>>>   #include <sys/systm.h>
>>>   #include <sys/buf.h>
>>> -#include <sys/ioccom.h>
>>> -#include <sys/mount.h>
>>> -#include <sys/vnode.h>
>>>   #include <sys/conf.h>
>>> +#include <sys/ioccom.h>
>>>   #include <sys/jail.h>
>>> +#include <sys/mount.h>
>>>   #include <sys/sx.h>
>>> +#include <sys/vnode.h>
>>>
>>>   #include <security/mac/mac_framework.h>
>>>
>>>
>>
>> tinderbox fails the KCSAN et al kernels:
>>
>> In file included from /usr/src/sys/ufs/ffs/ffs_suspend.c:36:
>> In file included from /usr/src/sys/sys/systm.h:44:
>> In file included from ./machine/atomic.h:73:
>> /usr/src/sys/sys/atomic_san.h:117:24: error: unknown type name 'uint8_t'
>> ATOMIC_SAN_FUNCS(char, uint8_t);
>>                         ^
>> it bisects to this commit
>>
> So the problem is that sys/systm.h includes machine/atomic.h which always
> had the pre-requisite sys/types.h, and this is documented in atomic(9).
> But sys/atomic_san.h uses C89 types in prototypes, not just macros, and
> this breaks for real.
> 
> I can
> 1. Just add sys/types.h to ufs_suspend.c (I prefer not)
> 2. Add sys/types.h to sys/atomic_san.h.
> 3. Add sys/types.h to sys/systm.h.
> 
> IMO #2 is not the best solution, since it pollutes systm.h only sometimes,
> when the right kernel options are used.  I would prefer #3, it seems, but
> want to ensure that there is a consensus about the addition to sys/systm.h.

FYI, I got a different ream of breakages downstream in CheriBSD due to this
commit removing param.h as well.  I also only ever recalled Bruce telling me
to do either <sys/types.h> or <sys/param.h> first (and only one of them).
I don't recall any special rules for <sys/systm.h>.

CheriBSD has a local diff that tries to sort headers in systm.h which is why
we get larger breakage:

--- a/sys/sys/systm.h
+++ b/sys/sys/systm.h
@@ -41,13 +41,22 @@
  #define        _SYS_SYSTM_H_
  
  #include <sys/cdefs.h>
-#include <machine/atomic.h>
-#include <machine/cpufunc.h>
  #include <sys/callout.h>
  #include <sys/kassert.h>
  #include <sys/queue.h>
  #include <sys/stdint.h>                /* for people using printf mainly */
  
+#include <machine/atomic.h>
+#include <machine/cpufunc.h>

I think if we want <sys/systm.h> to be self-contained we should swap <sys/cdefs.h>
for <sys/types.h>.  I think moving the <machine/*> includes down would also be
more consistent with style(9) as well.

-- 
John Baldwin