[DTrace-devel] [PATCH v3 1/5] Implement setopt()

[Eugene Loh]

On 9/8/22 15:36, Kris Van Hees wrote:

> On Thu, Sep 08, 2022 at 02:27:32PM -0400, Eugene Loh via DTrace-devel wrote:
>> On 9/8/22 13:12, Kris Van Hees via DTrace-devel wrote:
>>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>>> @@ -1751,6 +1751,17 @@ dt_cg_act_raise(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>>>    static void
>>>    dt_cg_act_setopt(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>>>    {
>>> +	dt_node_t	*opt = dnp->dn_args;
>>> +	dt_node_t	*val = opt->dn_list;
>>> +
>>> +	TRACE_REGSET("setopt(): Begin ");
>>> +	dt_cg_store_val(pcb, opt, DTRACEACT_LIBACT, NULL, DT_ACT_SETOPT);
>>> +
>>> +	/* If no value is given, we default to NULL. */
>>> +	if (val == NULL)
>>> +		val = dt_node_int(0);
>>> +	dt_cg_store_val(pcb, val, DTRACEACT_LIBACT, NULL, DT_ACT_SETOPT);
>>> +	TRACE_REGSET("setopt(): End   ");
>>>    }
>> The point is not that we're defaulting to null but that we're emitting
>> something with non-unit alignment.  The hack is sufficiently, um, nuanced
>> that a clearer and more accurate explanation here would be good.
> Actually, we *are* defaulting to NULL.  And we are using the fact that this
> means we are putting an 4-byte int in the data record rather than a string, and
> that the alignment for strings is 1 byte whereas the 4-byte int uses a 4 byte
> alignment.
>
> The fact that an alignment check is sufficient to distinguish the two is not
> relevant to this code in the code generator - that is something that the
> consumer code is able to use to make distinguishing between the cases easier.

Defaulting to NULL has unclear semantics.  A D user would be logical to 
assume that setting "quiet" to NULL would be like not setting "quiet" at 
all.  Someone reading the code might ask what that 0 value does at all, 
only to to discover that the value -- 0 or otherwise -- is ignored 
completely.  The producer-consumer interaction here is a little funny 
and not otherwise documented.  I think anyone reading the code (other 
than you or now me, I suppose) would benefit from the comment pointing 
out that what we're doing here is writing something with 4-byte 
alignment.  The consumer says nothing about looking for a NULL (which is 
understandable, since it does not look for a NULL) but speaks only about 
checking alignment.

>>> diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
>>> @@ -2150,6 +2150,7 @@ dt_consume_one_probe(dtrace_hdl_t *dtp, FILE *fp, char *data, uint32_t size,
>>>    		     void *arg)
>>>    {
>>>    	dtrace_epid_t		epid;
>>> +	dtrace_datadesc_t	*epd = pdat->dtpda_ddesc;
>>>    	dt_spec_buf_t		tmpl;
>>>    	dt_spec_buf_t		*dtsb;
>>>    	int			specid;
>> It is premature to compute epd here.  I assume this value is incorrect,
>> never used, and overwritten.
> Copy and paste mistake - fixed.

Not in v4?  Will there be a v5?