[DTrace-devel] [PATCH 1/4] Clean up dtrace::: arg handling and testing
Kris Van Hees
kris.van.hees at oracle.com
Wed May 4 18:52:55 UTC 2022
On Wed, May 04, 2022 at 02:11:30PM -0400, Eugene Loh via DTrace-devel wrote:
> 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.
That depends but that is not the issue here.
> 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.
Yes, in this case we should not copy the argument values from the registers
and instead populate zeros. I forgot that the legacy code explicitly passes
zeros to the BEGIN and END probes in the dtrace_probe() call, and that
dtrace_probe() function then copies the argument values (all zeros in this
case) to the cached argument data in the mstate.
> > 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.
In general we should, but not in this case, indeed.
> > > 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.
Those test do serve a purpose - they ensure that it is possible to use those
builtin variables. And in fact, we ought to add tests that test that it is
*not* possible (or even allowed) to assign values to these variables (and all
builtin variables, actually).
> > > 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.
Well, it doesn't matter whether this test is for a probe that passes values or
not. The point is to test that you can access all elements of the array (and
not beyond the predefined size). Since I will be implementing that variable
in the next few days, just let the test be and I will deal with it along with
the implementation code. That seems to be the most appropriate way to handle
this test.
> > > 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
>
> _______________________________________________
> 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