Re: [PATCH] gas: x86: ginsn: adjust ginsns for certain lea ops

[Indu Bhagat <indu.bhagat@oracle.com>]

On 1/23/24 02:15, Jan Beulich wrote:
> On 23.01.2024 10:38, Indu Bhagat wrote:
>> @@ -5678,47 +5680,7 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>>   			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
>>   			     GINSN_DST_REG, dst_reg, 0);
>>       }
>> -  else if (i.base_reg && !i.index_reg)
>> -    {
>> -      /* lea    -0x2(%base),%dst.  */
>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -
>> -      if (i.disp_operands)
>> -	src_disp = i.op[0].disps->X_add_number;
>> -
>> -      if (src_disp)
>> -	/* Generate an ADD ginsn.  */
>> -	ginsn = ginsn_new_add (insn_end_sym, true,
>> -			       GINSN_SRC_REG, base_reg, 0,
>> -			       GINSN_SRC_IMM, 0, src_disp,
>> -			       GINSN_DST_REG, dst_reg, 0);
>> -      else
>> -	/* Generate a MOV ginsn.  */
>> -	ginsn = ginsn_new_mov (insn_end_sym, true,
>> -			       GINSN_SRC_REG, base_reg, 0,
>> -			       GINSN_DST_REG, dst_reg, 0);
>> -    }
>> -  else if (!i.base_reg && i.index_reg)
>> -    {
>> -      /* lea (,%index,imm), %dst.  */
>> -      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
>> -	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
>> -	 is not carried forward either.  But this is fine because
>> -	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
>> -	 dest reg is interesting.  */
>> -      index_scale = i.log2_scale_factor;
>> -      index_reg = ginsn_dw2_regnum (i.index_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -      ginsn = ginsn_new_other (insn_end_sym, true,
>> -			       GINSN_SRC_REG, index_reg,
>> -			       GINSN_SRC_IMM, index_scale,
>> -			       GINSN_DST_REG, dst_reg);
>> -      /* FIXME - It seems to make sense to represent a scale factor of 1
>> -	 correctly here (i.e. not as "other", but rather similar to the
>> -	 base-without- index case above)?  */
>> -    }
>> -  else
>> +  else if (i.index_reg && i.base_reg)
>>       {
>>         /* lea disp(%base,%index,imm) %dst.  */
>>         /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
>> @@ -5732,6 +5694,50 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>>   			       GINSN_SRC_REG, index_reg,
>>   			       GINSN_DST_REG, dst_reg);
>>       }
>> +  else
>> +    {
>> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
>> +      gas_assert ((i.base_reg && !i.index_reg)
>> +		  || (!i.base_reg && i.index_reg));
>> +
>> +      index_scale = i.log2_scale_factor;
>> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
>> +      src1_reg = ginsn_dw2_regnum (src1);
>> +      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> +      /* It makes sense to represent a scale factor of 1 correctly here
>> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
>> +	 base-without-index case).  */
>> +      if (!index_scale)
>> +	{
>> +	  if (i.disp_operands)
>> +	    src_disp = i.op[0].disps->X_add_number;
>> +
>> +	  if (src_disp)
>> +	    /* Generate an ADD ginsn.  */
>> +	    ginsn = ginsn_new_add (insn_end_sym, true,
>> +				   GINSN_SRC_REG, src1_reg, 0,
>> +				   GINSN_SRC_IMM, 0, src_disp,
>> +				   GINSN_DST_REG, dst_reg, 0);
>> +	  else
>> +	    /* Generate a MOV ginsn.  */
>> +	    ginsn = ginsn_new_mov (insn_end_sym, true,
>> +				   GINSN_SRC_REG, src1_reg, 0,
>> +				   GINSN_DST_REG, dst_reg, 0);
> 
> You're still losing symbol information if "disp" involves one. Perhaps
> worth a comment.
> 
>> +	}
>> +      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
>> +	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
>> +	 is not carried forward either.  But this is fine because
>> +	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
>> +	 dest reg is interesting.  */
> 
> This comment would better move ...
> 
>> +      else
>> +	{
> 
> ... here. I also have to admit that I have trouble parsing the last sentence.
> 

When trying to reword this, I realized that it is not correct to 
translate lea %symbol, %rN to a 'GINSN_TYPE_MOV fake_const %rN' (when 
the destination is REG_SP/REG_FP a fake_const may then appear in the 
synthesized CFI).

Fixing that to generate GINSN_TYPE_OTHER then instead of GINSN_TYPE_MOV, 
led me to conclude that its time I take another of your review feedback: 
do not create ops now, only to say they are unnecessary later.

I have now removed code paths creating GINSN_TYPE_OTHER from this 
function.  We will let x86_ginsn_unhandled () code path create 
GINSN_TYPE_OTHER if necessary.

Sending V2 shortly.

Thanks

>> +	  gas_assert (i.index_reg);
>> +	  ginsn = ginsn_new_other (insn_end_sym, true,
>> +				   GINSN_SRC_REG, src1_reg,
>> +				   GINSN_SRC_IMM, index_scale,
>> +				   GINSN_DST_REG, dst_reg);
>> +	}
>> +    }
>>   
>>     ginsn_set_where (ginsn);
>>   
> 
> Since overall this is an improvement: Okay with the comment adjustments.
>