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

[Eugene Loh]

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?

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.

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?

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