[DTrace-devel] [PATCH v2] Add support for array/struct/union to trace()

[Kris Van Hees]

On Mon, Jan 31, 2022 at 03:10:53PM -0500, Eugene Loh via DTrace-devel wrote:
> On 1/28/22 11:27 PM, Kris Van Hees wrote:
> 
> > On Fri, Jan 28, 2022 at 11:01:58PM -0500, Eugene Loh via DTrace-devel wrote:
> > > For the moment, just two questions:
> > > 
> > > Does this look correct?
> > > 
> > > # dtrace -n '
> > > struct {
> > >    int8_t x;
> > > } A;
> > > 
> > > BEGIN {
> > >    A.x = 0x34;
> > >    trace(A);
> > >    exit(0);
> > > }'
> > > dtrace: description '
> > > struct ' matched 1 probe
> > > CPU     ID                    FUNCTION:NAME
> > >    2      1                           :BEGIN
> > > 
> > > No output?
> > I did this with the updated patches I posted and get:
> > 
> > dtrace -n '
> > struct {
> >      int8_t x;
> > } A;
> > 
> > BEGIN {
> >      A.x = 0x34;
> >      trace(A);
> >      exit(0);
> > }'
> > dtrace: description '
> > struct ' matched 1 probe
> > CPU     ID                    FUNCTION:NAME
> >    1      1                           :BEGIN   4
> > 
> > And 0x34 is '4' so that would make sense.  DTrace v1 outputs 52 because of the
> > lack of support for integers of different sizes.
> > 
> > The reason why you get no output (by default) with this patch by itself is that
> > the data is seen with alignment 1 and therefore interpreted as a string.  And
> > that goes wrong.  This is one of the reasons why the string length prefix had
> > to go.  And with the patches to remove that prefux, the trace() action will be
> > able to handle this.
> > 
> > So, not a problem with this patch.
> 
> I confess I do not follow all this.
> 
> E.g., if align=1 is confusing, how about just using align=2?

The thing is that the consumer interprets the data based on the metadata it has
about it, and that does not include the full source type.  It looks at the
alignment to make a determination about this, along with the DT_NF_REF flag to
know whether this might be an array/struct/union (which is an improvement over
v1).  But that leaves us in the situation that a char array and a 1-byte struct
both have the DT_NF_REF flag set, and thus are interpreted the same way.

If I force the alignment to be 2, it would print it as a struct, but the real
question becomes: what *is* the proper behaviour?  How do we decide whether one
is more preferable than the other?

> Also, if this patch purports to add support for struct even though this case
> does not work, then how about adding this test case to this patch (marking
> the test XFAIL and providing an explanation) and then lifting the XFAIL in
> the later patch that fixes the problem.

Sure.

> Finally, even if I use all the posted patches, I still don't understand the
> behavior.  E.g., here is a series of struct tests based on your
> tst.struct.d:
> 
> # dtrace -n 'struct {int32_t a;} st; BEGIN {st.a = 0x34; trace(st);
> exit(0);}'
> dtrace: description 'struct ' matched 1 probe
> CPU     ID                    FUNCTION:NAME
>   3      1                           :BEGIN
>              0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
>          0: 34 00 00 00                                      4...
> 
> 
> # dtrace -n 'struct {int16_t a;} st; BEGIN {st.a = 0x34; trace(st);
> exit(0);}'
> dtrace: description 'struct ' matched 1 probe
> CPU     ID                    FUNCTION:NAME
>   2      1                           :BEGIN
>              0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
>          0: 34 00                                            4.
> 
> 
> # dtrace -n 'struct {int8_t a;} st; BEGIN {st.a = 0x34; trace(st);
> exit(0);}'
> dtrace: description 'struct ' matched 1 probe
> CPU     ID                    FUNCTION:NAME
>   1      1                           :BEGIN   4
> 
> 
> It appears that there is a change of interpretation with the last case.  In
> each case, there is only one integer to print. Nevertheless, the first two
> cases dump a struct (seemingly the correct behavior) while the last one
> dumps neither a struct nor an int but a char.  Why a char?

See above...

> > > And...
> > > 
> > > On 1/28/22 2:25 PM, Kris Van Hees via DTrace-devel wrote:
> > > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > > > @@ -905,6 +905,25 @@ static const uint_t	ldstw[] = {
> > > > +/*
> > > > + * When loading bit-fields, we want to convert a byte count in the range
> > > > + * 1-8 to the closest power of 2 (e.g. 3->4, 5->8, etc).  The clp2() function
> > > > + * is a clever implementation from "Hacker's Delight" by Henry Warren, Jr.
> > > > + */
> > > What is "the range 1-8"?  Can't the input be any 32-bit integer? (Well, any
> > > 64-bit integer that fits into the lowest 32 bits.)  I know this is a
> > > pre-existing comment, but if it's wrong and the code is being touched, we
> > > can fix it.
> > It states "a byte count in the range 1-8", i.e. anywhere from 1 to 8 bytes,
> > because that is the purpose why that function was added.  Granted, we are now
> > using it for other purposes, so I can update the comment to cover the more
> > generic case.
> 
> Great, thanks.  Yeah, if we cared only about 1-8, the code could have been
> simplified.
> 
> > > > +static size_t
> > > > +clp2(size_t x)
> > > > +{
> > > > +	x--;
> > > > +
> > > > +	x |= (x >> 1);
> > > > +	x |= (x >> 2);
> > > > +	x |= (x >> 4);
> > > > +	x |= (x >> 8);
> > > > +	x |= (x >> 16);
> > > > +
> > > > +	return x + 1;
> > > > +}
> > > _______________________________________________
> > > 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