Page MenuHomeFreeBSD

Reduce code duplication in machine/_types.h
ClosedPublic

Authored by arichardson on Apr 21 2021, 5:40 PM.

Details

Summary

Many of these typedefs are the same accross all architectures or can
be set based on an architecture-independent compiler provided macro
(e.g. __SIZEOF_SIZE_T). These macros have been available since GCC 4.6
and Clang sometime before 3.0 (godbolt.org does not have any older clang
versions installed).

I originally considered using the compiler-provided FOO_TYPE directly.
However, in order to do so we have to check that those match the previous
typedef exactly (not just that they have the same size) since any change
would be an ABI break. For example, changing long to long long results
in different C++ name mangling.

This de-deduplication will allow us to only change the (u)intptr_t
definition in sys/_types.h in CheriBSD instead of having to change
machine/_types.h for all CHERI-enabled architectures.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sys/sys/_types.h
38–42

I can't think of why they wouldn't; if Clang disagrees with FreeBSD on the type then that would cause issues still, surely? I would much rather use __FOO_TYPE__ directly, and if we really need to check it for all architectures then we can, just a bunch of clang -target foo -dM -E - </dev/null | grep calls.

sys/sys/_types.h
50

These macros require GCC 4.6 or newer, but D29012 already depends on that so this should not be raising the oldest supported compiler version.

sys/sys/_types.h
38–42

I think this mismatch should only cause issues for code that uses FOO_TYPE directly since the clang-provided stddef.h/stdint.h/etc. aren't used on FreeBSD.

sys/sys/_types.h
38–42

I'd be happy to use them directly if we think this is safe for both Clang+GCC.

sys/sys/_types.h
38–42

The _TYPE__ defines are built-in to the compiler, they don't need any headers. However, at least on amd64, clang and gcc seem to have a few differences (apart from one using short unsigned int and the other unsigned short, which aren't functionally different):

% clang -dM -E -x c /dev/null|grep _TYPE__|sort > types.clang
% gcc -dM -E -x c /dev/null|grep _TYPE__|sort > types.gcc
% % diff types.clang types.gcc
1c1
< #define __CHAR16_TYPE__ unsigned short
---
> #define __CHAR16_TYPE__ short unsigned int
3c3
< #define __INT16_TYPE__ short
---
> #define __INT16_TYPE__ short int
9c9
< #define __INT_FAST16_TYPE__ short
---
> #define __INT_FAST16_TYPE__ int
12,13c12,13
< #define __INT_FAST8_TYPE__ signed char
< #define __INT_LEAST16_TYPE__ short
---
> #define __INT_FAST8_TYPE__ int
> #define __INT_LEAST16_TYPE__ short int
17a18
> #define __SIG_ATOMIC_TYPE__ int
19c20
< #define __UINT16_TYPE__ unsigned short
---
> #define __UINT16_TYPE__ short unsigned int
25c26
< #define __UINT_FAST16_TYPE__ unsigned short
---
> #define __UINT_FAST16_TYPE__ unsigned int
28,29c29,30
< #define __UINT_FAST8_TYPE__ unsigned char
< #define __UINT_LEAST16_TYPE__ unsigned short
---
> #define __UINT_FAST8_TYPE__ unsigned int
> #define __UINT_LEAST16_TYPE__ short unsigned int

So specifically, the opinions on what are "fast" types differs.

sys/arm/include/_types.h
57

Just noting that these ifdefs are dropped. Is lint something historical?

sys/arm/include/_types.h
57

I believe that was support for xlint which was removed in 2017.

This looks good to me, though you might want to consider a FreeBSD_version bump since it is a KPI change, even if it is just internal...
I don't think you have to do an exp run, but one would hurt if you really wanted to do that. This file is rarely, if ever, used outside sys/*/include/*.h, even for weirdo drivers that need to know about bits of the kernel we don't guarantee to be stable.

sys/arm/include/_types.h
57

It was linters in general, the last of which was xlint which we deorbited in 2017 after a decade plus of neglect.
It's time has long since passed: compilers have been able to do all the cross checks since the late 1990s or early 2000s.

This revision is now accepted and ready to land.Apr 21 2021, 10:22 PM
sys/arm/include/_types.h
43–44

