[DTrace-devel] [PATCH v3 1/3] Introduce temporary string space for string manipulation functions

[Kris Van Hees]

On Fri, Sep 03, 2021 at 06:11:42PM -0400, Eugene Loh wrote:
> A few questions/comments...
> 
> On 9/3/21 3:52 PM, Kris Van Hees wrote:
> > Functions must request a temporary string slot and have it associated
> > with their return value using a statement like:
> >      dt_cg_tstring_alloc(pcb, dnp);
> 
> Just to clarify your earlier response... if a function needs scratch 
> space for computation (but not for the output result), that's beyond the 
> scope of this patch.  Did I get that right?

Yes.

> > Functions that may be receiving a temporary string as argument (which
> > is any function or action that accept string arguments) must free the
> > temporary string once it is no longer needed.  This should be done
> > using code like:
> >
> >      if (dnp->dn_tstring)
> >          dt_cg_tstring_free(pcb, dnp);
> 
> How about putting that dnp->dn_tstring check inside of 
> dt_cg_tstring_free()?  That would save some boilerplate code.

Hm, I think that would obfuscate things a little bit.  It really shouldn't
be called unless there is a tstring.

> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > @@ -276,20 +269,25 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> >   
> >   	/*
> >   	 * The size of the map value (a byte array) is the sum of:
> > -	 *	- size of the DTrace context, rounded up to the nearest
> > +	 *	- size of the DTrace machine state, rounded up to the nearest
> >   	 *	  multiple of 8
> >   	 *	- 8 bytes padding for trace buffer alignment purposes
> >   	 *	- maximum trace buffer record size, rounded up to the
> >   	 *	  multiple of 8
> >   	 *	- the greater of:
> >   	 *		+ the maximum stack trace size
> > -	 *		+ three times the maximum string size
> > +	 *		+ four times the maximum string size (incl. length)
> > +	 *		  plus the maximum string size (to accomodate the BPF
> > +	 *		  verifier)
> >   	 */
> >   	memsz = roundup(sizeof(dt_mstate_t), 8) +
> >   		8 +
> >   		roundup(dtp->dt_maxreclen, 8) +
> >   		MAX(sizeof(uint64_t) * dtp->dt_options[DTRACEOPT_MAXFRAMES],
> > -		    3 * dtp->dt_options[DTRACEOPT_STRSIZE]);
> > +		    DT_TSTRING_SLOTS *
> > +			(DT_STRLEN_BYTES + dtp->dt_options[DTRACEOPT_STRSIZE] +
> > +		    dtp->dt_options[DTRACEOPT_STRSIZE]
> > +		));
> 
> Thanks for the clarification, but I'm still a little puzzled.  Do you 
> want the extra strsize overall or per slot?  The code above seems to 
> have it per slot.

It should be extra overall, not per slot.  I have a misplaced ) there.  Fixing.

> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -2841,6 +2912,8 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >   			dt_cg_arglist(idp, dnp->dn_left->dn_args, dlp, drp);
> >   
> >   		dt_cg_store_var(dnp, dlp, drp, idp);
> > +		if (dnp->dn_right->dn_kind == DT_NODE_FUNC)
> > +			dt_cg_tstring_free(yypcb, dnp->dn_right);
> >   	} else {
> 
> I don't understand this stuff, but the usual ->dn_tstring check is not 
> performed here.  Is that okay?  Of course, no problem if the check ends 
> up being moved into tstring_free().

Woops, that should be there for sure.  Fixing.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel