Re: [PATCH v2 01/22] test: Handle dtrace:::ERROR arg3 specially

Kris Van Hees
2024-08-30T16:05:03Z

On Fri, Aug 30, 2024 at 01:28:37AM -0400, Eugene Loh wrote:
> I don't get it.  The patch was supposed to be about testing ERROR arg3.  The
> "ASSERTION" in the test still claims this, and the name of the test does
> too.  But the test doesn't have an ERROR probe nor does it access arg3.
> So...?

DTrace provides a default ERROR probe to handle error conditions, which is
what prints the default error message, accessing the arg1..arg5 values. So
there is no need to explicitly include an ERROR probe. That default ERROR
probe clause is defined in dt_handle.c.

> Also, the test checks fewer things than does the version in the patch.  You
> say that's sufficient.  Maybe it is and maybe it isn't.  I'd vote for the
> more stringent test.  Tests can check for bugs that we anticipate, but I
> think better testing improves our chances of catching bugs we do not
> anticipate.  Certainly checking that a PC matches at least one of the
> dt_probe_error() sites is a better sanity check than what was in the v1 of
> this patch, but it's not checking that the arg3 value is correct.  Given
> that we now (with v2) have a test that checks correctness, I don't see why
> we'd switch to a test that checks less.
>
> I'm not sure how "ready" or final the code is supposed to be, but it also
> has some small variations from the bulk of the test suite.  Not necessarily
> anything monumental, but I think it's nice to keep boilerplate stuff as
> uniform as possible.  The "rm $DIRNAME" and the missing blank "#" line after
> "licenses" seem to be uncommon practices in our test suite.  Again, not a
> big deal, but seems like stuff that should be boringly regular from one test
> to the next.

It is a sample alternative simpler test.

> Anyhow, my vote is for a test that checks ERROR and arg3 and correctness.

