[DTrace-devel] [PATCH 4/6] Fix bit-field operations

[Eugene Loh]

On 3/31/21 10:50 PM, Kris Van Hees wrote:

> On Fri, Mar 19, 2021 at 01:45:32PM -0400, eugene.loh at oracle.com wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Fix bugs in the dt_cg_field_[set|get] functions.
>>
>> Add tests for the functionality.  Note that, due to an earlier patch
>> ("6d9c2398 Provide a more consistent implementation of the trace() action"),
>> results for signed bit fields with a nonzero sign bit differ from the
>> incorrect results of DTv1.  Current results match GCC.
> This is confusing.  I don't think we need to make any reference to DTrace v1
> (the original DTrace port to Linux that we did years ago).  The tests do not
> exist in the earlier version, so there is no need to compare.  You could
> mention that you verified the behaviour with this patch and that it matches
> the legacy implementation or something like that.

I'm confused.  The results do NOT match the legacy implementation, which 
is bad because it's missing the patch in question.  I'm not saying my 
wording is not confusing, but only that I don't understand your 
recommendation.

>> (Also note that bit fields are not packed together as efficiently as
>> possible.  Rather, following legacy DTrace behavior, each bit field is
>> aligned to the size of the next largest integer type.  Thus, simplifications
>> in BPF code generation can be imagined.  Or, bit fields should be packed more
>> tightly together, in a departure from earlier DTrace implementations but
>> bringing behavior in line with common user expectations.)
> One consideration here os whether this has an impact on accessing kernel
> structures.  If a kernel structure contains a bit field and we use that
> datatype (or the structure) to declare a D variable, can we access the
> content of the bit field in the kernel structure correctly?  And also any
> other data, e.g. other bit fields?  If GCC packs them together, but D is
> expecting the datatype to have them stored differently we will run into
> trouble.  This is something you may want to test on the legacy implementation
> because I don't think we can really access the content of kernel structures
> just yet.  But knowing whether there is an issue here would help fine tune
> the above paragraph in the commit message.

Consider the following test program.  All it does is prepare some 
"struct perf_event_attr", which has some bit fields.  Most of the struct 
is zero.  It then calls perf_event_open(), which won't really care much 
about the struct contents since pid=cpu=-1 is not legal.

         % cat perf_event_open.c
         #include <stdio.h>
         #include <string.h>
         #include <sys/syscall.h>
         #include <linux/perf_event.h>

         int main(int c, char **v) {
             struct perf_event_attr attr;

             memset(&attr, 0, sizeof(struct perf_event_attr));
             attr.type = 0xfeedface;
             attr.size = 0xdeadbeef;
             attr.config = 0x1234567887654321ll;
             attr.sample_freq = 0xcafecafecafecafell;
             attr.sample_type = 0xaabbccddeeffabcdll;
             attr.read_format = 0x9876543210fedcball;

             attr.wakeup_events = 0x11223344;
             attr.bp_type       = 0x55667788;

             /* using pid = cpu = -1 ensures this call will return an 
error */
             syscall(__NR_perf_event_open, &attr, -1, -1, -1, 0);

             return 0;
         }
         % gcc perf_event_open.c

Now we run a D script.  At perf_event_open:entry, it examines the struct 
contents.  Specifically, it dumps out the first fields as a sanity test 
and everything checks out okay:

         # /usr/sbin/dtrace -qn '
           syscall:vmlinux:perf_event_open:entry {
               p = (struct perf_event_attr *)arg0;
               printf("%x\n", p->type);
               printf("%x\n", p->size);
               printf("%x\n", p->config);
               printf("%x\n", p->sample_freq);
               printf("%x\n", p->sample_type);
               printf("%x\n", p->read_format);
            /* printf("%x\n", p->disabled); */
               exit(0);
           }' -c ./a.out
         feedface
         deadbeef
         1234567887654321
         cafecafecafecafe
         aabbccddeeffabcd
         9876543210fedcba

If we try a bit field (by uncommenting that line in the D script), we 
get failure:
         dtrace: invalid probe specifier
         syscall:vmlinux:perf_event_open:entry
         {......}: cg: bad field: off 320 type <1356> bits 18109968

which is presumably in dt_cg_field_get().  So, I start debugging... but 
I cannot reproduce the behavior!  Both the installed /usr/sbin/dtrace 
and my debugging dtrace report version 1.6.4.  My debugging build gets 
to dt_cg_node and looks at DT_TOK_DOT but decides that DT_NF_BITFIELD is 
not set and therefore does not call dt_cg_field_get().  So, it does not 
abort with the "bad field" messages.  It happily prints all the bit 
fields I want, always claiming they're 0x20, which is wrong.

So I don't quite know what's going on, but that's the status so far.  
DTv1 is not correctly reporting bit fields.

Back to the subject of the commit message, how about the following in 
place of the last paragraph?

     (Also note that bit fields are not packed together as efficiently as
     possible.  Rather, following legacy DTrace behavior, each bit field is
     aligned to the size of the next largest integer type.  This is a bug,
     preventing users from reading bit fields in kernel structs, but it is
     outside the scope of this patch.)