Re: [PATCH 3/4] cg: reject clauses using return() by default

[Kris Van Hees <kris.van.hees@oracle.com>]

On Tue, Jul 15, 2025 at 11:37:39AM +0100, Nick Alcock wrote:
> On 15 Jul 2025, Kris Van Hees verbalised:
> 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> 
> Reviewed-by: Nick Alcock <nick.alcock@oracle.com>
> 
> > ---
> >  libdtrace/dt_cg.c      | 10 +++++++++-
> >  libdtrace/dt_errtags.h |  1 +
> >  libdtrace/dtrace.h     | 19 ++++++++++---------
> >  3 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 738597ed..0607cd4e 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -875,6 +875,10 @@ dt_cg_call_clause(dtrace_hdl_t *dtp, dtrace_stmtdesc_t *sdp, dt_clause_arg_t *ar
> >  	 */
> >  	if (prp->prov->impl->reject_clause != NULL)
> >  		prp->prov->impl->reject_clause(prp, sdp->dtsd_clauseflags);
> > +	else if (sdp->dtsd_clauseflags & DT_CLSFLAG_RETURN)
> > +		xyerror(D_ACT_RETURN, "return() not allowed for %s:%s:%s:%s\n",
> > +			prp->desc->prv, prp->desc->mod, prp->desc->fun,
> > +			prp->desc->prb);
> 
> Ah, I guess these are the "default checks" given in the previous commit,
> so you can just set up the reject_clause() if destructive mode is on,
> and not need to check here at all.

Yes, this is where the comment block should start talking about default check.
The check has to be specific to DT_CLSFLAG_RETURN because we do not want to
just reject a clause based on whetehr destructive mode is on or not.  That is
done elsewhere (but in the future could be added here - as a future change to
the -w patch Eugene posted).

> > diff --git a/libdtrace/dtrace.h b/libdtrace/dtrace.h
> > index 20672d3f..f0c1b8dc 100644
> > --- a/libdtrace/dtrace.h
> > +++ b/libdtrace/dtrace.h
> > @@ -154,15 +154,16 @@ typedef struct dtrace_stmtdesc {
> >  } dtrace_stmtdesc_t;
> >  
> >  /* dtsd clause flags */
> > -#define DT_CLSFLAG_DATAREC		1	/* data-recording */
> > -#define DT_CLSFLAG_SPECULATE		2	/* speculate */
> > -#define DT_CLSFLAG_COMMIT		4	/* commit */
> > -#define DT_CLSFLAG_COMMIT_DISCARD	8	/* commit/discard */
> > -#define DT_CLSFLAG_EXIT			16	/* exit */
> > -#define DT_CLSFLAG_DESTRUCT		32	/* destructive */
> > -#define DT_CLSFLAG_AGGREGATION		64	/* aggregation */
> > -#define DT_CLSFLAG_USDT_INCLUDE		128	/* could be used in USDT clause */
> > -#define DT_CLSFLAG_USDT_EXCLUDE		256	/* could not be used in USDT clause */
> > +#define DT_CLSFLAG_DATAREC		0x0001	/* data-recording */
> > +#define DT_CLSFLAG_SPECULATE		0x0002	/* speculate */
> > +#define DT_CLSFLAG_COMMIT		0x0004	/* commit */
> > +#define DT_CLSFLAG_COMMIT_DISCARD	0x0008	/* commit/discard */
> > +#define DT_CLSFLAG_EXIT			0x0010	/* exit */
> > +#define DT_CLSFLAG_DESTRUCT		0x0020	/* destructive */
> > +#define DT_CLSFLAG_RETURN		0x0040	/* aggregation */
> > +#define DT_CLSFLAG_AGGREGATION		0x0080	/* return action */
> > +#define DT_CLSFLAG_USDT_INCLUDE		0x0100	/* could be used in USDT clause */
> > +#define DT_CLSFLAG_USDT_EXCLUDE		0x0200	/* could not be used in USDT clause */
> 
> Oh yes please! I mean we all know our powers of two anyway, but this is
> just cleaner. (Not sure why you stuffed DT_CLSFLAG_RETURN in the middle
> like that, though.)

It just seemed like the better place, so USDT_INCLUDE and USDT_EXCLUDE are
nicely at 0x0100 and 0x0200.