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

[Kris Van Hees]

On Wed, Jun 09, 2021 at 11:48:02AM -0400, Eugene Loh wrote:
> 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.)

That test becomes XFAIL because it cannot trigger a fault anymore right now.
The fact that is was a PASS before (i.e. the test failed) was not because of
the condition being tested but rather because something else in the test
failed (use of an unimplemented subroutine).

As I mentioned in my previous email reply to [03/20], updates to patches do
impact later patches and their effects on tests, so as I make changes to the
patches, the entire series gets updated to carry forward any effects from the
change.  Rather than flooding the list with tons of new versions, I ensure that
upadtes to patches do not cause regressions and I assume everyone else does the
same when they update patches in response to reviews.

	Kris

> 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);
> >> +}
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel