[DTrace-devel] [PATCH 3/5] Fix size for typecast tuple (and aggregation key) components

[Eugene Loh]

On 9/8/22 17:20, Kris Van Hees wrote:

> On Thu, Sep 08, 2022 at 05:07:13PM -0400, Eugene Loh wrote:
>> On 9/8/22 16:31, Kris Van Hees wrote:
>>
>>> On Thu, Sep 08, 2022 at 02:10:25PM -0400, Eugene Loh via DTrace-devel wrote:
>>>
>>>> Also, the patch should have a test.  Here is a suggestion:
>>> Test added (different from the one you suggested because a single case is quite
>>> sufficient).
>> A single case is sufficient for catching one, already known error. More test
>> cases are helpful for checking a little more broadly for errors one has not
>> yet thought of.  The additional cases are cheap and better resemble "test
>> coverage."  No cost.  Greater confidence.
> I'd be interested to know what errors you think the more broad test would
> catch, and how those are related to this patch.  If there are genuine concerns
> about these cases, I think it would warrant a separate patch.

There is a philosophical difference here, perhaps.  One approach is to 
say, "I found a particular problem;  so what is the narrowest test that 
would catch that problem?"  Another is to say, "There was a problem;  
how can I test more broadly for some range of similar problems?  How can 
I reassure myself I'm not overlooking somewhat related problems?"  
Increasing the test coverage for basically zero cost makes sense to me.  
The test you added has "sufficient" coverage for a specific 
implementation.  But what if one tuple writes some garbage and then the 
next tuple overwrites (corrects) that garbage?  You're okay in the 
"downsizing" (64 to 32 bits) case.  The new, limited test checks for the 
specific fix that was implemented.  But what about when a tuple is being 
"upsized"?  What is the aversion to more rigorous testing?  If we're 
only going to test for bugs we have already anticipated, we're 
overconfident in our ability to predict problems.

I guess I just think there are many more bugs to find, and we do 
ourselves a service by widening test coverage whenever there are 
convenient opportunities (like here) to do so.  When we add tests, 
rather than making them as narrow as possible, let us make them as wide 
as possible within reason.  When we fix a bug, let us not assume it was 
very narrow.  Let us add testing that demonstrates we've searched more 
broadly for related problems.

>>> v2 posted.
>> In any case, it appears that v2 has no tests at all.
> v3 posted to fix that (posted an older copy by accident).
>>>> BEGIN {
>>>>     @a[11111111, 22222222,        0x7fff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +1);
>>>>     @a[11111111, 22222222, (int  )0x7fff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +2);
>>>>     @a[11111111, 22222222, (short)0x7fff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +4);
>>>>     @a[11111111, 22222222, (long )0x7fff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +8);
>>>>     @a[11111111, 22222222,        0x7fffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +16);
>>>>     @a[11111111, 22222222, (int  )0x7fffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +32);
>>>>     @a[11111111, 22222222, (short)0x7fffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +64);
>>>>     @a[11111111, 22222222, (long )0x7fffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +128);
>>>>     @a[11111111, 22222222,        0xffff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +256);
>>>>     @a[11111111, 22222222, (int  )0xffff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +512);
>>>>     @a[11111111, 22222222, (short)0xffff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +1024);
>>>>     @a[11111111, 22222222, (long )0xffff7fff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +2048);
>>>>     @a[11111111, 22222222,        0xffffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +4096);
>>>>     @a[11111111, 22222222, (int  )0xffffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +8192);
>>>>     @a[11111111, 22222222, (short)0xffffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +16384);
>>>>     @a[11111111, 22222222, (long )0xffffffff, 44444444, 55555555] =
>>>> lquantize(3, 0, 10, 1, +32768);
>>>>     exit(0);
>>>> }