[DTrace-devel] [PATCH 4/4] clean up dt_cg_typecast(): put dst->dn_reg=src->dn_reg in dt_cg_typecast()

[Kris Van Hees]

On Tue, Aug 11, 2020 at 07:04:08PM -0700, Eugene Loh wrote:
> On 08/11/2020 05:59 PM, Kris Van Hees wrote:
> 
> > On Tue, Aug 11, 2020 at 06:11:12PM -0400, eugene.loh at oracle.com wrote:
> >> From: Eugene Loh <eugene.loh at oracle.com>
> >>
> >> Every call to dt_cg_typecast() first sets dst->dn_reg to src->dn_reg.
> >> Instead of making this assignment at each call site, it's easier to
> >> move the call into the callee dt_cg_typecast().
> > This is a bad idea because it requires callers to be aware of this side
> > effect, which isn't really obvious for a function that takes both a src
> > and dst operand.  Any future use of dt_cg_typecast() that might want to
> > typecast src and get a value in dst (where src is not to be modified) is
> > in for a major surprise in that case which is not good.
> 
> Callers already have to be aware of this whole thing because they're 
> forced to do this extra dst->dn_reg=src->dn_reg operation explicitly.  
> Otherwise, dt_cg_typecast() doesn't work properly... the "major 
> surprise" you're talking about.

That would be an indication of a flaw in the legacy code and one that we need
to fix, obviously.  The callers setting dst = src is reasonable and sensible
since they ought to know what they are doing and what they are expecting, but
indeed if the typecast is effectively a no-op then dt_cg_typecast should emit
it as a simple mov from src to dst (and if the caller made them to be the same
register then it will be a MOV %rX, %rX that at some point in the near future
will be removed during optimization).

> The same comment applies to your response to patch 1/4.
> 
> If you want to change how dt_cg_typecast() works, that's a different 
> story.  I guess then you can have that register move (as in patch 1/4), 
> but it needs to be performed even if dt_cg_typecast()'s "if" predicate 
> is false.  But currently, dt_cg_typecast() is an "in place" typecast as 
> far as registers go.

Well, the problem is that it is and it isn't.  It is implemented as an
operation with dst and src that can be different registers (even though all
the current callers make them be the same), but it fails to handle the case
where dst and src are not the same registers *and* no shift is necessary.  That
is simply a bug, or most likely simply sloppy coding based on the fact that all
placed where they end up using dt_cg_typecast they were already forcing it to
be an in-place operation which masks the flaw in the implementation.

> >> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> >> ---
> >>   libdtrace/dt_cg.c | 9 +++------
> >>   1 file changed, 3 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> >> index b3b3d3a7..3ef94247 100644
> >> --- a/libdtrace/dt_cg.c
> >> +++ b/libdtrace/dt_cg.c
> >> @@ -1117,7 +1117,7 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
> >>    * using logical shift (unsigned result) or arithmetic shift (signed result).
> >>    */
> >>   static void
> >> -dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> >> +dt_cg_typecast(const dt_node_t *src, dt_node_t *dst,
> >>       dt_irlist_t *dlp, dt_regset_t *drp)
> >>   {
> >>   	size_t srcsize;
> >> @@ -1125,6 +1125,8 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> >>   	struct bpf_insn instr;
> >>   	int reg, n;
> >>   
> >> +	dst->dn_reg = src->dn_reg;
> >> +
> >>   	/* If the destination type is '@' (any type) we need not cast. */
> >>   	if (dst->dn_ctfp == NULL && dst->dn_type == CTF_ERR)
> >>   		return;
> >> @@ -1187,7 +1189,6 @@ dt_cg_arglist(dt_ident_t *idp, dt_node_t *args,
> >>   
> >>   		dt_node_diftype(yypcb->pcb_hdl, dnp, &t);
> >>   
> >> -		isp->dis_args[i].dn_reg = dnp->dn_reg; /* re-use register */
> >>   		dt_cg_typecast(dnp, &isp->dis_args[i], dlp, drp);
> >>   		isp->dis_args[i].dn_reg = -1;
> >>   
> >> @@ -1655,7 +1656,6 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >>   			 * and then cast the result to the member type.
> >>   			 */
> >>   			dt_cg_node(mnp->dn_membexpr, dlp, drp);
> >> -			mnp->dn_reg = mnp->dn_membexpr->dn_reg;
> >>   			dt_cg_typecast(mnp->dn_membexpr, mnp, dlp, drp);
> >>   
> >>   			/*
> >> @@ -1946,7 +1946,6 @@ dt_cg_inline(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >>   	}
> >>   
> >>   	dt_cg_node(inp->din_root, dlp, drp);
> >> -	dnp->dn_reg = inp->din_root->dn_reg;
> >>   	dt_cg_typecast(inp->din_root, dnp, dlp, drp);
> >>   
> >>   	if (idp->di_kind == DT_IDENT_ARRAY) {
> >> @@ -2265,7 +2264,6 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >>   
> >>   	case DT_TOK_LPAR:
> >>   		dt_cg_node(dnp->dn_right, dlp, drp);
> >> -		dnp->dn_reg = dnp->dn_right->dn_reg;
> >>   		dt_cg_typecast(dnp->dn_right, dnp, dlp, drp);
> >>   		break;
> >>   
> >> @@ -2297,7 +2295,6 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >>   			dxp->dx_ident->di_id = dnp->dn_left->dn_reg;
> >>   
> >>   			dt_cg_node(mnp->dn_membexpr, dlp, drp);
> >> -			dnp->dn_reg = mnp->dn_membexpr->dn_reg;
> >>   			dt_cg_typecast(mnp->dn_membexpr, dnp, dlp, drp);
> >>   
> >>   			dxp->dx_ident->di_flags &= ~DT_IDFLG_CGREG;
> >> -- 
> >> 2.18.4
> >>
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel