RE: [PATCH 8/8] Support APX JMPABS

["Hu, Lin1" <lin1.hu@intel.com>]

 > On 02.11.2023 12:29, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -7790,7 +7790,8 @@ match_template (char mnem_suffix)
> >    if (!quiet_warnings)
> >      {
> >        if (!intel_syntax
> > -	  && (i.jumpabsolute != (t->opcode_modifier.jump ==
> JUMP_ABSOLUTE)))
> > +	  && (i.jumpabsolute != (t->opcode_modifier.jump == JUMP_ABSOLUTE))
> > +	  && t->mnem_off != MN_jmpabs)
> >  	as_warn (_("indirect %s without `*'"), insn_name (t));
> 
> Coming back to this, which I did comment on before already. The insn taking an
> immediate operand doesn't really justify this, as it leaves open the underlying
> question of why you use JumpAbsolute in the insn template in the first place. I've
> gone through all the uses of JUMP_ABSOLUTE, and I didn't find any where the
> respective handling would be applicable here. In fact it's unclear whether the
> insn needs marking as any JUMP_* at all: It's not really different from, say, "mov
> $imm, %rcx".
>

According to the doc JMPABS is a 64-bit only ISA extension, and acts as a near-direct branch with an absolute target. I made this markup simply because I was mimicking other jmp's. If we don't need the attribute, I'm glad I can remove it.

> 
> There's a further question regarding its operand representation,
> though: Can you explain why it's Imm64, not Disp64? The latter would, to me,
> seem more natural to use here. Not only from a assembler internals perspective,
> but also from the users' one: The $ in the operand carries absolutely no meaning
> (see also the related testcase comment below) in AT&T syntax, and there's no
> noticeable difference in Intel syntax afaict.
>

In my opinion, If compiler want  to jump "anywhere" and the displacement can not fit in a 32-bit immediate , compiler will fallback to indirect branches. My current knowledge is that jmpabs came about as a solution to the problem about indirect braches. It's not the same as the jmp. Currently the parameters of jmpabs are absolute addresses optimized by PLT or JIT. I think using imm64 avoids confusion with the disp64. That's why the designer designed it this way.

One colleague in our group have written an introductory document can be referred. (https://kanrobert.github.io/rfc/All-about-APX-JMPABS/)

>
> > @@ -8939,6 +8940,9 @@ process_operands (void)
> >  	}
> >      }
> >
> > +  if (i.tm.mnem_off == MN_jmpabs)
> > +    i.rex2_encoding = true;
> 
> Please see my earlier remarks wrt "rex2" vs "{rex2}". What you do here is effect
> the latter. Yet as indicated, the pseudo-prefix isn't really an indication of "must
> have REX2 prefix", but only a weak request to do so if possible. I think you want
> to set i.rex2 here instead, requiring a way to express that an empty REX2 prefix
> is wanted.
>

But in terms of encoding, i.rex2 should be 0. Can I do special handling in build_rex2_prefix? 

> 
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-jmpabs-inval.s
> > @@ -0,0 +1,17 @@
> > +# Check bytecode of APX_F jmpabs instructions with illegal encode.
> > +
> > +	.text
> > +# With 66 prefix
> > +	.byte
> 0x66,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +	.byte 0x66,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +# With 67 prefix
> > +	.byte
> 0x67,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +	.byte 0x67,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +# With F2 prefix
> > +	.byte
> 0xf2,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +	.byte 0xf2,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +# With F3 prefix
> > +	.byte
> 0xf3,0x64,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +	.byte 0xf3,0xd5,0x00,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> > +# REX2.M0 = 0 REX2.W = 1
> > +	.byte 0xd5,0x08,0xa1,0x01,0x00,0x00,0x00,0x00,0x00,0x00,0x00
> 
> As per earlier comments: This wants expressing via .insn, to yield input to gas
> human-readable (even if, as it looks, two .insn are going to be required per
> resulting construct). Further in the last comment, why is
> REX2.M0 mentioned there, but not elsewhere? Also what purpose serve the
> 0x64 bytes here? The encodings are invalid irrespective of them. Instead I'd kind
> have expected LOCK to also be covered.
>

Because this error line is only for the special case where M0 == 0, and base_opcode == 0xa1, W should be 0, other than 1. If M0 = 1, W = 1, base_opcode == 0xa1, I think it could decoding as mov rax, moffs or ( some future insn). Elsewhere it's just excluding invalid prefixes. I don't see in the docs that it triggers #UD, am I missing something?

> 
> Also a spec question as we're talking of what is or is not valid (i.e.
> causing #UD) here: Why would XCR0.APX=0 need to cause #UD? There's no use
> of eGPR-s here.
>

Sorry, what is XCR0.APX?
 
