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

[Kris Van Hees]

On Thu, Sep 08, 2022 at 04:10:49PM -0400, Eugene Loh wrote:
> 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. 

Now you are mixing user and developer viewpoints.  And yes, setopt("quiet",
NULL) actually *enables* quiet mode, as it always has in DTrace.  No change
there.

NULL is a proper thing to pass to represent no string was specified, because
the 2nd argument is indeed NULL.

I doubt anyone reading the code will wonder what that 0 value does, given that
I clearly mention in the comment: "If no value is given, we default to NULL."
What developer would not know that NULL is represented by 0?

And since the value for the option is a string, obviously the 0 is not the
content of that string but rather than absence of a string.

> 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?

Sure, though I already mentioned offline that I fixed that mistake.

r-b please?