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

[Eugene Loh]

On 1/31/22 4:06 PM, Kris Van Hees wrote:

> 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?

I suggest using align=2 for structs.  For better or for worse, we've 
reserved align=1 for strings.  The next smallest possible value is 2.  
This means we are sometimes using an unnecessary byte in the output 
stream.  That seems like a meager price to pay for an expedient solution.

Basing the alignment off the size makes no sense to me.  Let's say the 
struct is 8 bytes.  Aligning it on 8 bytes, using up even more room in 
the output buffer than align=2 does, does us no good because the 
consumer just looks at it as a stream of bytes anyhow.

For better or for worse, we are printing structs as byte streams. That's 
even true when the struct comprises only uniformly sized ints.  It's 
true even when the struct comprises only a single int. But then, if that 
single int is 8 bits, we print the struct as a char?  That makes no sense.

Again, it strikes me that the best solution is just to use align=2 for 
structs.  It reduces the amount of padding used for bigger structs and 
most of all it gives a consistent treatment of structs regardless of 
what they are holding.  And it avoids the anomaly of printing a struct 
that is simply an int as a char.

>> 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.

Using align=2 uniformly for all structs regardless of size would mean 
that this test would pass even just with this patch.  One would not need 
to wait for the patch that removes the string prefix.

>> 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...