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

[Eugene Loh]

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.

>>   	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,