I appreciate the detail in your test to extract very specific information,
and the way it is done. But that makes it also more fragile in view of any
changes in the disassembler output, error message output, code genertion of
the clauses that will trigger the errors (e.g. that there will be one and only
one dt_probe_error prior to the one you are looking for, ... That makes for
a lot of potential things that can change and cause the test to fail without
ERROR's arg3 being wrong.

I think you just end up depending (and essentially testing) way too many other
things that could lead to a failure without the thing being tested actually
being broken.

> On 8/29/24 23:34, Kris Van Hees wrote:
> > Attachment did not work apparently... Inlining...
> >
> > #!/bin/bash
> > #
> > # Oracle Linux DTrace.
> > # Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> > # Licensed under the Universal Permissive License v 1.0 as shown at
> > # http://oss.oracle.com/licenses/upl.
> >
> > ##
> > # ASSERTION: The ERROR arg3 value reports the correct PC value.
> > #
> > # SECTION: Variables/Built-in Variables/arg3
> > ##
> >
> > dtrace=$1
> >
> > DIRNAME="$tmpdir/arg3-ERROR.$$.$RANDOM"
> > mkdir -p $DIRNAME
> > cd $DIRNAME
> >
> > $dtrace $dt_flags -xdisasm=8 -Sqs /dev/stdin << EOT &> D.out
> > BEGIN
> > {
> > x = (int *)64;
> > y = *x; /* 1st ERROR */
> > }
> >
> > BEGIN
> > {
> > x = (int *)64;
> > y = *x; /* 2nd ERROR */
> > }
> >
> > BEGIN
> > {
> > x = (int *)64;
> > y = *x; /* 3rd ERROR */
> > }
> >
> > BEGIN
> > {
> > x = (int *)64;
> > y = *x; /* 4th ERROR */
> > }
> >
> > BEGIN
> > {
> > exit(0);
> > }
> > EOT
> >
> > awk '/call dt_probe_error/ {
> > sites[int($1)] = 1;
> > next;
> > }
> >
> > /error on enabled probe/ {
> > if (!($NF in sites)) {
> > print;
> > print " No call to dt_probe_error found at PC " $NF;
> > } else
> > ok++;
> > }
> >
> > END {
> > if (ok != 4) {
> > print "\nFound " ok " valid calls to dt_probe_error, expected 4";
> > exit 1;
> > }
> >
> > print "OK";
> > exit 0;
> > }' D.out
> >
> > rc=$?
> >
> > [ $rc -ne 0 ] && cat D.out
> >
> > rm -rf $DIRNAME
> >
> > exit $rc
> >
> > On Thu, Aug 29, 2024 at 11:24:53PM -0400, Kris Van Hees wrote:
> > > How about something a bit simpler like the attached? We don't really need to
> > > get too specific because as long as the reported PC in the ERROR reporting
> > > matches a dt_probe_error call site, we know that the reported PC is valid.
> > >
> > > I think that would be sufficient as a test for the arg3 value.
> > >
> > > On Thu, Aug 29, 2024 at 06:18:19PM -0400, eugene.loh@oracle.com wrote:
> > > > From: Eugene Loh <eugene.loh@oracle.com>
> > > >
> > > > The ERROR probe's arg3 reports the culprit PC, whose value can vary
> > > > with minor implementation changes. On the one hand, we do not want
> > > > tests to be overly sensitive to this value. On the other hand, we
> > > > do want to check the value is correct.
> > > >
> > > > Therefore:
> > > >
> > > > *) Change tests that dump ERROR's args to omit arg3.
> > > >
> > > > *) Add a new test that checks that ERROR's arg3 is correct.
> > > >
> > > > Signed-off-by: Eugene Loh <eugene.loh@oracle.com>
> > > > ---
> > > > test/unittest/error/tst.DTRACEFLT_UNKNOWN.d | 6 +-
> > > > test/unittest/error/tst.DTRACEFLT_UNKNOWN.r | 2 +-
> > > > .../regression/tst.DTRACEFLT_BADADDR.d_path.d | 8 +-
> > > > .../regression/tst.DTRACEFLT_BADADDR.d_path.r | 2 +-
> > > > .../unittest/variables/bvar/tst.arg3-ERROR.sh | 172 ++++++++++++++++++
> > > > 5 files changed, 181 insertions(+), 9 deletions(-)
> > > > create mode 100755 test/unittest/variables/bvar/tst.arg3-ERROR.sh
> > > >
> > > > diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> > > > index 001903ff..bfc77bf5 100644
> > > > --- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> > > > +++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.d
> > > > @@ -1,6 +1,6 @@
> > > > /*
> > > > * Oracle Linux DTrace.
> > > > - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> > > > + * Copyright (c) 2006, 2024, Oracle and/or its affiliates. All rights reserved.
> > > > * Licensed under the Universal Permissive License v 1.0 as shown at
> > > > * http://oss.oracle.com/licenses/upl.
> > > > */
> > > > @@ -19,8 +19,8 @@
> > > > ERROR
> > > > {
> > > > - printf("The arguments are %u %u %u %u %u\n",
> > > > - arg1, arg2, arg3, arg4, arg5);
> > > > + printf("The arguments are %u %u PC %u %u\n",
> > > > + arg1, arg2, arg4, arg5);
> > > > printf("The value of arg4 = %u\n", DTRACEFLT_UNKNOWN);
> > > > exit(0);
> > > > }
> > > > diff --git a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> > > > index b11f6c99..3e7caac4 100644
> > > > --- a/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> > > > +++ b/test/unittest/error/tst.DTRACEFLT_UNKNOWN.r
> > > > @@ -1,4 +1,4 @@
> > > > -The arguments are 2 2 4 1 64
> > > > +The arguments are 2 2 PC 1 64
> > > > The value of arg4 = 0
> > > > -- @@stderr --
> > > > diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> > > > index c23f9503..ec519f4f 100644
> > > > --- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> > > > +++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.d
> > > > @@ -1,10 +1,10 @@
> > > > /*
> > > > * Oracle Linux DTrace.
> > > > - * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
> > > > + * Copyright (c) 2015, 2024, Oracle and/or its affiliates. All rights reserved.
> > > > * Licensed under the Universal Permissive License v 1.0 as shown at
> > > > * http://oss.oracle.com/licenses/upl.
> > > > */
> > > > -/* @@xfail: dtv2 */
> > > > +/* @@xfail: dtv2 d_path */
> > > > /*
> > > > * ASSERTION:
> > > > @@ -18,8 +18,7 @@
> > > > ERROR
> > > > {
> > > > - printf("The arguments are %u %u %u %u %u\n",
> > > > - arg1, arg2, arg3, arg4, arg5);
> > > > + printf("The arguments are %u %u PC %u %u\n", arg1, arg2, arg4, arg5);
> > > > printf("The value of arg4 should be %u\n", DTRACEFLT_BADADDR);
> > > > printf("The value of arg5 should be %u\n", 0x18);
> > > > exit(0);
> > > > @@ -29,4 +28,5 @@ BEGIN
> > > > {
> > > > d = d_path((struct path *)0x18);
> > > > trace(d);
> > > > + exit(1);
> > > > }
> > > > diff --git a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> > > > index 8c601a43..be1f6d5b 100644
> > > > --- a/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> > > > +++ b/test/unittest/regression/tst.DTRACEFLT_BADADDR.d_path.r
> > > > @@ -1,4 +1,4 @@
> > > > -The arguments are 2 1 28 1 24
> > > > +The arguments are 2 1 PC 1 24
> > > > The value of arg4 should be 1
> > > > The value of arg5 should be 24
> > > > diff --git a/test/unittest/variables/bvar/tst.arg3-ERROR.sh b/test/unittest/variables/bvar/tst.arg3-ERROR.sh
> > > > new file mode 100755
> > > > index 00000000..4b139be2
> > > > --- /dev/null
> > > > +++ b/test/unittest/variables/bvar/tst.arg3-ERROR.sh
> > > > @@ -0,0 +1,172 @@
> > > > +#!/bin/bash
> > > > +#
> > > > +# Oracle Linux DTrace.
> > > > +# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> > > > +# Licensed under the Universal Permissive License v 1.0 as shown at
> > > > +# http://oss.oracle.com/licenses/upl.
> > > > +#
> > > > +
> > > > +##
> > > > +# ASSERTION: The 'arg3' variable in the ERROR probe is correct.
> > > > +#
> > > > +# SECTION: Variables/Built-in Variables/arg3
> > > > +##
> > > > +
> > > > +dtrace=$1
> > > > +
> > > > +DIRNAME="$tmpdir/arg3-ERROR.$$.$RANDOM"
> > > > +mkdir -p $DIRNAME
> > > > +cd $DIRNAME
> > > > +
> > > > +function dump_files {
> > > > + for x in $*; do
> > > > + echo ==== $x
> > > > + cat $x
> > > > + done
> > > > +}
> > > > +
> > > > +# Run DTrace, also collecting disassembly.
> > > > +
> > > > +$dtrace $dt_flags -o D.out -xdisasm=8 -S -qn '
> > > > +BEGIN /* dt_clause_0 */
> > > > +{
> > > > + nerrs = 0;
> > > > +}
> > > > +
> > > > +BEGIN /* dt_clause_1 */
> > > > +{
> > > > + x = (int *)64;
> > > > + y = *x;
> > > > + trace(y);
> > > > +}
> > > > +
> > > > +BEGIN /* dt_clause_2 */
> > > > +{
> > > > + x = (int *)64;
> > > > + y = *x;
> > > > + trace(y);
> > > > +}
> > > > +
> > > > +BEGIN /* dt_clause_3 */
> > > > +{
> > > > + x = (int *)64;
> > > > + y = *x;
> > > > + trace(y);
> > > > +}
> > > > +
> > > > +BEGIN /* dt_clause_4 */
> > > > +{
> > > > + x = (int *)64;
> > > > + y = *x;
> > > > + trace(y);
> > > > +}
> > > > +
> > > > +BEGIN /* dt_clause_5 */
> > > > +{
> > > > + exit(0);
> > > > +}
> > > > +
> > > > +ERROR
> > > > +{
> > > > + /* Report the problematic PC and continue execution. */
> > > > + printf("Error %d at %x\n", ++nerrs, arg3);
> > > > +}
> > > > +' >& disasm.out
> > > > +if [ $? -ne 0 ]; then
> > > > + echo DTrace failure
> > > > + dump_files disasm.out
> > > > + exit 1
> > > > +fi
> > > > +
> > > > +# Parse the disassembly for start PCs for the dt_clause_n.
> > > > +
> > > > +awk '
> > > > +BEGIN { phase = 0 }
> > > > +
> > > > +# Look for disassembly of dtrace:::BEGIN.
> > > > +phase == 0 && /^Disassembly of final program dtrace:::BEGIN:$/ { phase = 1; next }
> > > > +phase == 0 { next }
> > > > +
> > > > +# Look for a blank line, denoting the start of the variable table.
> > > > +phase == 1 && NF == 0 { phase = 2; next }
> > > > +phase == 1 { next }
> > > > +
> > > > +# Look for a blank line, denoting the start of the BPF relocations.
> > > > +phase == 2 && NF == 0 { phase = 3; next }
> > > > +phase == 2 { next }
> > > > +
> > > > +# Report PC for each "dt_clause_n" (or a blank line to finish).
> > > > +phase == 3 && /dt_clause_/ { print $3, $4; next }
> > > > +phase == 3 && NF == 0 { exit(0) }
> > > > +phase == 3 { next }
> > > > +' disasm.out > dt_clause_start_pcs.txt
> > > > +if [ $? -ne 0 ]; then
> > > > + echo ERROR: awk
> > > > + dump_files disasm.out dt_clause_start_pcs.txt
> > > > + exit 1
> > > > +fi
> > > > +
> > > > +# Confirm that we found expected clauses 0-5.
> > > > +
> > > > +for n in 0 1 2 3 4 5; do
> > > > + echo dt_clause_$n >> dt_clause_start_pcs.txt.check
> > > > +done
> > > > +if ! awk '{print $2}' dt_clause_start_pcs.txt | diff - dt_clause_start_pcs.txt.check; then
> > > > + echo ERROR: did not find expected dt_clause_n
> > > > + dump_files disasm.out dt_clause_start_pcs.txt
> > > > +fi
> > > > +
> > > > +# Dump the error PCs to a file.
> > > > +#
> > > > +# BEGIN has 6 clauses (0-5), but 1-4 have the problematic instructions
> > > > +# we are looking for: "y = *x". Each time, we check if address x is 0,
> > > > +# calling dt_probe_error() if necessary. Then we try to dereference x,
> > > > +# again calling dt_probe_error() if necessary. Since x==64, the second
> > > > +# dt_probe_error() is the problematic one. We look for those calls.
> > > > +for n in 1 2 3 4; do
> > > > + # For dt_clause_$n, find the starting PC.
> > > > + pc=`awk '$2 == "dt_clause_'$n'" { print $1 }' dt_clause_start_pcs.txt`
> > > > +
> > > > + # Look for the starting PC and then the second dt_probe_error().
> > > > + awk '
> > > > + BEGIN { phase = 0 }
> > > > +
> > > > + # Look for disassembly of dtrace:::BEGIN.
> > > > + phase == 0 && /^Disassembly of final program dtrace:::BEGIN:$/ { phase = 1; next }
> > > > + phase == 0 { next }
> > > > +
> > > > + # Look for the start PC of dt_clause_n.
> > > > + phase == 1 && ($1 + 0) == '$pc' { phase = 2; next }
> > > > + phase == 1 { next }
> > > > +
> > > > + # Look for the first dt_probe_error() call.
> > > > + phase == 2 && /dt_probe_error/ { phase = 3; next }
> > > > + phase == 2 { next }
> > > > +
> > > > + # Look for the second dt_probe_error() call, reporting the PC and quitting.
> > > > + phase == 3 && /dt_probe_error/ { print $1 + 0; exit(0) }
> > > > + phase == 3 { next }
> > > > + ' disasm.out >> err_pcs.txt
> > > > +done
> > > > +
> > > > +# Do a sanity check on DTrace's error output.
> > > > +
> > > > +awk '/^dtrace: error on enabled probe ID [0-9]* \(ID 1: dtrace:::BEGIN): invalid address \(0x40) in action #[0-9] at BPF pc [0-9]*$/ { print $NF }' \
> > > > + disasm.out > err_pcs.txt.chk1
> > > > +if ! diff -q err_pcs.txt err_pcs.txt.chk1; then
> > > > + echo ERROR: problem with DTrace error output
> > > > + dump_files disasm.out err_pcs.txt err_pcs.txt.chk1
> > > > + exit 1
> > > > +fi
> > > > +
> > > > +# Check the D script output... the arg3 values reported by the dtrace:::ERROR probe.
> > > > +
> > > > +if ! awk 'NF != 0 { print strtonum("0x"$NF) }' D.out | diff -q - err_pcs.txt; then
> > > > + echo ERROR: D script output looks wrong
> > > > + dump_files D.out err_pcs.txt
> > > > + exit 1
> > > > +fi
> > > > +
> > > > +echo success
> > > > +
> > > > +exit 0
> > > > --
> > > > 2.43.5
> > > >