>
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-jmpabs.s
> > @@ -0,0 +1,10 @@
> > +# Check 64bit APX_F JMPABS instructions
> > +
> > +	.text
> > + _start:
> > +	jmpabs	      $0x0202020202020202
> > +	jmpabs	      $0x2
> > +
> > +.intel_syntax noprefix
> > +	jmpabs	      0x0202020202020202
> > +	jmpabs	      0x2
> 
> I expect this isn't going to be the normal use of the insn. Instead I would foresee
> the typical users to be "jmpabs symbol" (and - as per above - intentionally
> omitting the $ already). IOW the testcase also wants to cover the case requiring
> a relocation, including a check that the correct relocation is emitted (covering
> both ELF and COFF; I'm not going to insist on also covering Mach-O, as - for a
> reason that escapes me - gas can't even be configured for x86_64-*-darwin*).
>

Based on the previous discussion, we think that this usage is not currently supported. If users want to use symbol, I think they can use "jmp symbol".

> 
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -106,6 +106,7 @@ static bool MOVSXD_Fixup (instr_info *, int, int);
> > static bool DistinctDest_Fixup (instr_info *, int, int);  static bool
> > PREFETCHI_Fixup (instr_info *, int, int);  static bool
> > PUSH2_POP2_Fixup (instr_info *, int, int);
> > +static bool JMPABS_Fixup (instr_info *, int, int);
> >
> >  static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
> >  						enum disassembler_style,
> > @@ -258,6 +259,9 @@ struct instr_info
> >    char scale_char;
> >
> >    enum x86_64_isa isa64;
> > +
> > +  /* Remember if the current op is jmpabs.  */  bool is_jmpabs;
> >  };
> 
> This field would probably best live next to op_is_jump (and then also be named
> op_is_jmpabs, assuming a separate boolean is indeed needed).
> I further expect that op_is_jump also wants setting for JMPABS.
>

Can I change op_is_jump's type from bool to unsigned int?
 
>
> > @@ -2032,7 +2036,7 @@ static const struct dis386 dis386[] = {
> >    { "lahf",		{ XX }, 0 },
> >    /* a0 */
> >    { "mov%LB",		{ AL, Ob }, PREFIX_REX2_ILLEGAL },
> > -  { "mov%LS",		{ eAX, Ov }, PREFIX_REX2_ILLEGAL },
> > +  { "mov%LS",		{ { JMPABS_Fixup, eAX_reg }, { JMPABS_Fixup,
> v_mode } }, PREFIX_REX2_ILLEGAL },
> >    { "mov%LB",		{ Ob, AL }, PREFIX_REX2_ILLEGAL },
> >    { "mov%LS",		{ Ov, eAX }, PREFIX_REX2_ILLEGAL },
> >    { "movs{b|}",		{ Ybr, Xb }, PREFIX_REX2_ILLEGAL },
> > @@ -9648,7 +9652,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int
> intel_syntax)
> >      }
> >
> >    if ((dp->prefix_requirement & PREFIX_REX2_ILLEGAL)
> > -      && ins.last_rex2_prefix >= 0)
> > +      && ins.last_rex2_prefix >= 0 && !ins.is_jmpabs)
> >      {
> >        i386_dis_printf (info, dis_style_text, "(bad)");
> >        ret = ins.end_codep - priv.the_buffer; @@ -13857,3 +13861,38 @@
> > PUSH2_POP2_Fixup (instr_info *ins, int bytemode, int sizeflag)
> >
> >    return OP_VEX (ins, bytemode, sizeflag);  }
> > +
> > +static bool
> > +JMPABS_Fixup (instr_info *ins, int bytemode, int sizeflag) {
> > +  if (ins->address_mode == mode_64bit
> > +      && ins->last_rex2_prefix >= 0
> > +      && (ins->rex2 & 0x80) == 0x0)
> > +    {
> > +      uint64_t op;
> > +
> > +      if (bytemode == eAX_reg)
> > +	return true;
> > +
> > +      if (!get64 (ins, &op))
> > +	return false;
> > +
> > +      if ((ins->prefixes & (PREFIX_OPCODE | PREFIX_ADDR)) != 0x0
> > +	  || (ins->rex & REX_W) != 0x0)
> > +	{
> > +	  oappend (ins, "(bad)");
> > +	  return true;
> > +	}
> > +
> > +      ins->mnemonicendp = stpcpy (ins->obuf, "jmpabs");
> > +      ins->all_prefixes[ins->last_rex2_prefix] = 0;
> 
> This doesn't look right. REX2.{R,X,B}{3,4} set still want recording in the output. I
> expect you may need to set a bit in rex2_used here, but how exactly that ought
> to work depends on how comments on earlier patches are going to be
> addressed. This may then also eliminate the need for ...
> 
> > +      ins->is_jmpabs = true;
> 
> ... this field, which likely will be covered by a more generic approach.
> 
>

Then this part of the discussion, as well as the modifications, I will wait for the front patch to be finalized.

>
> > +      oappend_immediate (ins, op);
> > +
> > +      return true;
> > +    }
> > +
> > +  if (bytemode == eAX_reg)
> > +    return OP_IMREG (ins, bytemode, sizeflag);  return OP_OFF64 (ins,
> > + v_mode, sizeflag);
> 
> v_mode is, afaics, properly passed into here. Why would you open-code that,
> instead of using bytemode? Not doing so will give the compiler more ICF
> opportunities.
> 

I reckon it's a mistake. Have fixed.

BRs,
Lin