[DTrace-devel] [PATCH 1/4] Clean up dtrace::: arg handling and testing
Eugene Loh
eugene.loh at oracle.com
Wed May 4 18:11:30 UTC 2022
On 5/4/22 1:27 AM, Kris Van Hees wrote:
> On Wed, May 04, 2022 at 01:01:41AM -0400, eugene.loh--- via DTrace-devel wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Currently, the dtrace provider trampoline sets probe arguments to the
>> values being passed in some function call. However:
>>
>> *) The BEGIN and END probes have no arguments defined. This
>> point is documented explicitly. The probe arguments are
>> certainly not the arguments passed into internal functions
>> BEGIN_probe() and END_probe() -- no such arguments are passed!
>>
>> *) The ERROR probe does have defined arguments, but they are
>> set up in clause code, not in the trampoline.
>>
>> Therefore, do not copy probe args in the dtrace-provider trampoline.
> The legacy implementation has always populated the argument data from
> the registers used for argument passing evne when no such arguments were
> actually supported for the probe. The current implementation remains true to
> that way of doing things and it is harmless.
Harmless is hardly desirable, and useless is undesirable. The code is
useless.
If there is behavior here we want to effect, where is that behavior
defined so that we can test it?
The code could hardly mimic the legacy implementation anyhow. It copies
in the arguments to BEGIN_probe() and END_probe(), both of which are
declared (void).
I don't even see where the legacy code copies in any registers, whether
for argument passing or otherwise. I admit I do not know the legacy
code, but dtrace_begin_probe() seems to copy in mostly just a bunch of
zeroes. So if we really wanted to mimic legacy's undocumented behavior,
we should be zeroing a bunch of this stuff.
> The ERROR probe has its own trampoline code because it gets inlined in the
> probe BPF program. So the trampoline() function in the dtrace provider never
> performs any argument setup anyway for ERROR probes.
>
> So, I would argue that we should continue to copy the values from the argument
> passing registers, if only to remain consistent with the legacy implementation.
>
>> The following tests exercise BEGIN-probe arguments but do not check
>> their values. This is meaningless but not explicitly forbidden.
>> Arguably, the tst.arg*.d tests could be eliminated in favor of
>> their tst.arg*clause.d cousins. For now, we leave these tests alone.
>>
>> demo/builtin/argX.d
>> unittest/builtinvar/tst.arg0.d
>> unittest/builtinvar/tst.arg1.d
>> unittest/builtinvar/tst.arg1to8.d
>>
>> The following tests require argX!=-1 to pass. They date back to a
>> period of development when built-in variables were not yet supported
>> and were all set to -1. In general, however, this test is not reliable:
>> there is no reason why an undefined variable cannot be -1. Support for
>> various defined probe arguments is tested elsewhere. Remove these tests.
>>
>> unittest/variables/bvar/tst.arg[0-9].d
> No, please leave these tests but certainly update them to no longer treat -1
> as a special value. That is long overdue. But we still want to be able to
> test that these builtin variables can be accessed.
Other tests already check that these built-in variables can be
accessed. These particular arg[0-9] tests were introduced simply
because of a specific choice of development strategy that is long behind us.
But I can restore these tests. It's sometimes surprising what a test
can turn up.
>> The following test was illegal in the first place and has never passed.
>> Remove it.
>>
>> unittest/variables/bvar/tst.args.d
> Well, no, let's rewrite it into a test that correctly tries to access the
> arguments through the args[] array.
Okay, but I'm unclear. Even for the BEGIN probe, which does not define
any args, let alone typed args? How about for the END probe? How about
for a probe that actually defines args... particularly typed args?
Should the test check that the values are correct? In general, it would
seem to me that an args test would be a rewrite from scratch and beyond
the scope of this particular patch.
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>> libdtrace/dt_prov_dtrace.c | 1 -
>> test/unittest/variables/bvar/tst.arg0.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg1.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg2.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg3.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg4.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg5.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg6.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg7.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg8.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.arg9.d | 23 -----------------------
>> test/unittest/variables/bvar/tst.args.d | 24 ------------------------
>> 12 files changed, 255 deletions(-)
>> delete mode 100644 test/unittest/variables/bvar/tst.arg0.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg1.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg2.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg3.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg4.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg5.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg6.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg7.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg8.d
>> delete mode 100644 test/unittest/variables/bvar/tst.arg9.d
>> delete mode 100644 test/unittest/variables/bvar/tst.args.d
>>
>> diff --git a/libdtrace/dt_prov_dtrace.c b/libdtrace/dt_prov_dtrace.c
>> index a7fc2cf5..4d401574 100644
>> --- a/libdtrace/dt_prov_dtrace.c
>> +++ b/libdtrace/dt_prov_dtrace.c
>> @@ -156,7 +156,6 @@ static void trampoline(dt_pcb_t *pcb)
>> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_map_update_elem));
>>
>> dt_cg_tramp_copy_regs(pcb, BPF_REG_8);
>> - dt_cg_tramp_copy_args_from_regs(pcb, BPF_REG_8);
>>
>> dt_cg_tramp_epilogue_advance(pcb, act);
>> }
>> diff --git a/test/unittest/variables/bvar/tst.arg0.d b/test/unittest/variables/bvar/tst.arg0.d
>> deleted file mode 100644
>> index a745db0d..00000000
>> --- a/test/unittest/variables/bvar/tst.arg0.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg0' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg0
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg0);
>> - exit(arg0 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg1.d b/test/unittest/variables/bvar/tst.arg1.d
>> deleted file mode 100644
>> index 7d2cd65a..00000000
>> --- a/test/unittest/variables/bvar/tst.arg1.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg1' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg1
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg1);
>> - exit(arg1 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg2.d b/test/unittest/variables/bvar/tst.arg2.d
>> deleted file mode 100644
>> index 08cf98aa..00000000
>> --- a/test/unittest/variables/bvar/tst.arg2.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg2' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg2
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg2);
>> - exit(arg2 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg3.d b/test/unittest/variables/bvar/tst.arg3.d
>> deleted file mode 100644
>> index a7eea667..00000000
>> --- a/test/unittest/variables/bvar/tst.arg3.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg3
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg3);
>> - exit(arg3 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg4.d b/test/unittest/variables/bvar/tst.arg4.d
>> deleted file mode 100644
>> index defc09ce..00000000
>> --- a/test/unittest/variables/bvar/tst.arg4.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg4' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg4
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg4);
>> - exit(arg4 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg5.d b/test/unittest/variables/bvar/tst.arg5.d
>> deleted file mode 100644
>> index 48ae3f12..00000000
>> --- a/test/unittest/variables/bvar/tst.arg5.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg5' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg5
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg5);
>> - exit(arg5 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg6.d b/test/unittest/variables/bvar/tst.arg6.d
>> deleted file mode 100644
>> index cd77dff3..00000000
>> --- a/test/unittest/variables/bvar/tst.arg6.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg6' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg6
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg6);
>> - exit(arg6 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg7.d b/test/unittest/variables/bvar/tst.arg7.d
>> deleted file mode 100644
>> index 314fdb1f..00000000
>> --- a/test/unittest/variables/bvar/tst.arg7.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg7' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg7
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg7);
>> - exit(arg7 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg8.d b/test/unittest/variables/bvar/tst.arg8.d
>> deleted file mode 100644
>> index dc03db3b..00000000
>> --- a/test/unittest/variables/bvar/tst.arg8.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg8' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg8
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg8);
>> - exit(arg8 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.arg9.d b/test/unittest/variables/bvar/tst.arg9.d
>> deleted file mode 100644
>> index 4456bba0..00000000
>> --- a/test/unittest/variables/bvar/tst.arg9.d
>> +++ /dev/null
>> @@ -1,23 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 'arg9' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/arg9
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(arg9);
>> - exit(arg9 != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> diff --git a/test/unittest/variables/bvar/tst.args.d b/test/unittest/variables/bvar/tst.args.d
>> deleted file mode 100644
>> index 09536766..00000000
>> --- a/test/unittest/variables/bvar/tst.args.d
>> +++ /dev/null
>> @@ -1,24 +0,0 @@
>> -/*
>> - * Oracle Linux DTrace.
>> - * Copyright (c) 2020, 2021, 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 */
>> -
>> -/*
>> - * ASSERTION: The 'args' variable can be accessed and is not -1.
>> - *
>> - * SECTION: Variables/Built-in Variables/args
>> - */
>> -
>> -#pragma D option quiet
>> -
>> -BEGIN {
>> - trace(args);
>> - exit(args != -1 ? 0 : 1);
>> -}
>> -
>> -ERROR {
>> - exit(1);
>> -}
>> --
>> 2.18.4
>>
>>
>> _______________________________________________
>> DTrace-devel mailing list
>> DTrace-devel at oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list