Re: [PATCH, V2 07/10] gas: synthesize CFI for hand-written asm

[Jan Beulich <jbeulich@suse.com>]

On 04.11.2023 08:29, Indu Bhagat wrote:
> On 11/2/23 08:53, Jan Beulich wrote:
>> On 30.10.2023 17:51, Indu Bhagat wrote:
>>> +static ginsnS*
>>> +ginsn_init (enum ginsn_type type, symbolS *sym, bool real_p)
>>> +{
>>> +  ginsnS *ginsn = ginsn_alloc ();
>>> +  ginsn->type = type;
>>> +  ginsn->sym = sym;
>>
>> Is a symbol hanging off of a ginsn ever intended to be altered? If
>> not, field and function argument would want to be pointer-to-const.
>>
> 
> No. This symbol is not intended to be altered.
> 
> However, using const symbolS* will cause complaints of "passing argument 
> 1 of ‘S_GET_NAME’ discards ‘const’ qualifier" etc.  Calling most of the 
> S_XXX (symbolS *sym) will need some type casting etc, but that will 
> somewhat defeat the purpose?

Well, yes, of course you shouldn't be casting away const-ness. I've made
a patch to adjust S_GET_NAME(), which I'll post after having run a full
set of tests on it.

>>> +static void
>>> +ginsn_set_src (struct ginsn_src *src, enum ginsn_src_type type, uint32_t reg,
>>> +	       int32_t immdisp)
>>
>> I find the use of fixed-width types suspicious here: For reg, likely it wants
>> to be unsigned int. For immdisp it's less clear: Immediates can be wider than
>> 32 bits, and they may also be either signed ot unsigned. From an abstract
>> perspective, assuming the immediate value actually is used for anything, I'd
>> expect offsetT to be used, following struct expressionS' X_add_number.
>>
> 
> Thanks, I will consider trying offsetT for immediate. It is a more 
> appropriate data type than int32_t.  For reg, why is uint32_t less 
> appropriate than unsigned int ?

Fixed-width types come with a price: In principle they may not even be
available, and they may also not be the most efficient types to deal with
for an architecture. Therefore my rule of thumb is that they're best
used only for "describing" interfaces. As long as (here) register numbers
will fit in an unsigned int, using that basic type looks more appropriate
to me.

>>> +ginsnS *
>>> +ginsn_new_mov (symbolS *sym, bool real_p,
>>> +	       enum ginsn_src_type src_type, uint32_t src_reg, int32_t src_disp,
>>> +	       enum ginsn_dst_type dst_type, uint32_t dst_reg, int32_t dst_disp)
>>> +{
>>> +  ginsnS *ginsn = ginsn_init (GINSN_TYPE_MOV, sym, real_p);
>>> +  /* src info.  */
>>> +  ginsn_set_src (&ginsn->src[0], src_type, src_reg, src_disp);
>>> +  /* dst info.  */
>>> +  ginsn_set_dst (&ginsn->dst, dst_type, dst_reg, dst_disp);
>>> +
>>> +  return ginsn;
>>> +}
>>
>> As indicated before, if both src and dst can be indirect here, ...
>>
>>> +ginsnS *
>>> +ginsn_new_store (symbolS *sym, bool real_p,
>>> +		 enum ginsn_src_type src_type, uint32_t src_reg,
>>> +		 enum ginsn_dst_type dst_type, uint32_t dst_reg, int32_t dst_disp)
>>> +{
>>> +  ginsnS *ginsn = ginsn_init (GINSN_TYPE_STORE, sym, real_p);
>>> +  /* src info.  */
>>> +  ginsn_set_src (&ginsn->src[0], src_type, src_reg, 0);
>>> +  /* dst info.  */
>>> +  gas_assert (dst_type == GINSN_DST_INDIRECT);
>>> +  ginsn_set_dst (&ginsn->dst, dst_type, dst_reg, dst_disp);
>>> +
>>> +  return ginsn;
>>> +}
>>> +
>>> +ginsnS *
>>> +ginsn_new_load (symbolS *sym, bool real_p,
>>> +		enum ginsn_src_type src_type, uint32_t src_reg, int32_t src_disp,
>>> +		enum ginsn_dst_type dst_type, uint32_t dst_reg)
>>> +{
>>> +  ginsnS *ginsn = ginsn_init (GINSN_TYPE_LOAD, sym, real_p);
>>> +  /* src info.  */
>>> +  gas_assert (src_type == GINSN_SRC_INDIRECT);
>>> +  ginsn_set_src (&ginsn->src[0], src_type, src_reg, src_disp);
>>> +  /* dst info.  */
>>> +  ginsn_set_dst (&ginsn->dst, dst_type, dst_reg, 0);
>>> +
>>> +  return ginsn;
>>> +}
>>
>> ... I can't see what these are needed for.
>>
> 
> For x86, they may not seem necessary. But for other architectures, or 
> say for future uses-cases, we may need them. I think it is more 
> meaningful (and readable) to see a LOAD/STORE/MOV ginsn for a machine 
> instruction of the same type.  For other RISC-like ISAs, it is clearer 
> to have separate MOV/LOAD/STORE instructions.
> 
> ginsn is meant to provide an infrastructure for other uses cases that 
> may crop up later.

But then I consider it as odd that you munge loads/stores on x86 into
ginsn_new_mov(), by using "indirect" operands. Imo it would be better
to be consistent here, one way or the other.

Jan