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

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

On 1/10/24 01:44, Jan Beulich wrote:
> On 10.01.2024 07:10, Indu Bhagat wrote:
>> On 1/9/24 01:30, Jan Beulich wrote:
>>> On 08.01.2024 20:33, Indu Bhagat wrote:
>>>> On 1/5/24 05:58, Jan Beulich wrote:
>>>>> On 03.01.2024 08:15, Indu Bhagat wrote:
>>>>>> +/* Check whether a '66H' prefix accompanies the instruction.
>>>>> With APX 16-bit operand size isn't necessarily represented by a 66h
>>>>> prefix, but perhaps with an "embedded prefix" inside the EVEX one.
>>>>> Therefore both the comment and even more so ...
>>>>>
>>>>>> +   The current users of this API are in the handlers for PUSH, POP
>>>>>> +   instructions.  These instructions affect the stack pointer implicitly:  the
>>>>>> +   operand size (16, 32, or 64 bits) determines the amount by which the stack
>>>>>> +   pointer is decremented (2, 4 or 8).  When '66H' prefix is present, the
>>>>>> +   instruction has a 16-bit operand.  */
>>>>>> +
>>>>>> +static bool
>>>>>> +ginsn_prefix_66H_p (i386_insn insn)
>>>>> ... the function name would better not allude to just the legacy
>>>>> encoding. Maybe ginsn_opsize_prefix_p()?
>>>>>
>>>> Isnt 66H_p more readable and easier to follow because that's what the
>>>> function is currently checking ?  If more scenarios were being handled,
>>>> ginsn_opsize_prefix_p () would fit better.
>>> Well, as said - with APX you can't get away with just 0x66 prefix checking.
>>> That prefix is simply illegal to use with EVEX-encoded insns.
>>>
>> I am using the following in ginsn_opsize_prefix_p ():
>>
>> !(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX] == 0x66
> That addresses one half of my earlier remarks. Note however that elsewhere
> we never check i.prefix[DATA_PREFIX] against being 0x66; we only ever check
> for it being zero or non-zero. I'd like this to remain consistent.
> 
> For EVEX-encoded APX insns this isn't going to be sufficient though. See
> respective code in process_suffix():
> 
> 	  /* The DATA PREFIX of EVEX promoted from legacy APX instructions
> 	     needs to be adjusted.  */
> 	  if (i.tm.opcode_space == SPACE_EVEXMAP4)
> 	    {
> 	      gas_assert (!i.tm.opcode_modifier.opcodeprefix);
> 	      i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
> 	    }
> 	  else if (!add_prefix (prefix))
> 	    return 0;
> 
> So you'll need to also check for that combination, plus take care of
> avoiding insns where PREFIX_0X66 alters operation, not operand size
> (ADCX being an example).

[V5 is now committed. I am continuing to work on some of the discussed 
pending items from V4.]

Thanks. So looks like to correctly check for prefix 66H, one needs to 
check that:
   - !(i.prefix[REX_PREFIX] & REX_W) && i.prefix[DATA_PREFIX]
   - (i.tm.opcode_space == SPACE_EVEXMAP4
      && i.tm.opcode_modifier.opcodeprefix == PREFIX_0X66);
   - selectively handle the specific ops where PREFIX_0x66 alters 
operation.  I tried looking around but haven't found a targetted way to 
identify such ops.  Is there a way ?

Alternatively, since x86_ginsn_new () will be called after 
process_suffix (), I wonder if I can update the code to simply check for 
i.suffix to be 'w' for reliably detecting the 16-bit ops for all x86 
insns.  Is the check on suffix correct and reliable ?