[DTrace-devel] [PATCH 02/14] Replace not-NULL test with more general runtime test

[Eugene Loh]

On 5/2/23 11:32, Nick Alcock wrote:

> On 2 May 2023, eugene loh outgrape:
>
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> When we dereference a pointer, it should generally be safe to
>> dereference D (including alloca) pointers, even if a more robust
>> mechanism is needed to guard against dereferencing unsafe pointers
>> in the more general case.  We do, however, check whether a pointer
>> is NULL.
>>
>> There are times, however, when even a D pointer cannot safely be
>> dereferenced.  This will especially be true once NULL string pointers
>> will be supported.
> Err, why not soup up the not-null check to handle null strings, then?
> (And what other cases are there?)

At this point in the patch series, it has nothing to do with NULL 
strings.  In fact, this patch also removes the xfail on a test that is 
unrelated to NULL strings.  All the test does is stores a pointer in an 
associative array, reads it back, and tries to dereference it.  At this 
point, what check can we perform to validate the pointer?  Checking for 
NULL is too weak.  So we just feed the address to the BPF helper and 
check whether that's successful.

Maybe I'm missing something.  Feel free to make a more detailed suggestion.

>> So drop the NULL-pointer check and implement dereferencing with a
>> more general scalar load.
> Reviewed-by: Nick Alcock <nick.alcock at oracle.com>
>
> with one question below (but with my reviewed-by because the answer is
> almost certainly "of course it's fine, you fool" in which case this
> commit seems like a good idea).
>
> The historical background here is that in the past alloca pointers had a
> unique nonzero null representation which needed special checking. They
> don't any more, so a great many of these notnull checks can probably be
> generalized.
>
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>>   libdtrace/dt_cg.c                   | 6 +-----
>>   test/unittest/pointers/tst.basic2.d | 3 +--
>>   2 files changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> index 483603ef..c1fd46c0 100644
>> --- a/libdtrace/dt_cg.c
>> +++ b/libdtrace/dt_cg.c
>> @@ -5884,7 +5884,6 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>   			uint_t	op;
>>   			ssize_t	size;
>>   
>> -			dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
>>   			op = dt_cg_ldsize(dnp, ctfp, dnp->dn_type, &size);
>>   
>>   			/*
>> @@ -5899,10 +5898,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>   						 dnp->dn_reg);
>>   			}
>>   
>> -			if (dnp->dn_child->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
>> -				emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
>> -			else
>> -				dt_cg_load_scalar(dnp, op, size, dlp, drp);
>> +			dt_cg_load_scalar(dnp, op, size, dlp, drp);
> This does turn a simple load asm instruction into a helper call for
> things for which a load instruction should always work (if non-null!).

Which things?  Anyhow, for a narrower set of things than we were 
allowing.  That's why the test was failing.  Just checking for 
ALLOCA/DPTR was insufficient.  Need a more stringent test.

> Do we mind?

Using the dt_cg_load_scalar() mechanism "all the time" instead of the 
nonnull check simplifies code generation.  I don't know how to 
demonstrate the increased cost of using the helper function, though I 
agree theoretically there is such a cost.  Anyhow, I think it's an 
acceptable solution and do not know a reasonable alternative... and the 
simpler code generation is nice.

> Is this always even possible?

What is "this"?