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

[Eugene Loh]

On 3/17/22 6:45 PM, Kris Van Hees wrote:

> 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.
Okay.  I'll try to eliminate isptr.
> 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)
Ugh.  Both dt_lltostr() and dt_inet_ntoa() are actually void.  I'll fix 
those comments (which is all they are) in their respective patches.
> 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.
Got it.
> Then, with the comment describing the function updated, this seems to be
> both useful and consistent.
Okay.  I'll clean up the patch series and repost.  Thanks for the comments.
> 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