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

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

On 11/6/23 03:03, Jan Beulich wrote:
> 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.
> 

Thanks. I had rebased the series after your commit.  I had initially 
thought of this to be a big patch,  but clearly I was overthinking.

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

Changed the type to unsigned int for reg. I see that the components 
surrounding the ginsn/scfi like dw2gencfi uses unsigned int for register 
numbers.  So it makes sense to stay uniform.

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

What I have not defined (deliberately) is any "temporary registers" in 
ginsn.  This means a CISC instruction cannot be split into multiple 
gisnns with data dependencies.  So to keep things simple, if machine 
instruction is a MOV, we pick MOV ginsn (with indirect access if this 
was a load/store).  If the machine instruction is an ADD, we pick ADD 
ginsn (with indirect access if applicable).

If I understand you correct, you suggest that we pick load/store ops 
like so:
   mov  disp(%reg), %reg --> load disp(%reg), %reg
   mov  %reg, disp(%reg) --> store %reg, disp(%reg)

Then what about arith ops with indirect access ? In theory those are 
load + arith + store. In other words, the above deliberation of picking 
load/store for mov seems inconsistent then.

For SCFI in aarch64 (WIP), load / store ginsns are used actively.