Re: [PATCH 2/6] x86-64: Intel64 adjustments for conditional jumps

[Jan Beulich <jbeulich@suse.com>]

On 09.03.2020 13:13, H.J. Lu wrote:
> On Mon, Mar 9, 2020 at 12:11 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 06.03.2020 16:35, H.J. Lu wrote:
>>> On Fri, Mar 6, 2020 at 6:53 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 06.03.2020 15:39, H.J. Lu wrote:
>>>>> On Fri, Mar 6, 2020 at 12:12 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> Just like for unconditional direct JMP, AMD and Intel differ in their
>>>>>> handling. Mirror JMP handling to Jcc.
>>>>>>
>>>>>> gas/
>>>>>> 2020-03-XX  Jan Beulich  <jbeulich@suse.com>
>>>>>>
>>>>>>         * testsuite/gas/i386/x86-64-branch-2.s,
>>>>>>         testsuite/gas/i386/x86-64-branch-3.s: Add Jcc cases.
>>>>>>         * testsuite/gas/i386/ilp32/x86-64-branch.d,
>>>>>>         testsuite/gas/i386/opcode-suffix.d,
>>>>>>         testsuite/gas/i386/x86-64-branch-2.d,
>>>>>>         testsuite/gas/i386/x86-64-branch-3.d,
>>>>>>         testsuite/gas/i386/x86-64-branch.d: Adjust expectations.
>>>>>>
>>>>>> opcodes/
>>>>>> 2020-03-XX  Jan Beulich  <jbeulich@suse.com>
>>>>>>
>>>>>>         * i386-dis.c (safe-ctype.h): Include.
>>>>>>         (X86_64_0F8x): New enumerator.
>>>>>>         (dis386): Extend comment ahead of it.
>>>>>>         (dis386_twobyte): Vector Jcc to X86_64_0F8x.
>>>>>>         (condition_code): New.
>>>>>>         (x86_64_table): Add X86_64_0F8x entry.
>>>>>>         (print_insn): Set condition_code. Move advancing of codep after
>>>>>>         it.
>>>>>>         (putop): Handle two-char escape case for 'C'. Handle 'C' prefix
>>>>>>         case for 'P' and '@'.
>>>>>>         * i386-opc.tbl (j<cc>): Split into AMD64 and Intel64 variants.
>>>>>>         * i386-tbl.h: Re-generate.
>>>>>> ---
>>>>>> I wonder if the suffix handling done here wouldn't also be the more
>>>>>> suitable one for JMP and CALL. In particular the 'q' suffix printed
>>>>>> unconditionally in 64-bit mode is more of a problem than helpful imo.
>>>>>>
>>>>>> --- a/gas/testsuite/gas/i386/ilp32/x86-64-branch.d
>>>>>> +++ b/gas/testsuite/gas/i386/ilp32/x86-64-branch.d
>>>>>> @@ -22,7 +22,7 @@ Disassembly of section .text:
>>>>>>  [      ]*[a-f0-9]+:    e9 00 00 00 00          jmpq   0x24     20: R_X86_64_PC32       \*ABS\*\+0x10003c
>>>>>>  [      ]*[a-f0-9]+:    66 e8 00 00 00 00       data16 callq 0x2a       26: R_X86_64_PLT32      foo-0x4
>>>>>>  [      ]*[a-f0-9]+:    66 e9 00 00 00 00       data16 jmpq 0x30        2c: R_X86_64_PLT32      foo-0x4
>>>>>> -[      ]*[a-f0-9]+:    66 0f 82 00 00 00 00    data16 jb 0x37  33: R_X86_64_PLT32      foo-0x4
>>>>>> +[      ]*[a-f0-9]+:    66 0f 82 00 00 00 00    data16 jbq 0x37 33: R_X86_64_PLT32      foo-0x4
>>>>>>  [      ]*[a-f0-9]+:    66 c3                   data16 retq *
>>>>>>  [      ]*[a-f0-9]+:    66 c2 08 00             data16 retq \$0x8
>>>>>>  [      ]*[a-f0-9]+:    ff d0                   callq  \*%rax
>>>>>
>>>>> I think it is a very bad idea to add suffix to jcc.
>>>>
>>>> Well, do you have an alternative suggestion, also in line with
>>>> JMP then? (See the somewhat related post-commit-message remark
>>>
>>> Since assembly doesn't require `q' suffix, can we drop it from disassembler?
>>
>> Why would we not be in the position to do so? But if we do,
>> can we then please settle on doing so uniformly (i.e. for
>> all near branch insns), i.e. in the above snippet e.g. not
>> just CALL and JMP, but also RET?
> 
> Let's drop 'q' suffix on them.
> 
>> The other question then is what to do in suffix-always mode:
>> I'd like to have things consistent there as well, and hence
>> I think we should emit suffixes in that case also for Jcc.
> 
> Since current assembler doesn't take 'q' suffix on Jcc:
> 
> [hjl@gnu-cfl-2 tmp]$ cat x.s
> jbq 1f
> 1:
> nop
> [hjl@gnu-cfl-2 tmp]$ gcc -c x.s
> x.s: Assembler messages:
> x.s:1: Error: invalid instruction suffix for `jb'
> [hjl@gnu-cfl-2 tmp]$
> 
> disassembler should never add 'q' suffix on Jcc.  But we can
> drop 'q' suffix on CALL/JMP/RET in suffix-always mode.

Coming back to this, in the hope of being able to resume work
on the patch at some point. I think I've gone a little too far
with the changes done so far (after this conversation), in
that I've made things disassemble e.g. like this:

[a-f0-9]+:	66 e9 00 00 00 00    	data16 jmp 6 <bar-0x7>	2: R_X86_64_PLT32	foo-0x4
[a-f0-9]+:	66 48 e9 00 00 00 00 	data16 jmpq d <bar>	9: R_X86_64_PLT32	foo-0x4

i.e. the redundant (other than for nullifying the operand size
override) REX.W gets transformed into a 'q' suffix. I'm now
thinking that instead all redundant prefixes would better be
printed as prefixes, despite the more cluttered resulting
output. If you agree, I'll have to go through all the adjusted
test cases again, hence my desire to have your general consent
up front. (Of course I mean to extend this underlying rule to
things like PUSH, in separate patches.)

That way I can then also avoid adding 'q' suffixes to Jcc.
What I'm not going to be able to avoid though is adding
'w' suffixes in AMD64 mode (and for 32-bit), as it's neither
reasonable to express this via prefix, nor would this be
consistent with JMP (and other insns).

Jan