[DTrace-devel] [PATCH v5] Implement TLS variables

[Eugene Loh]

Old email...

On 12/2/21 1:18 AM, Kris Van Hees wrote:
> On Wed, Dec 01, 2021 at 08:25:25PM -0500, Eugene Loh via DTrace-devel wrote:
>> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> Thanks.
>
>> Incidentally, I'm not sure what to make of the following:
>>      # cat z.d
>>      struct foo {
>>          int x1, x2, x3, x4;
>>      };
>>      self struct foo a;
>>      struct foo g;
>>      BEGIN {
>>          g.x1 = g.x2 = g.x3 = g.x4 = 31415926;
>>          printf("%d %d %d %d\n", g.x1, g.x2, g.x3, g.x4);
>>          g = self->a;
>>          printf("%d %d %d %d\n", g.x1, g.x2, g.x3, g.x4);
>>          exit(0);
>>      }
>>      # dtrace -qs z.d
>>      31415926 31415926 31415926 31415926
>>      0 0 0 0
>> Since self->a is unassigned, get_tvar() presumably returns a NULL pointer.
>> Why doesn't g=self->a then have problems?
> It was not having problems because bpf_probe_read() apparently clears its
> destination when the source is NULL (or presumably any invalid pointer).
> And we end up doing that bpf_probe_read() because a load of an unassigned
> TLS variable returns 0.  But when dt_cg_load_var() is called for by-ref var
> loading, we should be checking the value in case it is NULL (and if it is
> NULL, trigger a fault).
>
> Fixed.

Thanks for fixing this.

Unfortunately:

1)  This test case was not added to the test suite.  (Or was it?)

2)  A later patch (978f2c0a4fba "Avoid checking for NULL pointers 
multiple times") seems to have ripped the fix back out.

I must say, I'm confused by the explanation in the later patch:

     The dt_cg_load_var() function included a check to see that a loaded
     pointer value was not NULL (and to generate a probe error if it was).
     However, callers of this function are already checking for NULL before
     using the pointer (if such a check is necessary).  Therefore, this
     does not need to be done in dt_cg_load_var().

I do not think the claim is true.  E.g., in this test case, the 
right-hand side is an uninitialized TLS variable.  If we do not check 
ptr==NULL in dt_cg_load_var(), then we go back up to its only caller, 
which is DT_TOK_IDENT processing.  We then go up to DT_TOK_ASGN 
processing, drop down into dt_cg_asgn_op() and ... there are now lots of 
code paths to check.  Specifically, for our test case, we go into 
dt_cg_store_var(), then dt_cg_memcpy(), and none of these have the 
ptr==NULL check.

So, where do we want these checks?

There seem to be many ways of handling this, but how would it be if I 
post a patch that:

*)  restores the NULL check that 978f2c0a4fba eliminated

*)  adds the above test case to the the test suite