[DTrace-devel] [PATCH v2 01/22] test: Handle dtrace:::ERROR arg3 specially
Eugene Loh
eugene.loh at oracle.com
Fri Aug 30 05:28:37 UTC 2024
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...?
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.
Anyhow, my vote is for a test that checks ERROR and arg3 and correctness.
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 at oracle.com wrote:
>>> From: Eugene Loh <eugene.loh at 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 at 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
>>>
More information about the DTrace-devel
mailing list