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

[Kris Van Hees]

On Fri, Apr 02, 2021 at 01:05:49AM -0400, Eugene Loh wrote:
> 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.

I think the confusion is this:

Do you mean that the DTrace on BPF implementation with your patch gives
results that are dufferent from the legacy implementation or not?  If they
differ, I assume that means that the legacy implementation results are not
correct?  If that is the case, then I think you need to state that more
clearly.  The way you stated it makes me also see the option to interpret it
as meaning that the DTrace on BPF implementation had different results from
the legacy implementation but that now with the patch the results are correct
and match what GCC gives.

I think you need to spell out very clearly what difference in results you are
talking about, and identify clearly which results are the correct ones (I hope
DTrace on BPF because if not, this patch is obviously wrong :)).

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

Thankis for testing this!  That is good to know and it certainly gives us
good cause to change the implementation of bitfields to match what GCC
is producing because the support for bitfields is really only truly useful
to make sure we can access data in kernel and userspace structures, i.e.
data that is stored according to how GCC packs things.

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

Sounds good.