[DTrace-devel] [PATCH v4 19/20] Implement a framework for D subroutines incl. strlen() and speculation() implementation

[Eugene Loh]

Actually, I'm also curious how this patch fares with the test suite.  I 
might not have the right versions of things, but for me this patch led 
to FAIL in test/stress/buffering/err.resize3.d . (It's an err.*.d.  So 
the test expects DTrace to fail.  That was the case due to BPF verifier 
complaints.  With the patch, however, those complaints were placated, 
meaning DTrace encountered no errors and the test barfed.)

On 6/7/21 5:53 PM, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>
> with a few nits.  E.g., do we have a length limit on commit subject 
> lines?  It might make sense to omit the trailing word "implementation" 
> or even to ditch mention of the specific subroutines given that, e.g., 
> speculation isn't even really done yet.  And...
>
> On 6/3/21 11:20 AM, Kris Van Hees wrote:
>> This patch provides the skeleton for the implementation of subroutines
>> in D.  It is equivalent to the implementation of D actions in the code
>> generator.
>>
>> The strlen() subroutine is provided as a first example of how to
>> imeplement subroutines.  The speculation() subroutine is provided as
>
> imeplement => implement
>
>> an example of a dummy implementation.  It always simply returns 0, but
>> this allows various tests for the compiler to be exercised.
>>
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> +static void
>> +dt_cg_call_subr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>> +{
>> +    dt_ident_t    *idp = dnp->dn_ident;
>> +    dt_cg_subr_f    *fun;
>> +
>> +    if (idp->di_kind != DT_IDENT_FUNC)
>> +        dnerror(dnp, D_CG_EXPR, "%s %s( ) may not be called from a D "
>> +                    "expression (D program context "
>> +                    "required)\n",
>> +            dt_idkind_name(idp->di_kind), idp->di_name);
>
> It's nice to reduce line breaks in error message strings so that 
> someone who gets a message has an easier time grepping for it. So, if 
> we're going >80 chars on the subject line, how about going >80 chars 
> in a useful place like this?  Or, at least improve the breaks in the 
> string.  Say:
>         if (idp->di_kind != DT_IDENT_FUNC)
>                 dnerror(dnp, D_CG_EXPR,
>                         "%s %s( ) may not be called from a D expression "
>                         "(D program context required)\n",
>                         dt_idkind_name(idp->di_kind), idp->di_name);
>
>> +
>> +    assert(idp->di_id > 0 && idp->di_id <= DIF_SUBR_MAX);
>
> > should probably be >=
>
>> +
>> +    fun = _dt_cg_subr[idp->di_id];
>> +    if (fun == NULL)
>> +        dnerror(dnp, D_FUNC_UNDEF, "unimplemented subroutine: %s\n",
>> +            idp->di_name);
>> +    fun(dnp, dlp, drp);
>> +}