sys/time: appease gcc -Wtype-limits

[rlibby]

Summary

In environments where time_t is 32 bits, including the 32-bit library
build on amd64, the overflow being tested for cannot happen, and gcc
complains with -Wtype-limits, causing the gcc build to fail. Work
around this by ifdef'ing out the saturation code on i386.

Fixes: e3799530b3ba ("sys/time: Add saturating sbt conversions")

Test Plan

pkg install amd64-gcc14
env MAKEOBJDIRPREFIX=/usr/obj/gcc14 CROSS_TOOLCHAIN=amd64-gcc14 make buildworld

[markj]

sys/sys/time.h

360

This looks slightly wrong: now, if we have _ts.tv_sec = 0x7fffffff, _ts.tv_nsec = 0, then that'll be converted to SBT_MAX, whereas before it would have been 0x7fffffff00000000`, i.e., we're adding ~1s to the result. That's a corner case and probably doesn't really matter, but is it intentional?

[rlibby]

sys/sys/time.h

360

Yeah, you're right. It's one second out of 2 billion, I don't know if we care, but no I didn't mean to affect that behavior.

This would be easier if we had a TIME_MAX/MIN or weren't in a system header.

I tried a version of convincing gcc with something like if (sizeof(_ts.tv_sec) * __CHAR_BIT > 32), but it wasn't enough for it to bite. Let me take another attempt at something like that.

[rlibby]

sys/sys/time.h

360

It's just really aggressive. Neither of these silence the warning:

if (0 && _ts.tv_sec > SBT_MAX >> 32)

if ((int64_t)_ts.tv_sec > SBT_MAX >> 32)

Frankly the former not silencing the warning seems like a gcc bug.

The least tortured thing I have come up with is

#ifndef __i386__

Ugly, but straightforward. Thoughts?

[jfree]

sys/sys/time.h

360

The underlying issue here is that time_t is 32-bits on x86 and arm platforms where __LP64__ is not defined. See sys/{x86,arm}/include/_types.h.

We could do something like:

static __inline sbintime_t
tstosbt_sat(struct timespec _ts)
{
#ifdef __LP64__
	if (_ts.tv_sec > SBT_MAX >> 32)
		return (SBT_MAX);
	if (_ts.tv_sec < -(SBT_MAX >> 32) - 1)
		return (-SBT_MAX - 1);
#endif
	return (tstosbt(_ts));
}

But this skips the check for other platforms (arm64, powerpc64, riscv64) that don't define __LP64__ but have a 64-bit time_t.

I think we might need to do something like:

static __inline sbintime_t
tstosbt_sat(struct timespec _ts)
{
	if (sizeof(_ts.tv_sec) > sizeof(int32_t)) {
		if (_ts.tv_sec > SBT_MAX >> 32)
			return (SBT_MAX);
		if (_ts.tv_sec < -(SBT_MAX >> 32) - 1)
			return (-SBT_MAX - 1);
	}
	return (tstosbt(_ts));
}

Which is ugly, but seems to cover all cases...

[jfree]

sys/sys/time.h

360

But this skips the check for other platforms (arm64, powerpc64, riscv64) that don't define __LP64__ but have a 64-bit time_t.

Oops, minor correction here. I didn't mean to include 64 when I gave the list of platforms. arm64, powerpc64, riscv64 should define __LP64__. The 32-bit variants of these architectures would be the ones to define __LP64__ but have a 64-bit time_t.

[jfree]

sys/sys/time.h

360

But this skips the check for other platforms (arm64, powerpc64, riscv64) that don't define __LP64__ but have a 64-bit time_t.

Oops, minor correction here. I didn't mean to include 64 when I gave the list of platforms. arm64, powerpc64, riscv64 should define __LP64__. The 32-bit variants of these architectures would be the ones to define __LP64__ but have a 64-bit time_t.

Sigh, I really need to check my comments before I press submit. A correction to the previous correction:

The 32-bit variants of these architectures would be the ones to NOT define __LP64__ but have a 64-bit time_t.

[rlibby]

sys/sys/time.h

360

I'm not sure LP64 is the right check. It looks to me like i386 is the only arch left that defines a 32-bit time_t. arm, arm64, powerpc, and riscv all unconditionally typedef int64_t time_t, it's only x86 that conditions it on LP64. I'm not sure which of those arch types we still build for 32 bits though.

The sizeof-based check unfortunately doesn't work. gcc doesn't inhibit the warning even though it should be able to tell that the -Wtype-limits comparison it is complaining about would not be evaluated in that case. And you can't use sizeof in a preprocessor condition to just ifdef it out that way.

[rlibby]

Second try: just ifdef it out for i386.

[imp]

So this commit shows that the use of this in sys/kern/sys_timerfd.c has been wrong the whole time. We do the math, it overflows, but we can't detect it via these means on i386.

So while the ifdef hides the instant issues, the problems are bigger.

sys/sys/time.h

363

So on i386, this then won't detect the overflow in the math that this is looking for so that we can saturate the math.

[rlibby]

sys/sys/time.h

363

Yes... presumably whatever was computing the timespec/timeval would have just overflowed on i386 if it naively did it in the same way.

I'm not familiar with sys_timerfd.c, but glancing through it, it seems like it is just dealing with the passed-in itimerspec / timespec and adding intervals and boot time. It looks like sys_timerfd.c is generally setting callouts by converting to sbt time since boot, so it is limited to 68 years of uptime, which seems okay.

It looks like i386 will have a 2038 problem, but sys_timerfd.c is probably the least of i386's problems in that respect.

Maybe we just shouldn't advertise the saturation functions as being general purpose and move them to sys_timerfd.c or a kernel header or ifdef _KERNEL them.

I'm pretty ambivalent about the solution here if you all have strong feelings. I just want to fix the gcc build.

[jfree]

This #ifndef __i386__ is bound to be removed once support for i386 is left behind, so I'm ok with this, even if its a little ugly for the time being.

On another note, I spoke to @imp about this earlier today. It doesn't look like -Wtype-limits is used in the kernel. Since tsosbt_sat() and tvtosbt_sat() are only used in the kernel, it would also be appropriate to guard them with #ifdef _KERNEL. I think I like the current #ifndef __i386__ better though since it feels odd to have a guard for only these functions (and having them available outside of the kernel is a plus).

[imp]

Yea, I'd prefer if we could do

#if sizeof(time_t) == 4
<the code>
#endif

but that's not possible, and i386 is likely the next best thing, assuming it unbreaks the world build on i386. And #ifdef _KERNEL being the backup plan to that, should further testing find something else wrong with it.

[imp]

sys/sys/time.h

363

So @jfree and I chatted. The overflow I thought I saw in the code wasn't an overflow. We're subtracting numbers that we know will never over or underflow in the one place I was worried about, and the saturation when we convert that to a sbintime is correct. On 32-bit-time_t platforms, users can't specify something that will overflow, while they can on the others. While one could have uses like I imagined, there's none in the code to go fix.