[DTrace-devel] [PATCH] Trigger DIVZERO fault when the divider is 0

[Kris Van Hees]

On Thu, Jan 21, 2021 at 09:22:24PM -0500, Eugene Loh wrote:
> The code looks good.  My comments are only around the tests, which are 
> most of the patch anyhow.
> 
> I'm not sure if the copyright dates should include 2006 or not. 
> Regardless, the years should probably match for
>      tst.DTRACEFLT_DIVZERO.div.d
>      tst.DTRACEFLT_DIVZERO.mod.d

No, because the div tests is a copy of the original tst.DTRACEFLT_DIVZERO.d,
whereas tst.DTRACEFLT_DIVZERO.mod.d is new.

> The tests should not check arg5 since we document arg5 as undefined for 
> DTRACEFLT_DIVZERO.  As a result, one can just get rid of the .r.p file 
> (which you do anyhow) but also just print the other args for .r checking 
> instead of all that ERROR-probe code to check args.

I prefer checking the argument values in the script itself rather than using
a .r file for that.  As you correctly mention below, in the script we can make
use of the DTRACEFLT_DIVZERO constant for validation.

The arg5 value is OK to be tested (and in fact is something I would like to
test for) because even though it is documented as being undefined, it is
actually guaranteed to be 0 in this version (by virtue of needing to have a
value for it anyhow).  So, nothing wrong with testing it even though one should
not depend on it being 0 per the documentation.

> Actually, one can check the args in the ERROR probe, but just do 
> something simpler like this:
>      ERROR { exit(arga != vala || argb != valb || argc != valc); }
> rather than replicating
>      ERROR /arga != vala || argb != valb || argc != valc/ { exit(1); }
>      ERROR /arga == vala && argb == valb && argc != valc/ { exit(0); }
> along with the increased chance of making a mistake (as is done above).
> 
> The reason we would want to check the args in the ERROR probe (as above) 
> rather than dump for comparison with a .r file is because arg4's value 
> should be arg4 == DTRACEFLT_DIVZERO, which is something we can check in 
> the ERROR probe.  In contrast, if we dump the numerical value, it 
> becomes "4", which is not a documented value.  That is, we can check:
>      ERROR { exit(arg1 != val1 || arg2 != val2 || arg3 != val3 || arg4 
> != DTRACEFLT_DIVZERO); }

Yes, indeed.  I'll make that change - I am not sure why I used the predicate
instead.  Probably because I was providing more output while debugging using
the tests.

> I see that the old test checked arg1 (EPID), while its replacements do 
> not.  Can we add a check for arg1?

Hm, yes, we should be able to do that by recording the epid in a variable
in the clause that triggers the fault, and then check arg1 against that in
the ERROR probe.  Good question.

> And, I remain disappointed/uneasy that we have lost arg3 (DIF offset).

Again, testing the offset (now BPF instruction offset rather than DIF offset)
requires more complex tests because the value is not as predictable as the
other arguments.  Since we do not yet have tests that use disassebly output
along with tracing output for validation, this is not really an issue to deal
with right now.  Also, providing that offset value is going to require quite
a bit more work to get right for all cases so that will be for a later patch.