Re: CVS commit: src/tests/kernel

[Takashi YAMAMOTO <yamt9999%gmail.com@localhost>]

On Wed, Mar 18, 2026 at 8:23 PM Taylor R Campbell <riastradh%netbsd.org@localhost> wrote:
>
> > Module Name:    src
> > Committed By:   yamt
> > Date:           Tue Mar 17 08:10:58 UTC 2026
> >
> > Modified Files:
> >         src/tests/kernel: t_time_arith.c
> >
> > Log Message:
> > tests/kernel/t_time_arith.c: remove tests which don't make much sense
> >
> > remove tests which use ms and us values as ns because:
> >
> > * they don't make much sense.
> >
> > * their expected results assume a particular implementation.
> >
> > @@ -242,58 +242,6 @@ const struct itimer_transition {
> >               {2,999999999}, {0,0}, 0,
> >               NULL},
> >
> > -     /* (2^63 - 1) us */
> > -     [32] = {{.it_value = {3,0}, .it_interval = {9223372036854,775807}},
> > -             {2,999999999}, {0,0}, 0,
> > -             NULL},
> > -     /* 2^63 us */
> > ...
>
> The reasoning behind these test cases is:
>
> 1. It is critically important to make sure the code gives _some_
>    sensible answer instead of undefined behaviour of arithmetic
>    overflow for all inputs.
>
>    We have had a spate of bugs in that area, and I spent a lot of
>    effort squashing them.  That entailed coming up with and thinking
>    about sensible results of all of these test cases -- I didn't
>    simply call the function as currently implemented and record its
>    outputs; I reasoned through what I expected for every case, and
>    then adjusted as appropriate when my reasoning was wrong.
>
> 2. Edge cases are important to test.  Although these aren't really
>    edge cases of _this_ implementation since the implementation
>    currently works in nanoseconds, they _would_ be edge cases if the
>    implementation were changed to work in microseconds or
>    milliseconds (which would, in turn, be reasonable given our low
>    resolution of sleep times anyway).

actually, they are not edge cases at all.

for example,
{9223372036854,775808} here is
{.tv_sec=9223372036854,.tv_nsec=775808}.
in us, it's 9223372036854000775.

i suspect you actually meant {9223372036854,775808000},
which is 9223372036854775808 = 2^63 in us?

>
> 3. The answers may not be technically correct in theory, but they are
>    applicable only to time intervals that are so large -- 2^63 us is
>    nearly three hundred millennia -- that it is reasonable for us to
>    treat periodic intervals of that duration as `never' in practice.
>
> So please put these test cases back.
>
> If you want to mark them as with a comment explaining that they're not
> technically correct, that's fine.  If you want to implement
> timespecdiv and make the answers more technically correct, that's fine
> too.  But please don't just drop tests that exercise these plausible
> edge cases.
>
> > @@ -406,6 +354,9 @@ ATF_TC_BODY(itimer_transitions, tc)
> >               volatile bool aborted = true;
> >               volatile bool expect_abort = false;
> >
> > +             if (!timespecisset(&it.it_time.it_interval))
> > +                     continue;
> > +
> >               fprintf(stderr, "case %u\n", i);
>
> I guess this doesn't hurt since (a) none of the explicitly written
> test cases use a zero interval, and (b) the kern_time.c caller only
> uses itimer_transition if the interval is nonzero anyway.  But it
> would be nice to have a comment here explaining this; I spent a few
> minutes puzzling over this part of the change before I realized it.

sure.