I don't feel strongly about it, beyond wanting the gcc build to work. If @kib and @imp you guys don't agree on the direction here, my inclination is just to leave it as it is after 00dccc3164c6dff38350a1baeeea7238acf2efc3 and move on.

What do you think of this? https://github.com/rlibby/freebsd/commit/73605b3a9268d5ea7ad52650ba9ba7c808842dc5

In D56401#1291397, @kib wrote:

Of course it must use the same __SIZEOF_<uppercase type name> as the type used for time_t.

In D56370#1291214, @asomers wrote:

-       ASSERT_EQ(0, pthread_create(&th0, NULL, setattr_th, (void*)FULLSRC))
+       ASSERT_EQ(0, pthread_create(&th0, NULL, setattr_th, (void*)&FULLSRC))
                << strerror(errno);

In D56401#1291100, @kib wrote:

Did you considered using SIZEOF_LONG and SIZEOF_INT instead of 8/4?

Second try: just ifdef it out for i386.

Thanks for the follow up.

In D53152#1214504, @mav wrote:

I think there should be 1, not 2.

Great, thanks. I'll push as is.

So, I realized that --param min-pagesize was introduced in gcc 11.3 and this will cause an error for older gcc. @imp is it still gcc 9 that's the minimum? I can either wrap this with something like

.if ${COMPILER_TYPE} == "gcc" and ${COMPILER_VERSION} >= 110300
CFLAGS.gcc+= --param min-pagesize=1024
.endif

or just drop this as not worth the ugliness.

kp feedback: just initialize the copies to 0

I'll defer to you and kib as maintainers whether this is a good direction. Personally I do like the flattening of the structure that results from moving line 561/563 and following out of the loop, but I'm less sure about the initial loop unrolling. FWIW my preference for the warning would still be to just NULL initialize child.

In D52940#1210706, @dougm wrote:

It seems to me that a sufficiently smart compiler could use this to figure out that child would not be used uninitialized.

--- a/sys/sys/tree.h
+++ b/sys/sys/tree.h
@@ -540,6 +540,11 @@ name##_RB_INSERT_COLOR(struct name *head,                          \
        struct type *child, *child_up, *gpar;                           \
        __uintptr_t elmdir, sibdir;                                     \
                                                                        \
+       gpar = _RB_UP(parent, field);                                   \
+       if ((_RB_BITS(gpar) & _RB_LR) == _RB_LR) {                      \
+               __unreachable();                                        \
+               return (NULL);                                          \
+       }                                                               \
        do {                                                            \
                /* the rank of the tree rooted at elm grew */           \
                gpar = _RB_UP(parent, field);                           \

dougm feedback: _UPMOD_ macros don't improve clarity

@dougm, do you have a preference here? I'm happy to follow a suggestion or have you take it over.

whitespace fixup

kib feedback: macro names could be more clear about modifying the
argument. I went with "MOD" instead of "SET".

kib feedback

There are other options for how to spell the internal macros. I'm open to something else that might be more clear. Something in particular I considered is just providing

#define _RB_OP_RVAL(elm, op, dir)	((__typeof(elm))		\
					((__uintptr_t)(elm) op (dir)))
#define	_RB_OP(elm, op, dir)	((elm) = _RB_OP_RVAL((elm), op, (dir)))
#define	_RB_OPUP(elm, field, op, dir)	_RB_OP(_RB_UP((elm), field), op, (dir))

and not _RB_{OR,XOR,ORUP,XORUP} and then having the rb code look like e.g.

			_RB_OPUP(parent, field, ^, elmdir);

instead of _RB_XORUP(parent, field, elmdir).

asomers feedback: use ATF_REQUIRE_EQ family of tests.

In D45893#1207280, @asomers wrote:

So is -1 being cast to true?

Apologies, trying to recall. IIRC...

mjg feedback: indicate when i_din2 is NULL

Alternately, we could fib and do

printf(", extsize=%d", ip->i_din2 != NULL ? ip->i_din2->di_extsize : 0);

Sure, I'm ambivalent, I just wanted to fix the trap. I'll prepare the patch.

In D52795#1206234, @mjg wrote:

maybe print that the pointer is NULL?