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

[Kris Van Hees]

I am not sure about this patch...  First of all, passing the isptr argument
seems unnecessary because the type of dnp should give us that info already.

But more importantly, you are now using this helper function to make BPF
calls to functions with differnet (incompatible) prototypes (once the other
patches are applied also):

	void dt_basename(char *src, char *dst)
	void dt_dirname(char *src, char *dst)
	int dt_lltostr(uint64_t VAL, char *STR)
	uint64_t dt_inet_ntoa(uint8_t *src, char *dst)

And the comment block before the helper states:

    /*
     * 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.
     */

So, there is quite some inconsistency here.  None of the functions you use
this helper for take just one argument.  None of them return a tstring.
And both their argument lists vary (well, the first argument), and the
return type is different.

While it does "work", I do not thiink it is appropriate to do things this
way.  At a minimum, the first argument should probably be uintptr_t so that
you properly capture that it is a value that can be a pointer or an integer.
And ensure that they all have the same return type.

Then, with the comment describing the function updated, this seems to be
both useful and consistent.

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,
> +			   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");
> +	dt_cg_node(arg, dlp, drp);
> +	if (isptr)
> +		dt_cg_check_notnull(dlp, drp, arg->dn_reg);
>  
>  	/*
>  	 * 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);
>  
>  	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  ");
>  }
>  
>  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