[DTrace-devel] [PATCH 6/7] dlibs, test: reduce complexity of tests
Nick Alcock
nick.alcock at oracle.com
Fri Nov 3 13:24:00 UTC 2023
On 31 Oct 2023, Kris Van Hees via DTrace-devel told this:
> Some tests depended on kernel variable type information that is not
> currently available when only BTF type data can be found. Use
> explicit casting.
Some of these tests were in part meant to check that variables were
available and work. I think we should probably XFAIL those
when BTF is in use.
But in general, see below. A good few tests that should be split into
two variants, one run only when CTF is present without casts, one always
run with casts. (Sorry I'm being so critical, but these tests have been
the last thing finding CTF-related bugs on too many occasions for me to
want to break the lot.)
I would also like to have some way to *prevent* CTF-to-BTF fallback so
that if CTF does silently break we don't always unconditionally paper it
over when running the testsuite: it is still a major way for me to make
sure CTF is still working...
> Some tests should be skipped when there is no CTF archive.
Ack (or XFAILed, I'd say).
> diff --git a/libdtrace/io.d.in b/libdtrace/io.d.in
> index 7d270426..ae867018 100644
> --- a/libdtrace/io.d.in
> +++ b/libdtrace/io.d.in
> @@ -127,8 +127,11 @@ translator devinfo_t < struct bio *B > {
> dev_major = B->__disk_chk == NULL ? 0 : getmajor(B->__bio_part_dev);
> dev_minor = B->__disk_chk == NULL ? 0 : getminor(B->__bio_part_dev);
> dev_instance = 0;
> - dev_name = B->__disk_chk == NULL ? "nfs" : stringof(`major_names[
> - getmajor(B->__bio_part_dev) % 255]->name);
> + dev_name = B->__disk_chk == NULL
> + ? "nfs"
> + : stringof(((struct blk_major_name **)`major_names)[
> + getmajor(B->__bio_part_dev) % 255
> + ]->name);
But that's just horrible. Is there really no better way? Can't we use
the CTF info if it's there?
> diff --git a/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.d b/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.d
> index 6ce31c38..7443dfb1 100644
> --- a/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.d
> +++ b/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.d
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2012, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2023, 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.
> */
> @@ -16,6 +16,6 @@
>
> BEGIN
> {
> - x[123] = *`cad_pid;
> - x[456] = `max_pfn;
> + x[123] = curthread;
> + x[456] = pid;
> }
> diff --git a/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.r b/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.r
> index 4ebf0357..d1aa3877 100644
> --- a/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.r
> +++ b/test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.r
> @@ -1,2 +1,2 @@
> -- @@stderr --
> -dtrace: failed to compile script test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.d: [D_OP_INCOMPAT] line 20: operands have incompatible types: "struct pid" = "long unsigned int"
> +dtrace: failed to compile script test/unittest/assocs/err.D_OP_INCOMPAT.dupgtype.d: [D_OP_INCOMPAT] line 20: operands have incompatible types:
This test has in the past detected bugs where variable type names are
returned incorrectly. This won't now work. Can we duplicate this test,
one of which is as it was (and XFAILed in the absence of CTF), the other
of which does this? That'll test both cases :)
> diff --git a/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.d b/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.d
> index 22cdee17..1e00b5b0 100644
> --- a/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.d
> +++ b/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.d
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2012, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2023, 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.
> */
> @@ -16,6 +16,6 @@
>
> BEGIN
> {
> - self->x[123] = *`cad_pid;
> - self->x[456] = `max_pfn;
> + self->x[123] = curthread;
> + self->x[456] = pid;
> }
> diff --git a/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.r b/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.r
> index 623c6b8b..537d0ea3 100644
> --- a/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.r
> +++ b/test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.r
> @@ -1,2 +1,2 @@
> -- @@stderr --
> -dtrace: failed to compile script test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.d: [D_OP_INCOMPAT] line 20: operands have incompatible types: "struct pid" = "long unsigned int"
> +dtrace: failed to compile script test/unittest/assocs/err.D_OP_INCOMPAT.dupttype.d: [D_OP_INCOMPAT] line 20: operands have incompatible types:
(But the incompatible type code is shared, so we don't need to duplicate
this test too -- any bugs such duplication found would be found by the
previous change.)
> diff --git a/test/unittest/codegen/tst.kernel_read_deref_struct.d b/test/unittest/codegen/tst.kernel_read_deref_struct.d
> index 5baf604c..c8ad5d3d 100644
> --- a/test/unittest/codegen/tst.kernel_read_deref_struct.d
> +++ b/test/unittest/codegen/tst.kernel_read_deref_struct.d
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 2023, 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.
> */
> @@ -9,7 +9,7 @@
>
> BEGIN
> {
> - trace(*`cad_pid);
> + trace(*(struct pid *)`cad_pid);
> exit(0);
> }
Don't we care about codegen getting CTF types right without explicit
casts? (Or is there a distinct test for that?)
> diff --git a/test/unittest/funcs/tst.subr.d b/test/unittest/funcs/tst.subr.d
> index 4054036b..9f7203c0 100644
> --- a/test/unittest/funcs/tst.subr.d
> +++ b/test/unittest/funcs/tst.subr.d
> @@ -39,13 +39,13 @@
> #define NUM_UNIMPLEMENTED 7
>
> INTFUNC(rand())
> -INTFUNC(mutex_owned(&`bpf_verifier_lock))
> -INTFUNC(mutex_owner(&`bpf_verifier_lock))
> -INTFUNC(mutex_type_adaptive(&`bpf_verifier_lock))
> -INTFUNC(mutex_type_spin(&`bpf_verifier_lock))
> -INTFUNC(rw_read_held(&`tasklist_lock))
> -INTFUNC(rw_write_held(&`tasklist_lock))
> -INTFUNC(rw_iswriter(&`tasklist_lock))
> +INTFUNC(mutex_owned((struct mutex *)&`bpf_verifier_lock))
> +INTFUNC(mutex_owner((struct mutex *)&`bpf_verifier_lock))
> +INTFUNC(mutex_type_adaptive((struct mutex *)&`bpf_verifier_lock))
> +INTFUNC(mutex_type_spin((struct mutex *)&`bpf_verifier_lock))
> +INTFUNC(rw_read_held((rwlock_t *)&`tasklist_lock))
> +INTFUNC(rw_write_held((rwlock_t *)&`tasklist_lock))
> +INTFUNC(rw_iswriter((rwlock_t *)&`tasklist_lock))
This has *definitely* found CTF-related bugs in the past. Please
duplicate, one CTF-only without explicit casts, and then this one.
> diff --git a/test/unittest/inline/tst.InlineDataAssign.d b/test/unittest/inline/tst.InlineDataAssign.d
> index f7bb071c..85061a4d 100644
> --- a/test/unittest/inline/tst.InlineDataAssign.d
> +++ b/test/unittest/inline/tst.InlineDataAssign.d
> @@ -39,7 +39,7 @@ inline double new_double = 2.34567890;
> inline long double new_long_double = 3.567890123;
> */
>
> -inline unsigned long * pointer = &`max_pfn;
> +inline unsigned long * pointer = (unsigned long *)&`max_pfn;
>
> BEGIN
> {
> diff --git a/test/unittest/inline/tst.InlineExpression.d b/test/unittest/inline/tst.InlineExpression.d
> index de215ed2..ea7a7a87 100644
> --- a/test/unittest/inline/tst.InlineExpression.d
> +++ b/test/unittest/inline/tst.InlineExpression.d
> @@ -41,7 +41,7 @@ inline double new_double = 2.34567890;
> inline long double new_long_double = 3.567890123;
> */
>
> -inline unsigned long * pointer = &`max_pfn;
> +inline unsigned long * pointer = (unsigned long *)&`max_pfn;
> inline int result = 3 > 2 ? 3 : 2;
>
> BEGIN
> diff --git a/test/unittest/inline/tst.InlineTypedef.d b/test/unittest/inline/tst.InlineTypedef.d
> index f348cca1..ac8c2452 100644
> --- a/test/unittest/inline/tst.InlineTypedef.d
> +++ b/test/unittest/inline/tst.InlineTypedef.d
> @@ -22,7 +22,7 @@ typedef char new_char;
> inline new_char char_var = 'c';
>
> typedef unsigned long * pointer;
> -inline pointer p = &`max_pfn;
> +inline pointer p = (pointer)&`max_pfn;
Ditto these three -- but particularly the last, which exercises code
paths the other two do not.
> --- a/test/unittest/lexer/tst.keyword-member.d
> +++ b/test/unittest/lexer/tst.keyword-member.d
> @@ -13,13 +13,13 @@
>
> BEGIN
> {
> - printf("%p\n", &`cgrp_dfl_root.cgrp.self);
> - printf("%p\n", &`cgrp_dfl_root.cgrp.self.cgroup->self);
> - printf("%p\n", `cgrp_dfl_root.cgrp.self.cgroup->self.cgroup);
> - printf("%p\n", &`cgrp_dfl_root.cgrp./* foo */self.cgroup->self);
> - printf("%p\n", `cgrp_dfl_root.cgrp.self.cgroup->/* foo */self.cgroup);
> - printf("%p\n", &`cgrp_dfl_root.cgrp.self/* foo */.cgroup->self);
> - printf("%p\n", `cgrp_dfl_root.cgrp.self.cgroup->self/* foo */.cgroup);
> + printf("%p\n", &(*(struct cgroup_root *)&`cgrp_dfl_root).cgrp.self);
> + printf("%p\n", &(*(struct cgroup_root *)&`cgrp_dfl_root).cgrp.self.cgroup->self);
> + printf("%p\n", (*(struct cgroup_root *)&`cgrp_dfl_root).cgrp.self.cgroup->self.cgroup);
> + printf("%p\n", &(*(struct cgroup_root *)&`cgrp_dfl_root).cgrp./* foo */self.cgroup->self);
> + printf("%p\n", (*(struct cgroup_root *)&`cgrp_dfl_root).cgrp.self.cgroup->/* foo */self.cgroup);
> + printf("%p\n", &(*(struct cgroup_root *)&`cgrp_dfl_root).cgrp.self/* foo */.cgroup->self);
> + printf("%p\n", (*(struct cgroup_root *)&`cgrp_dfl_root).cgrp.self.cgroup->self/* foo */.cgroup);
Just skip this in !CTF cases instead, or duplicate it -- adding the
extra bracketing for casts changes the lexing in ways that might well
disturb it (particularly for hte earlier ones).
> --- a/test/unittest/options/err.ctfpath.r
> +++ b/test/unittest/options/err.ctfpath.r
> @@ -1,3 +1 @@
> --- @@stderr --
> -dtrace: script 'test/unittest/options/err.ctfpath.d' matched 1 probe
> -dtrace: could not enable tracing: Module does not contain any CTF data
> +Cannot open CTF archive /dev/null: File is not in CTF or ELF format.; trying BTF.
That message doesn't look very nice. Maybe you should shave the trailing
dot off, or rephrase it to 'Trying BTF.'
> diff --git a/test/unittest/options/err.ctfpath.sh b/test/unittest/options/err.ctfpath.sh
> new file mode 100755
> index 00000000..d0898729
> --- /dev/null
> +++ b/test/unittest/options/err.ctfpath.sh
> @@ -0,0 +1,18 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2023, 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.
> +#
> +
> +dtrace=$1
> +
> +$dtrace $dt_flags -xdebug -xctfpath=/dev/null -n 'BEGIN { exit(0); }' |&
> + awk '/Cannot open CTF archive \/dev\/null/ {
> + sub(/^[^:]+: /, "");
> + print;
> + exit(1);
> + }'
> +
> +exit $?
What happens if the kernel has no CTF *or* BTF? This test doesn't allow
for that, and it would cause a test failure -- and we probably do want
it to pass in that case, and to test that possibility.
> diff --git a/test/unittest/pointers/tst.GlobalVar.d b/test/unittest/pointers/tst.GlobalVar.d
> index 33af2026..0112ab91 100644
> --- a/test/unittest/pointers/tst.GlobalVar.d
> +++ b/test/unittest/pointers/tst.GlobalVar.d
> @@ -19,7 +19,7 @@
> BEGIN
> {
> i = 0;
> - pfnAddress = &`max_pfn;
> + pfnAddress = (unsigned long *)&`max_pfn;
> pfnValue = *pfnAddress;
> printf("Address of max_pfn: %x\n", (int)pfnAddress);
> printf("Value of max_pfn: %d\n", pfnValue);
> diff --git a/test/unittest/pointers/tst.basic1.d b/test/unittest/pointers/tst.basic1.d
> index 75ec0489..b63597f5 100644
> --- a/test/unittest/pointers/tst.basic1.d
> +++ b/test/unittest/pointers/tst.basic1.d
> @@ -18,7 +18,7 @@
>
> BEGIN
> {
> - pfnAddress = &`max_pfn;
> + pfnAddress = (unsigned long *)&`max_pfn;
> pfnValue = *pfnAddress;
> printf("Address of max_pfn: %x\n", (int)pfnAddress);
> printf("Value of max_pfn: %d\n", pfnValue);
> diff --git a/test/unittest/pointers/tst.basic2.d b/test/unittest/pointers/tst.basic2.d
> index c17f9251..f026a10d 100644
> --- a/test/unittest/pointers/tst.basic2.d
> +++ b/test/unittest/pointers/tst.basic2.d
> @@ -18,7 +18,7 @@
>
> BEGIN
> {
> - assoc_array["pfnAddress"] = &`max_pfn;
> + assoc_array["pfnAddress"] = (unsigned long *)&`max_pfn;
> pfnValue = *(assoc_array["pfnAddress"]);
> printf("Address of max_pfn: %x\n", (int)assoc_array["pfnAddress"]);
> printf("Value of max_pfn: %d\n", pfnValue);
These three have also shown up actual bugs: please duplicate at least
one as a non-casting no-CTF version.
> diff --git a/test/unittest/scalars/err.D_OP_INCOMPAT.dupgtype.d b/test/unittest/scalars/err.D_OP_INCOMPAT.dupgtype.d
> index 5a2f51bc..fd4452c7 100644
> --- a/test/unittest/scalars/err.D_OP_INCOMPAT.dupgtype.d
> +++ b/test/unittest/scalars/err.D_OP_INCOMPAT.dupgtype.d
> @@ -1,6 +1,6 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2012, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2023, 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.
> */
> @@ -15,6 +15,6 @@
>
> BEGIN
> {
> - x = *`cad_pid;
> - x = `max_pfn;
> + x = curthread;
> + x = pid;
> }
Ditto, as with the assocs version.
> diff --git a/test/unittest/tracemem/err.D_TRACEMEM_ADDR.badaddr.d b/test/unittest/tracemem/err.D_TRACEMEM_ADDR.badaddr.d
> index 0ad7d4e4..478a0f47 100644
> --- a/test/unittest/tracemem/err.D_TRACEMEM_ADDR.badaddr.d
> +++ b/test/unittest/tracemem/err.D_TRACEMEM_ADDR.badaddr.d
> @@ -14,5 +14,11 @@
>
> BEGIN
> {
> - tracemem(`init_mm, 123);
> + tracemem(*curthread, 123);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> }
Doesn't all this end up removing every single test that actually
verifies that CTF variables have an address that points to something
meaningful? We probably want at least one of those...
--
NULL && (void)
More information about the DTrace-devel
mailing list