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

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

On 1/23/24 23:51, Jan Beulich wrote:
> On 24.01.2024 07:40, Indu Bhagat wrote:
>> @@ -5664,76 +5666,64 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>>   {
>>     offsetT src_disp = 0;
>>     ginsnS *ginsn = NULL;
>> -  unsigned int base_reg;
>> -  unsigned int index_reg;
>> +  unsigned int src1_reg;
>> +  const reg_entry *src1;
>>     offsetT index_scale;
>>     unsigned int dst_reg;
>>   
>>     if (!i.index_reg && !i.base_reg)
>>       {
>> -      /* lea symbol, %rN.  */
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
>> -      ginsn = ginsn_new_mov (insn_end_sym, false,
>> -			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
>> -			     GINSN_DST_REG, dst_reg, 0);
>> +      if (i.disp_operands && i.op[0].disps->X_op == O_constant)
>> +	{
>> +	  /* lea const, %rN.  */
>> +	  src_disp = i.op[0].disps->X_add_number;
>> +	  dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> +	  ginsn = ginsn_new_mov (insn_end_sym, false,
>> +				 GINSN_SRC_IMM, 0, src_disp,
>> +				 GINSN_DST_REG, dst_reg, 0);
>> +	  ginsn_set_where (ginsn);
>> +	}
> 
> Since earlier on you've been mentioning that you primarily target insn
> forms actually in use in existing code, I wonder whether you've ever
> seen any use of this. The same is better (shorter) expressed by MOV,
> and hence I'd expect people to prefer that form. IOW the question here
> is: Is there much value in having this code, rather than simply
> penalizing people bogusly using such by having this case end at the
> common x86_ginsn_unhandled path as well.
> 

IMO this is not a useful pattern either for the case when it is better 
expressed by a MOV.  I have not seen such usages so far, and I dont 
expect this to be seen either.

I am OK with removing this handling and default to x86_ginsn_unhandled 
code path with code comments here. Makes sense.

>> +      /* Skip handling lea symbol, %rN here.  Deal with it in the
>> +	 x86_ginsn_unhandled code path.  TBD_GINSN_GEN_NOT_SCFI.  */
>>       }
>> -  else if (i.base_reg && !i.index_reg)
>> +  else if ((i.base_reg && !i.index_reg)
>> +	   || (!i.base_reg && i.index_reg))
> 
> I frequently see conditionals like this written this way. Maybe it's
> indeed clearer to a majority; personally I'd prefer the shorter
> 
>    else if (!i.base_reg != !i.index_reg)
> 
> However, considering the earlier if() this is an else-if to, even
> 
>    else if (!i.base_reg || !i.index_reg)
> 
> would also suffice (but as per above that if() may want to go away).
> 

OK.  I will surely spend a few extra seconds reading this
   (!i.base_reg != !i.index_reg)
as compared to the original construct, but I am fine with using it if 
that's your preference.  I have switched to that expression.

>>       {
>> -      /* lea    -0x2(%base),%dst.  */
>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
> 
> Would be nice if the missing commas were added here.
> 

OK.

>> -      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);
>> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
>> +      src1_reg = ginsn_dw2_regnum (src1);
> 
> Since I can't spot any other use of src1, why not simply
> 
>        src1_reg = ginsn_dw2_regnum (i.base_reg ? i.base_reg : i.index_reg);
> 
> ? Otherwise at the very least please omit the pointless parentheses in
> the conditional expression.
> 

Will remove the parentheses.

>>         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
>> -    {
>> -      /* lea disp(%base,%index,imm) %dst.  */
>> -      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
>> -	 for index reg.  */
>> -      base_reg = ginsn_dw2_regnum (i.base_reg);
>> -      index_reg = ginsn_dw2_regnum (i.index_reg);
>> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
>> -      /* Generate an GINSN_TYPE_OTHER ginsn.  */
>> -      ginsn = ginsn_new_other (insn_end_sym, true,
>> -			       GINSN_SRC_REG, base_reg,
>> -			       GINSN_SRC_REG, index_reg,
>> -			       GINSN_DST_REG, dst_reg);
>> -    }
>> +      /* It makes sense to represent a scale factor of 1 precisely here
>> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
>> +	 base-without-index case).  Ignore the case when disp has a symbol
>> +	 instead.  */
>> +      if (!index_scale
>> +	  && (!i.disp_operands
>> +	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))
> 
> This is functionally identical to the shorter
> 
>        if (!index_scale
> 	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))
> 
> But: What about any of
> 
> 	lea	(%rax,%riz),%rbp
> 	lea	(%rax,4),%rbp
> 	lea	(%rax,%riz,4),%rbp
> 
> ?

Current behaviour is:

lea  (%rax,%riz),%rbp
ginsn: OTH 0, 0, %r6

lea  (%rax,4),%rbp
****  Warning: scale factor of 4 without an index register
ginsn: MOV %r0, %r6

lea  (%rax,%riz,4),%rbp
ginsn: OTH 0, 0, %r6

lea  sym(,%riz), %rbp
ginsn: OTH 0, 0, %r6

lea  (,%riz), %rbp
ginsn: MOV %r4, %r6
(We use DWARF register number 4 {%rsi} in lieu of %riz).

With respect to SCFI correctness, I dont see an issue...