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

[Jan Beulich <jbeulich@suse.com>]

On 17.01.2024 02:20, Indu Bhagat wrote:
> 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 ?

I'm afraid I'm not aware of any.

> 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 ?

Without doing a full audit I'm inclined to say "perhaps". One important
thing to consider here is Intel syntax, where (memory) operand size is
normally expressed via (here) "word ptr" rather than a suffix. And iirc
while suffix derivation (when none was specified) would look at
register operands, it wouldn't normally look at memory ones.

It feels to me as if it was more robust if you simply set an indicator
in SHORT_MNEM_SUFFIX handling within process_suffix(), or if
alternatively you broke out the conditional there into a helper function
which you then could re-use for your purpose (especially in the latter
case beware of the JUMP_BYTE handling, though).

Jan