[DTrace-devel] [PATCH 1/3] Rename "path" helper to "tstring1" helper

[Kris Van Hees]

Ignore - I should have commented on v2

On Tue, Apr 19, 2022 at 11:21:58PM -0400, Kris Van Hees via DTrace-devel wrote:
> On Thu, Mar 17, 2022 at 04:37:49PM -0400, eugene.loh--- via DTrace-devel wrote:
> > From: Eugene Loh <eugene.loh at oracle.com>
> > 
> > When the code generator sets up a call to a precompiled BPF function,
> > it emits a lot of boilerplate code.  The implementations of DTrace
> > subroutines typically make such calls.  So there is potential for
> > much code reuse.
> > 
> > The "path" helper is one such attempt.  Since its usefulness is
> > broader than just for a "path name" argument, rename this helper to
> > suggest its broader usage more clearly.
> > 
> > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > ---
> >  libdtrace/dt_cg.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index c40fef41..b78c6e02 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -3316,20 +3316,21 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >  }
> >  
> >  /*
> > - * This function is a helper function for subroutines that take a path
> > + * This function is a helper function for subroutines that take one
> >   * argument and return the tstring resulting from applying a given BPF
> >   * function (passed by name) on it.
> >   */
> >  static void
> > -dt_cg_subr_path_helper(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> > -		       const char *fname)
> > +dt_cg_subr_tstring1_helper(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> 
> I would suggest dt_cg_subr_arg_to_tstring( as function name, to make it more
> clear that it relates to a subroutine that takes 1 argument and returns a
> tstring.
> 
> > +			   const char *fname, int isptr)
> >  {
> >  	dt_ident_t	*idp;
> > -	dt_node_t	*str = dnp->dn_args;
> > +	dt_node_t	*arg = dnp->dn_args;
> >  
> > -	TRACE_REGSET("    subr-path_helper:Begin");
> > -	dt_cg_node(str, dlp, drp);
> > -	dt_cg_check_notnull(dlp, drp, str->dn_reg);
> > +	TRACE_REGSET("    subr-tstring1_helper:Begin");
> 
> s/subr-tstring1_helper/subr-arg-to-tstring/
> 
> > +	dt_cg_node(arg, dlp, drp);
> > +	if (isptr)
> > +		dt_cg_check_notnull(dlp, drp, arg->dn_reg);
> 
> This should be changed to support alloca pointers:
> 
> 	if (isptr)
> 		dt_cg_check_ptr_arg(dlp, drp, arg);
> 
> >  	/*
> >  	 * The result needs be be a temporary string, so we request one.
> > @@ -3346,9 +3347,9 @@ dt_cg_subr_path_helper(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> >  	if (dt_regset_xalloc_args(drp) == -1)
> >  		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >  
> > -	emit(dlp, BPF_MOV_REG(BPF_REG_1, str->dn_reg));
> > -	dt_regset_free(drp, str->dn_reg);
> > -	dt_cg_tstring_free(yypcb, str);
> > +	emit(dlp, BPF_MOV_REG(BPF_REG_1, arg->dn_reg));
> > +	dt_regset_free(drp, arg->dn_reg);
> > +	dt_cg_tstring_free(yypcb, arg);
> 
> While dt_cg_tstring_free() is safe to call for any argument, I think you should
> actually put a if (isptr) before this, simply because we *know* whether the
> argument is a pointer.  The dt_cg_tstring_free() function will still check if
> there is a tstring, but if ptr is not a pointer there is no point to even call
> it.
> 
> >  	emit(dlp,  BPF_MOV_REG(BPF_REG_2, dnp->dn_reg));
> >  
> > @@ -3359,19 +3360,19 @@ dt_cg_subr_path_helper(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> >  	dt_regset_free_args(drp);
> >  	dt_regset_free(drp, BPF_REG_0);
> >  
> > -	TRACE_REGSET("    subr-path_helper:End  ");
> > +	TRACE_REGSET("    subr-tstring1_helper:End  ");
> 
> s/subr-tstring1_helper/subr-arg-to-tstring/
> 
> >  }
> >  
> >  static void
> >  dt_cg_subr_basename(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >  {
> > -	dt_cg_subr_path_helper(dnp, dlp, drp, "dt_basename");
> > +	dt_cg_subr_tstring1_helper(dnp, dlp, drp, "dt_basename", 1);
> >  }
> >  
> >  static void
> >  dt_cg_subr_dirname(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >  {
> > -	dt_cg_subr_path_helper(dnp, dlp, drp, "dt_dirname");
> > +	dt_cg_subr_tstring1_helper(dnp, dlp, drp, "dt_dirname", 1);
> >  }
> >  
> >  /*
> > -- 
> > 2.18.4
> > 
> > 
> > _______________________________________________
> > DTrace-devel mailing list
> > DTrace-devel at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel