Page MenuHomeFreeBSD

Make timespecadd(3) and friends public
AbandonedPublic

Authored by asomers on Mar 17 2018, 11:20 PM.

Details

Summary

Make timespecadd(3) and friends public

The timespecadd(3) family of macros were imported from NetBSD back in
r35029. However, they were initially guarded by #ifdef _KERNEL. In the
meantime, we have grown at least 28 syscalls that use timespecs in some
way, leading many programs both inside and outside of the base system to
redefine those macros. It's better just to make the definitions public.

Our kernel's definitions of timespecadd and timespecsub are slightly different
than NetBSD's and OpenBSD's. This revision changes our definition to match
theirs'.

Bump _FreeBSD_version due to the breaking KPI change.

Test Plan

Ran all existing ATF tests.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18254
Build 17982: arc lint + arc unit

Event Timeline

asomers created this revision.Mar 17 2018, 11:20 PM
cem added a comment.Mar 18 2018, 4:07 PM

Rather than churning all of the locations that use a 2-argument timespecfoo(), did you consider a compatibility implementation like this?

#define timespecadd_impl(arg1, arg2, arg3, arg4, ...)  do {                     \
        struct timespec *dst;                                                   \
                                                                                \
        _Static_assert((arg4) == NULL, "timespecadd: 4th argument");            \
        if ((arg3) != NULL)                                                     \
                dst = (arg3);                                                   \
        else                                                                    \
                dst = (arg1);                                                   \
                                                                                \
        dst->tv_sec = (arg1)->tv_sec + (arg2)->tv_sec;                          \
        dst->tv_nsec = (arg1)->tv_nsec + (arg2)->tv_nsec;                       \
        if (dst->tv_nsec >= 1000000000L) {                                      \
                dst->tv_sec++;                                                  \
                dst->tv_nsec -= 1000000000L;                                    \
        }                                                                       \
} while (0)

#define timespecadd(arg1, arg2, ...) do {                                       \
        timespecadd_impl((arg1), (arg2), ## __VA_ARGS__, NULL, NULL);           \
} while (0)
jilles added a subscriber: jilles.Mar 18 2018, 5:49 PM

The changes to adapt to the new API look correct to me.

In D14725#309586, @cem wrote:

Rather than churning all of the locations that use a 2-argument timespecfoo(), did you consider a compatibility implementation like this?

#define timespecadd_impl(arg1, arg2, arg3, arg4, ...)  do {                     \
        struct timespec *dst;                                                   \
                                                                                \
        _Static_assert((arg4) == NULL, "timespecadd: 4th argument");            \
        if ((arg3) != NULL)                                                     \
                dst = (arg3);                                                   \
        else                                                                    \
                dst = (arg1);                                                   \
                                                                                \
        dst->tv_sec = (arg1)->tv_sec + (arg2)->tv_sec;                          \
        dst->tv_nsec = (arg1)->tv_nsec + (arg2)->tv_nsec;                       \
        if (dst->tv_nsec >= 1000000000L) {                                      \
                dst->tv_sec++;                                                  \
                dst->tv_nsec -= 1000000000L;                                    \
        }                                                                       \
} while (0)
#define timespecadd(arg1, arg2, ...) do {                                       \
        timespecadd_impl((arg1), (arg2), ## __VA_ARGS__, NULL, NULL);           \
} while (0)

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

Apart from that, this compatibility code has a run-time effect where a call like timespecadd(a, b, c) writes to *a iff c is NULL, which makes it harder to remove eventually. If compatibility is wanted, it should be a pure compile-time check such as using C11 _Generic or our __generic.

sys/compat/linuxkpi/common/include/linux/time.h
74–75

Apart from violating -Waggregate-return, this is a nice and simple API. There is no need to think about pointer aliasing, and struct timespec is small enough that copying is not an undue burden.

Since we do not want to remove previously working warning flags from users, this may not be a good idea for a userland API.

sys/netsmb/smb_trantcp.c
556

The last two calls could be replaced by one call timespecadd(&nbp->nbp_timo, &nbp->nbp_timo, &nbp->nbp_timo);

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

Completely agree.

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

Completely agree.

I agree, too. But I *do* think that userland should have some form of timespecadd, and I think we need to follow NetBSD's example, or 3rd party programs will have even worse compatibility issues. Compared to that, changing our internal KPI isn't too hard.

sys/compat/linuxkpi/common/include/linux/time.h
74–75

Yeah. The aggregate return issue only applies to Linux KPI, and my revision doesn't change it.

cem added a comment.EditedMar 18 2018, 9:26 PM

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

I don't follow that line of argument — that compatibility means we should abandon 3-arg timespecfoo, or bringing timespecfoo into userspace. But I will bow to quorum here, which seems to be: we'd rather churn it than have compatibility.

Apart from that, this compatibility code has a run-time effect where a call like timespecadd(a, b, c) writes to *a iff c is NULL, which makes it harder to remove eventually.

Any such code currently has undefined behavior. At best it has runtime behavior of immediately panicing, so, ¯\_(ツ)_/¯. Maybe don't write UB code. Consider also this API is usually used on immediate stack variables, rather than pointers that can sometimes be NULL.

If compatibility is wanted, it should be a pure compile-time check such as using C11 _Generic or our __generic.

Can something like this be implemented with _Generic? I don't see it, but would be curious to learn how.

In D14725#309715, @cem wrote:

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

I don't follow that line of argument — that compatibility means we should abandon 3-arg timespecfoo, or bringing timespecfoo into userspace. But I will bow to quorum here, which seems to be: we'd rather churn it than have compatibility.

Moving to 3-arg timespecfoo means that we will eventually have to change all uses of 2-arg timespecfoo; I hope that is not controversial. Then the question is just whether we should eat that churn right away or do it piecemeal.

Apart from that, this compatibility code has a run-time effect where a call like timespecadd(a, b, c) writes to *a iff c is NULL, which makes it harder to remove eventually.

Any such code currently has undefined behavior. At best it has runtime behavior of immediately panicing, so, ¯\_(ツ)_/¯. Maybe don't write UB code. Consider also this API is usually used on immediate stack variables, rather than pointers that can sometimes be NULL.

In the new version, it has a behaviour that might be depended on.

If compatibility is wanted, it should be a pure compile-time check such as using C11 _Generic or our __generic.

Can something like this be implemented with _Generic? I don't see it, but would be curious to learn how.

Here is an implementation of something similar using C11 _Generic:

struct macro23_noarg;

static inline void
M_2(int a, int b, struct macro23_noarg *__unused dummy)
{
	printf("two -> %d,%d\n", a, b);
}

static inline void
M_3(int a, int b, int c)
{
	printf("three -> %d,%d,%d\n", a, b, c);
}

#define M_impl(a, b, c, d, ...) do { \
	_Generic(d, \
		struct macro23_noarg *: _Generic(c, \
			struct macro23_noarg *: M_2, \
			default: M_3 \
		)(a, b, c) \
	); \
} while(0)

#define M(a, ...) M_impl((a), __VA_ARGS__, (struct macro23_noarg *)0, (struct macro23_noarg *)0, (struct macro23_noarg *)0)

Unfortunately the expressions in a generic selection must be valid to a fair degree; selecting between MM_2(a, b) and MM_3(a, b, c) causes warnings about the MM_3 call in the MM_2 case (GCC and Clang agree). Therefore, this selects between functions.

An API client must not use the struct macro23_noarg type directly.

cem removed a reviewer: cem.Mar 19 2018, 8:31 PM

So it sounds like this is not a good candidate for _GENERIC. Shall I commit it as-is then? I've already updated all clients of the 2-argument versions to use the 3-argument versions.

ping. Since I can't use GENERIC, are you two satisified with my solution of replacing all 2-argument uses with the 3-argument version?

asomers updated this revision to Diff 45695.Jul 22 2018, 7:16 PM

Rebase. Also, remove contrib/ntp from the diff to reduce divergence from upstream.

asomers abandoned this revision.Jul 30 2018, 9:27 PM

Committed by r336914. I don't know why Phabricator didn't close it automatically.

Phabricator's still working:

Still Importing...
This commit is still importing. Changes will be visible once the import finishes.