[DTrace-devel] [PATCH 2/2] test: Add print() tests
Alan Maguire
alan.maguire at oracle.com
Mon Nov 20 10:40:03 UTC 2023
On 17/11/2023 19:40, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> though perhaps I'm no longer eligible as a reviewer since my name is on
> there now!
>
> All that said:
>
> 1) I still don't understand why these tests aren't squashed into the
> patch that introduces the features being tested. In particular, how can
> that other patch introduce a new feature that isn't being tested
> (admittedly only until the very next patch)?
>
Over the last few years I've been converted to the small, granular patch
model used in the kernel. It has some particular advantages there around
git bisection and using Fixes: tags to identify where a bug was
introduced. In a case like this, separating the functionality and
the tests would allow a more precise identification of where bugs were
fixed using Fixes: tags, and a more precise git bisection. Consider
if we see a test failing in the future, and the cause is too-specific
test output expectation or similar. In such a case, the fix would
be purely about the test. Or in the converse case, if an issue in
the implementation of print() surfaced while the tests still passed,
that would be purely about the implementation. These sorts of issues
crop up frequently upstream in the bpf tree - for example where
a test is flaky - and in such cases separation of functionality + tests
is preferred.
All that said, I don't mind squashing these patches together if that's
normal practice for this project of course.
> 2) It might still be nice to have some err.* tests to check the arg
> checking for print(). I'm attaching some examples. I suppose there
> should also be a test for size-zero errors???
>
good idea; I've added these, thanks for writing them! With respect
to a zero-sized type, I've tried a few things, and (pleasingly) the
D compiler rejects all the ways I tried creating zero-sized types, so I
settled on using a 0-sized type that has had a consistent definition
since 5.4:
$ /usr/local/bin/pahole bpf_raw_tracepoint_args
struct bpf_raw_tracepoint_args {
__u64 args[0]; /* 0 0 */
/* size: 0, cachelines: 0, members: 1 */
};
I've added a test using this and it fails with:
dtrace: failed to compile script
test/unittest/print/err.D_PRINT_SIZE.zero.d: [D_PRINT_SIZE] line 13:
print( ) argument #1 reference has type 'struct bpf_raw_tracepoint_args'
with size 0; cannot print( ) it.
Cleaning took 0.010716189 (0 kprobes, 0 uprobes)
..as expected.
I can send a new version soon but Kris indicated he may have some
feedback too and it'd be good to figure out how best to denote
the "pointer to" issue for patch 1, so probably best to wait a bit.
Thanks again for the review and the help with the tests!
Alan
> 3) It would also be good to have a reg-leak test, given that there
> might be a reg leak. E.g., take one of your simple tests and replicate
> the "print(t)" 20 times or so. Presumably that test will fail. Then
> fix the reg leak and run the test again. This would show the test's
> value for checking reg leaks. (Another reason for tests like this is
> that we run up against limitations of the BPF verifier. It's nice to
> know that new functionality isn't pushing up against those limits. The
> limits I'm talking about are usually due to conditionalized code. Not a
> real risk here. Main motivation is to check for reg leaks.)
>
this is a great idea, and before applying the register free we see the
test fail with
dtrace: failed to compile script
test/unittest/print/tst.print.regleak.d: Insufficient registers to
generate code
> On 11/17/23 06:48, Alan Maguire wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> use local type declaration + alloca() to create custom types for
>> testing. Verify
>>
>> - various types are printed correctly
>> - zeroed values are skipped
>> - we truncate display at offset <value> with -x printsize=<value>
>>
>> Also verify that we can print kernel-defined type correctly.
>>
>> Thanks to Eugene for reworking the tests to use the much
>> cleaner .d/.r format.
>>
>> Co-developed-by: Eugene Loh <eugene.loh at oracle.com>
>> Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
>> ---
>> test/unittest/print/tst.print.local.d | 45 +++++++++++++++++++
>> test/unittest/print/tst.print.local.r | 21 +++++++++
>> test/unittest/print/tst.print.local.trunc.d | 46 ++++++++++++++++++++
>> test/unittest/print/tst.print.local.trunc.r | 12 +++++
>> test/unittest/print/tst.print.local.zeroed.d | 46 ++++++++++++++++++++
>> test/unittest/print/tst.print.local.zeroed.r | 8 ++++
>> test/unittest/print/tst.print.skb.d | 19 ++++++++
>> test/unittest/print/tst.print.skb.r | 14 ++++++
>> 8 files changed, 211 insertions(+)
>> create mode 100644 test/unittest/print/tst.print.local.d
>> create mode 100644 test/unittest/print/tst.print.local.r
>> create mode 100644 test/unittest/print/tst.print.local.trunc.d
>> create mode 100644 test/unittest/print/tst.print.local.trunc.r
>> create mode 100644 test/unittest/print/tst.print.local.zeroed.d
>> create mode 100644 test/unittest/print/tst.print.local.zeroed.r
>> create mode 100644 test/unittest/print/tst.print.skb.d
>> create mode 100644 test/unittest/print/tst.print.skb.r
>>
>> diff --git a/test/unittest/print/tst.print.local.d
>> b/test/unittest/print/tst.print.local.d
>> new file mode 100644
>> index 00000000..ba37be7a
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.local.d
>> @@ -0,0 +1,45 @@
>> +/*
>> + * 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.
>> + */
>> +/* @@nosort */
>> +
>> +#pragma D option quiet
>> +
>> +enum eh { FIRSTVAL = 0, SECONDVAL = 1, THIRDVAL = 3 };
>> +
>> +/* DTrace will not parse nested anonymous structs/unions; also
>> bitfields are not supported. */
>> +struct test_struct {
>> + int a;
>> + char b[5];
>> + union {
>> + __u8 c;
>> + __u64 d;
>> + } u;
>> + struct {
>> + void *e;
>> + uint64_t f;
>> + } s;
>> + bool g;
>> + enum eh h;
>> +};
>> +
>> +BEGIN
>> +{
>> + t = (struct test_struct *)alloca(sizeof (struct test_struct));
>> + t->a = 1;
>> + t->b[0] = 't';
>> + t->b[1] = 'e';
>> + t->b[2] = 's';
>> + t->b[3] = 't';
>> + t->b[4] = '\0';
>> + t->u.d = 123456789;
>> + t->s.e = (void *)0xffff9b7aca7e0000,
>> + t->s.f = 987654321;
>> + t->g = 1;
>> + t->h = SECONDVAL;
>> + print(t);
>> + exit(0);
>> +}
>> diff --git a/test/unittest/print/tst.print.local.r
>> b/test/unittest/print/tst.print.local.r
>> new file mode 100644
>> index 00000000..e634bbea
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.local.r
>> @@ -0,0 +1,21 @@
>> +{ptr} = *
>> + (struct test_struct) {
>> + .a = (int)1,
>> + .b = (char [5]) [
>> + (char)'t',
>> + (char)'e',
>> + (char)'s',
>> + (char)'t',
>> + ],
>> + .u = (union) {
>> + .c = (__u8)21,
>> + .d = (__u64)123456789,
>> + },
>> + .s = (struct) {
>> + .e = (void *){ptr},
>> + .f = (uint64_t)987654321,
>> + },
>> + .g = (bool)1,
>> + .h = (enum eh)SECONDVAL,
>> + }
>> +
>> diff --git a/test/unittest/print/tst.print.local.trunc.d
>> b/test/unittest/print/tst.print.local.trunc.d
>> new file mode 100644
>> index 00000000..2e7beeee
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.local.trunc.d
>> @@ -0,0 +1,46 @@
>> +/*
>> + * 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.
>> + */
>> +/* @@nosort */
>> +
>> +#pragma D option quiet
>> +#pragma D option printsize=8
>> +
>> +enum eh { FIRSTVAL = 1, SECONDVAL = 2, THIRDVAL= 3 };
>> +
>> +/* DTrace will not parse nested anonymous structs/unions; also
>> bitfields are not supported. */
>> +struct test_struct {
>> + int a;
>> + char b[5];
>> + union {
>> + __u8 c;
>> + __u64 d;
>> + } u;
>> + struct {
>> + void *e;
>> + uint64_t f;
>> + } s;
>> + bool g;
>> + enum eh h;
>> +};
>> +
>> +BEGIN
>> +{
>> + t = (struct test_struct *)alloca(sizeof (struct test_struct));
>> + t->a = 1;
>> + t->b[0] = 't';
>> + t->b[1] = 'e';
>> + t->b[2] = 's';
>> + t->b[3] = 't';
>> + t->b[4] = '\0';
>> + t->u.d = 12345678,
>> + t->s.e = (void *)0xfeedfacefeedface;
>> + t->s.f = 100,
>> + t->g = 1;
>> + t->h = FIRSTVAL;
>> + print(t);
>> + exit(0);
>> +}
>> diff --git a/test/unittest/print/tst.print.local.trunc.r
>> b/test/unittest/print/tst.print.local.trunc.r
>> new file mode 100644
>> index 00000000..5b30b661
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.local.trunc.r
>> @@ -0,0 +1,12 @@
>> +{ptr} = *
>> + (struct test_struct) {
>> + .a = (int)1,
>> + .b = (char [5]) [
>> + (char)'t',
>> + (char)'e',
>> + (char)'s',
>> + (char)'t',
>> + ],
>> + ...
>> + }
>> +
>> diff --git a/test/unittest/print/tst.print.local.zeroed.d
>> b/test/unittest/print/tst.print.local.zeroed.d
>> new file mode 100644
>> index 00000000..0372f581
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.local.zeroed.d
>> @@ -0,0 +1,46 @@
>> +/*
>> + * 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.
>> + */
>> +/* @@nosort */
>> +
>> +#pragma D option quiet
>> +
>> +enum eh { FIRSTVAL = 1, SECONDVAL = 2, THIRDVAL= 3 };
>> +
>> +/* DTrace will not parse nested anonymous structs/unions; also
>> bitfields are not supported. */
>> +struct test_struct {
>> + int a;
>> + char b[5];
>> + union {
>> + __u8 c;
>> + __u64 d;
>> + } u;
>> + struct {
>> + void *e;
>> + uint64_t f;
>> + } s;
>> + bool g;
>> + enum eh h;
>> +};
>> +
>> +BEGIN
>> +{
>> + t = (struct test_struct *)alloca(sizeof (struct test_struct));
>> + t->a = 0;
>> + /* initial null should prevent displaying rest of char array */
>> + t->b[0] = '\0';
>> + t->b[1] = 'e';
>> + t->b[2] = 's';
>> + t->b[3] = 't';
>> + t->b[4] = '\0';
>> + t->u.d = 0,
>> + t->s.e = (void *)NULL,
>> + t->s.f = 0,
>> + t->g = 1;
>> + t->h = FIRSTVAL;
>> + print(t);
>> + exit(0);
>> +}
>> diff --git a/test/unittest/print/tst.print.local.zeroed.r
>> b/test/unittest/print/tst.print.local.zeroed.r
>> new file mode 100644
>> index 00000000..c4e87680
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.local.zeroed.r
>> @@ -0,0 +1,8 @@
>> +{ptr} = *
>> + (struct test_struct) {
>> + .b = (char [5]) [
>> + ],
>> + .g = (bool)1,
>> + .h = (enum eh)FIRSTVAL,
>> + }
>> +
>> diff --git a/test/unittest/print/tst.print.skb.d
>> b/test/unittest/print/tst.print.skb.d
>> new file mode 100644
>> index 00000000..f09686e8
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.skb.d
>> @@ -0,0 +1,19 @@
>> +/*
>> + * 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.
>> + */
>> +/* @@nosort */
>> +
>> +#pragma D option quiet
>> +
>> +BEGIN
>> +{
>> + skb = (struct sk_buff *)alloca(sizeof (struct sk_buff));
>> + skb->len = 123;
>> + skb->network_header = 32;
>> + skb->data = (unsigned char *)0xfeedfacefeedface;
>> + print(skb);
>> + exit(0);
>> +}
>> diff --git a/test/unittest/print/tst.print.skb.r
>> b/test/unittest/print/tst.print.skb.r
>> new file mode 100644
>> index 00000000..f389bc4f
>> --- /dev/null
>> +++ b/test/unittest/print/tst.print.skb.r
>> @@ -0,0 +1,14 @@
>> +{ptr} = *
>> + (struct sk_buff) {
>> + .len = (unsigned int)123,
>> + (struct) {
>> + (struct) {
>> + .network_header =
>> (__u16)32,
>> + },
>> + .headers = (struct) {
>> + .network_header =
>> (__u16)32,
>> + },
>> + },
>> + .data = (unsigned char
>> *){ptr},
>> + }
>> +
More information about the DTrace-devel
mailing list