[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 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.

> 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