[DTrace-devel] [PATCH 1/2] Add clause flags

[Kris Van Hees]

On Tue, Sep 15, 2020 at 10:24:47PM -0700, Eugene Loh wrote:
> Suggestions incorporated, except for that business of setting the 
> DATAREC flag in dt_cg_store_value().  See questions below.
> 
> 
> On 09/15/2020 09:50 PM, Kris Van Hees wrote:
> 
> > On Tue, Sep 15, 2020 at 10:26:35PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> There are compile-time rules to enforce regarding which actions are
> >> compatible with which others.  E.g., data-recording actions cannot
> >> follow commits, etc.  Introduce flags that track clause actions during
> >> compilation so that these rules can be enforced.
> >>
> >> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> >> @@ -589,6 +601,16 @@ dt_cg_act_exit(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >>   	struct bpf_insn	instr;
> >>   	dt_irlist_t	*dlp = &pcb->pcb_ir;
> >>   	uint_t		off;
> >> +	int		*cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> >> +
> >> +	/* process clause flags */
> >> +	if (*cfp & DT_CLSFLAG_SPECULATE)
> >> +		dnerror(dnp, D_EXIT_SPEC,
> >> +		    "exit( ) may not follow speculate( )\n");
> >> +        if (*cfp & DT_CLSFLAG_COMMIT)
> >> +                dnerror(dnp, D_DREC_COMM,
> >> +		    "data-recording actions may not follow commit( )\n");
> >> +	*cfp |= DT_CLSFLAG_EXIT | DT_CLSFLAG_DATAREC;
> > This should only set DT_CLSFLAG_EXIT, and the DT_CLSFLAG_DATAREC should be set
> > in dt_cg_store_val() since that is the function that calls dt_add_rec() and
> > then also generates the instructions to actually write the value to the output
> > buffer.
> 
> I do not understand the rationale.  It would seem to me that here is a 
> perfectly fine place to set the DATAREC flag.  We know what the action 
> is:  it's exit().  We need not care about how the action is implemented 
> (whether with a call to dt_cg_store_val, direct calls to dt_add_rec, or 
> whatever).  Indeed, we just tested for D_DREC_COMM ("data-recording 
> actions may not... blahblahblah").  So we have said definitively that 
> this is a data-recording action, regardless of how we now implement that 
> action.

OK

> >>   	dt_cg_node(dnp->dn_args, &pcb->pcb_ir, pcb->pcb_regs);
> >>   
> >> @@ -638,6 +660,13 @@ dt_cg_act_printf(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >>   	dt_pfargv_t	*pfp;
> >>   	char		n[DT_TYPE_NAMELEN];
> >>   	char		*str;
> >> +	int		*cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> >> +
> >> +	/* process clause flags */
> >> +	if (*cfp & DT_CLSFLAG_COMMIT)
> >> +		dnerror(dnp, D_DREC_COMM,
> >> +		    "data-recording actions may not follow commit( )\n");
> >> +	*cfp |= DT_CLSFLAG_DATAREC;
> > See comment above about setting DT_CLS_FLAG_DATARC from dt_cg_store_val().
> 
> Actually, the printf action is a better example of what I'm talking 
> about.  The subsequent code has different code paths:  one that calls 
> dt_rec_add() directly and the other is a call to dt_cg_store_val().  We 
> should set the DATAREC flag because of what we know about the action, 
> not because of what we know about the various functions we use to 
> implement it -- that is, that a side effect of dt_cg_store_val is to set 
> the DATAREC flag while dt_rec_add does not have such a side effect.
> 
> >>   	/*
> >>   	 * Ensure that the format string is a string constant.
> >> @@ -810,6 +853,13 @@ dt_cg_act_trace(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >>   	char		n[DT_TYPE_NAMELEN];
> >>   	dt_node_t	*arg = dnp->dn_args;
> >>   	int		type = 0;
> >> +	int		*cfp = &pcb->pcb_stmt->dtsd_clauseflags;
> >> +
> >> +	/* process clause flags */
> >> +	if (*cfp & DT_CLSFLAG_COMMIT)
> >> +		dnerror(dnp, D_DREC_COMM,
> >> +		        "data-recording actions may not follow commit( )\n");
> >> +	*cfp |= DT_CLSFLAG_DATAREC;
> > See comment above about setting DT_CLS_FLAG_DATARC from dt_cg_store_val().
> >
> >>   	if (dt_node_is_void(arg)) {
> >>   		dnerror(arg, D_TRACE_VOID,
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel