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

[Eugene Loh]

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.

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.


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