This check is redundand now

68–69

vm_offset_t/vm_size_t can be made MI I think with uintptr_t/ptrdiff_t (but I did not checked far enough)

sys/sys/_types.h
50

This brings one more hidden hard dependency on gcc/clang.
This should be _LP64 in fact.

73

We do use _LP64 for this kind of check.

sys/arm/include/_types.h
68–69

uintptr_t is not the right type for vm_offset_t; it's an integer address-sized offset, not a whole pointer.

sys/sys/_types.h
38–42

The fast types are a long-standing ABI difference between GCC and Clang I believe. But this patch still leaves those as MD; the main benefit is having the fixed-width types be MI, which everything else can then be defined in terms of.

50

No it should not. CHERI also wants these and is _not_ LP64.

73

And we keep wanting to fix that to use more precise and correct checks.

sys/arm/include/_types.h
68–69

I do not understand your comment. Your description of 'address-sized offset' perfectly fits both uintptr_t and vm_offset_t.

sys/sys/_types.h
73

This check is compiler-specific vs. using industry-wide common feature (probably defined by standard but I do not want to dig right now).

sys/arm/include/_types.h
68–69

Traditional architectures conflate integers and pointers, and the C code reflects that. CHERI does not make such mistakes, and thus vm_offset_t is _not_ a uintptr_t, as the latter carries around a full capability.

sys/sys/_types.h
73

No it is not. All of ICC, GCC, Clang define ILP32/LP64/LLP64 macros when true as well as the __SIZEOF_FOO__ (yes, even ICC). MSVC defines _neither_ (you use _WIN64). Only really crusty compilers that nobody in their right mind should be using, like Sun's (do you really think we support that already today?), define solely the cruddy ILP32/LP64/LLP64.

sys/sys/_types.h
73

And were there a compiler where these were not defined that we needed to care about, they could be defined in sys/cdefs.h like we do other things for non-mainstream compilers....

sys/sys/_types.h
73

A quick survey of POSIX and C standards show nothing standardized for either SIZEOF_FOO, nor ILP32, LP64, WIN64, etc, so we're left with what the compilers support. They survey was quick, and I may have missed something in an annex or that's standardized via a different path...

sys/sys/_types.h
73

Windows does not define LP64 of any kind because it is not LP64, it is LLP64.

FWIW http://archive.opengroup.org/public/tech/aspen/lp64_wp.htm and I remember it was part of more elaborate doc than just this single page, but I do not want to put much efforts in digging.

Current POSIX attitude seems to be that it should only define how applications select 32 or 64 bit environment, and there are getconf -v _POSIX_V6_LP64_OFF64 PATH or getconf -v _POSIX_V6_LP64_OFF64 CFLAGS like invocations that provide the path/cflags/ldflags for corresponding 32 or 64 bit compilation environment, Of course we do not support it, while I remember that Solaris and HP-UX did.

sys/arm/include/_types.h
68–69

Specifically, CHERI C adds a new intrinsic ptraddr_t type that represents the scalar integer type which holds addresses which is distinct from uintptr_t which holds pointers (with contain an address and metadata in CHERI C). In CheriBSD we have added a vm_ptr_t which holds user pointers as distinct from vm_offset_t which holds user addresses.

We could in fact move vm_offset_t and vm_size_t to the MI header I believe. In CheriBSD we already provide a fallback for __ptraddr_t and we would simply define vm_offset_t always as ptraddr_t and vm_ptr_t always as uintptr_t. It wouldn't be a huge diff in CheriBSD to do that. In FreeBSD it would be ok to just use uintptr_t for vm_offset_t.

sys/sys/_types.h
73

Windows does not define LP64 of any kind because it is not LP64, it is LLP64.

Yes, I know, that's why I said "ILP32/LP64/LLP64", not just "ILP32/LP64".

Add changes suggested by @kib:

  • Drop check for cdefs include (redundant now that sys/_types.h must be include)
  • Move vm_offset_t and vm_size_t to sys/_types.h
This revision now requires review to proceed.Apr 26 2021, 2:23 PM
This revision is now accepted and ready to land.Apr 26 2021, 2:57 PM