[X86][MC] Fix parsing Intel syntax indirect branch with symbol only

[alvinhochun]

Summary

Clang on Windows targets often requires indirect calls through the
import address table (IAT), and also .refptr stubs for MinGW target.
On 32-bit this generates assembly in the form of
call dword ptr [__imp__func], which MC had failed to handle correctly.
64-bit targets are not affected because rip-relative addressing is used.

Reported on: https://github.com/llvm/llvm-project/issues/62010

Depends on D149695, D149920

[alvinhochun]

Format the commit message better

[epastor]

Looks about right to me, as an extension of D124413 - but I would appreciate it if ayzhao@ can weigh in here, since that was his patch.

[alvinhochun]

I'm aware of the check-clang failure in CodeGen/ms-inline-asm-functions.c. It seems to be caused by the MS inline assembly being translated in a way that breaks with this patch, which means I may have to look into fixing that code too. That also revealed an issue with my patch regarding how call [offset function] is handled -- apparently GAS treats it the same as call function. I'll try to get this fixed.

[alvinhochun]

Turns out this change affects MS inline assembly and possibly other inline assemblies as well. Looking at ms-inline-asm-functions.c (on Compiler Explorer - https://godbolt.org/z/3xfW75Go4):

• Direct call: __asm call k; generates the inline assembly call dword ptr ${1:P} (which seems wrong, should be call ${1:P} instead?), which is converted into call dword ptr _k, gets compiled as call dword ptr [_k] with this patch (incorrect).
• Call through a global function pointer, which is marked as a "broken case" in the test: __asm call kptr; generates the inline assembly call $4, which is converted into call [offset _kptr], gets compiled as call dword ptr [_kptr] with this patch, which actually fixes the test case, except that call [offset _kptr] doesn't seem right (see below).
• Direct jump: __asm jmp k; generates the inline assembly jmp dword ptr ${1:P}, same issue as direct call.

Among these, the usage of operator offset in call seems questionable. I checked the behaviour of other assemblers:

• NASM:
  • Does not support offset operator.
• MASM (ml) Version 14.35.32215.0:
  • call offset fn is oddly assembled as push cs \n call fn.
  • call [offset fn] generates error A2023:instruction operand must have size.
  • call dword ptr [offset fn] is assembled as call dword ptr [fn].
• GAS (Intel syntax):
  • Both call offset fn and call [offset fn] are assembled the same way as call fn, which seems odd.
  • call dword ptr [offset fn] is assembled as call dword ptr [fn].

---

I think this issue may end up being too much for me to handle. I already spent more time than I wanted tracing the code and trying various hacks, but couldn't quite come up with proper fixes. Will someone be willing to take over this?

[MaskRay]

It looks we accept a case (t2, w/o or with the patch) that GNU as rejects.

% as -msyntax=intel =(printf 'call qword ptr [rip + fn_ref]') -o gas.o
/tmp/zshjfuv2e: Assembler messages:
/tmp/zshjfuv2e: Warning: end of file not at end of a line; newline inserted
/tmp/zshjfuv2e:1: Error: invalid operands (*UND* and *UND* sections) for `+'

We parse it as callq *fn_ref(%rip).

[alvinhochun]

In D149579#4313475, @MaskRay wrote:

It looks we accept a case (t2, w/o or with the patch) that GNU as rejects.

% as -msyntax=intel =(printf 'call qword ptr [rip + fn_ref]') -o gas.o
/tmp/zshjfuv2e: Assembler messages:
/tmp/zshjfuv2e: Warning: end of file not at end of a line; newline inserted
/tmp/zshjfuv2e:1: Error: invalid operands (*UND* and *UND* sections) for `+'

It does work if you add .intel_syntax noprefix to the assembly or pass -mnaked-reg to as.

[MaskRay]

In D149579#4312025, @alvinhochun wrote:

Turns out this change affects MS inline assembly and possibly other inline assemblies as well. Looking at ms-inline-asm-functions.c (on Compiler Explorer - https://godbolt.org/z/3xfW75Go4):

• Direct call: __asm call k; generates the inline assembly call dword ptr ${1:P} (which seems wrong, should be call ${1:P} instead?), which is converted into call dword ptr _k, gets compiled as call dword ptr [_k] with this patch (incorrect).
• Call through a global function pointer, which is marked as a "broken case" in the test: __asm call kptr; generates the inline assembly call $4, which is converted into call [offset _kptr], gets compiled as call dword ptr [_kptr] with this patch, which actually fixes the test case, except that call [offset _kptr] doesn't seem right (see below).
• Direct jump: __asm jmp k; generates the inline assembly jmp dword ptr ${1:P}, same issue as direct call.

Among these, the usage of operator offset in call seems questionable. I checked the behaviour of other assemblers:

• NASM:
  • Does not support offset operator.
• MASM (ml) Version 14.35.32215.0:
  • call offset fn is oddly assembled as push cs \n call fn.
  • call [offset fn] generates error A2023:instruction operand must have size.
  • call dword ptr [offset fn] is assembled as call dword ptr [fn].
• GAS (Intel syntax):
  • Both call offset fn and call [offset fn] are assembled the same way as call fn, which seems odd.
  • call dword ptr [offset fn] is assembled as call dword ptr [fn].

---

I think this issue may end up being too much for me to handle. I already spent more time than I wanted tracing the code and trying various hacks, but couldn't quite come up with proper fixes. Will someone be willing to take over this?

OK. I think after D149695 (landed), D149920, and this patch D149579, these cases should be correct.

[MaskRay]

llvm/test/MC/X86/intel-syntax-branch.s

49

ICC and MSVC parse this differently.

Is this syntax valid?

[alvinhochun]

Rebase and add test cases with offset operator, and some TODOs/FIXMEs for comment

[alvinhochun]

In D149579#4320765, @MaskRay wrote:

OK. I think after D149695 (landed), D149920, and this patch D149579, these cases should be correct.

Thank you so much for looking into this!

I have one question in inline comment.

llvm/test/MC/X86/intel-syntax-branch.s

49

MASM ml.exe assembles this as jmp dword ptr [fn_ref] in my test, so does GAS. I don't know how valid this syntax is though.

62–68

About the call [offset fn_ref] case, do you think we should reject it since MASM does not accept this syntax and GAS appears to behave oddly with it? The ms-inline-asm-functions.c test does generate call [offset _kptr] so this will also need to be changed.

[MaskRay]

llvm/test/MC/X86/intel-syntax-branch.s

49

LG. cl.exe compiles void (*kptr)(); ... __asm call kptr; to call DWORD PTR _kptr, so it seems that the bracket can be omitted.

62–68

Yes, I think we should reject call [offset fn_ref]. Confirmed with a MinGW dev (lh_mouse) that this is invalid.

[MaskRay]

Thanks!

[MaskRay]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

2660

I think this patch can be pushed now.

Remove #if 0 from this patch. Just add the lines in D150048.

[MaskRay]

llvm/test/MC/X86/intel-syntax-branch-fail.s

1 ↗

(On Diff #519869)

Don't create a separate test for negative tests. Use the --defsym=ERR=1 and .ifdef ERR pattern to keep positive and negative tests in one file.

[MaskRay]

llvm/test/MC/X86/intel-syntax-branch-fail.s

5 ↗

(On Diff #519869)

[[#@LINE+-1]] => [[#@LINE-1]]

Remove the file name intel-syntax-branch-fail.s.

I'd remove TODO-CHECK in this patch and add `// CHECK: :[[#@LINE+-1]]:1: error: OFFSET` operator cannot be used in an unconditional branch`` in D150048.

[alvinhochun]

Apply suggestions: remove TODO, put negative tests in the same file as positive tests.

clang/test/CodeGen/ms-inline-asm-functions.c

int foo(void) {
   // CHECK:     calll   _k
 
   // Marginally harder: indirect calls, via dllimport or function pointer.
   __asm call r;
   // CHECK:     calll   *({{.*}})
   __asm call kimport;
   // CHECK:     calll   *({{.*}})
 
-  // Broken case: Call through a global function pointer.
+  // Call through a global function pointer.
   __asm call kptr;
-  // CHECK:     calll   _kptr
-  // CHECK-FIXME: calll   *_kptr
+  // CHECK:     calll   *_kptr
 }
 
 int bar(void) {
   // CHECK-LABEL: _bar:
   __asm {
     jmp k
     ja k
     JAE k

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

class IntelExprStateMachine {
     unsigned BaseReg = 0, IndexReg = 0, TmpReg = 0, Scale = 0;
     int64_t Imm = 0;
     const MCExpr *Sym = nullptr;
     StringRef SymName;
     InfixCalculator IC;
     InlineAsmIdentifierInfo Info;
     short BracCount = 0;
     bool MemExpr = false;
+    bool BracketUsed = false;
     bool OffsetOperator = false;
     bool AttachToOperandIdx = false;
     bool IsPIC = false;
     SMLoc OffsetOperatorLoc;
     AsmTypeInfo CurType;
 
     bool setSymRef(const MCExpr *Val, StringRef ID, StringRef &ErrMsg) {
       if (Sym) {
         ErrMsg = "cannot use more than one symbol in memory operand";
         return true;
       }
       Sym = Val;
       SymName = ID;
       return false;
     }
 
   public:
     IntelExprStateMachine() = default;
 
     void addImm(int64_t imm) { Imm += imm; }
     short getBracCount() const { return BracCount; }
     bool isMemExpr() const { return MemExpr; }
+    bool isBracketUsed() const { return BracketUsed; }
     bool isOffsetOperator() const { return OffsetOperator; }
     SMLoc getOffsetLoc() const { return OffsetOperatorLoc; }
     unsigned getBaseReg() const { return BaseReg; }
     unsigned getIndexReg() const { return IndexReg; }
     unsigned getScale() const { return Scale; }
     const MCExpr *getSym() const { return Sym; }
     StringRef getSymName() const { return SymName; }
     StringRef getType() const { return CurType.Name; }
bool onLBrac() {
         break;
       case IES_INIT:
       case IES_CAST:
         assert(!BracCount && "BracCount should be zero on parsing's start");
         State = IES_LBRAC;
         break;
       }
       MemExpr = true;
+      BracketUsed = true;
       BracCount++;
       return false;
     }
     bool onRBrac(StringRef &ErrMsg) {
       IntelExprState CurrState = State;
       switch (State) {
       default:
         State = IES_ERROR;
return CreateMemForMSInlineAsm(RegNo, Disp, BaseReg, IndexReg, Scale, Start,
                                    End, Size, SM.getSymName(),
                                    SM.getIdentifierInfo(), Operands);
 
   // When parsing x64 MS-style assembly, all non-absolute references to a named
   // variable default to RIP-relative.
   unsigned DefaultBaseReg = X86::NoRegister;
   bool MaybeDirectBranchDest = true;
 
-  if (Parser.isParsingMasm()) {
-    bool IsUnconditionalBranch =
-        Name.equals_insensitive("jmp") || Name.equals_insensitive("call");
+  if (Parser.isParsingMasm()) {
     if (is64BitMode() && SM.getElementSize() > 0) {
       DefaultBaseReg = X86::RIP;
     }
     if (IsUnconditionalBranch) {
       if (PtrInOperand) {
         MaybeDirectBranchDest = false;
         if (is64BitMode())
           DefaultBaseReg = X86::RIP;
       } else if (!BaseReg && !IndexReg && Disp &&
                  Disp->getKind() == MCExpr::SymbolRef) {
         if (is64BitMode()) {
           if (SM.getSize() == 8) {
             MaybeDirectBranchDest = false;
             DefaultBaseReg = X86::RIP;
           }
         } else {
           if (SM.getSize() == 4 || SM.getSize() == 2)
             MaybeDirectBranchDest = false;
         }
       }
     }
+  } else if (IsUnconditionalBranch) {
+    if (PtrInOperand || SM.isBracketUsed())
+      MaybeDirectBranchDest = false;
   }
 
   if ((BaseReg || IndexReg || RegNo || DefaultBaseReg != X86::NoRegister))
     Operands.push_back(X86Operand::CreateMem(
         getPointerWidth(), RegNo, Disp, BaseReg, IndexReg, Scale, Start, End,
         Size, DefaultBaseReg, /*SymName=*/StringRef(), /*OpDecl=*/nullptr,
         /*FrontendSize=*/0, /*UseUpRegs=*/false, MaybeDirectBranchDest));
   else

llvm/test/MC/X86/intel-syntax-branch.s

+// RUN: llvm-mc -triple i686-unknown-unknown -x86-asm-syntax=intel %s | FileCheck %s --check-prefixes=CHECK-32,CHECK
+// RUN: llvm-mc -triple x86_64-unknown-unknown --defsym X64=1 -x86-asm-syntax=intel %s | FileCheck %s --check-prefixes=CHECK-64,CHECK
+
+// RUN: not llvm-mc -triple i686-unknown-unknown --defsym ERR=1 -x86-asm-syntax=intel %s 2>&1 | FileCheck %s --check-prefixes=ERR-32
+
+t0:
+call direct_branch
+jmp direct_branch
+// CHECK-LABEL: t0:
+// CHECK-64: callq direct_branch
+// CHECK-32: calll direct_branch
+// CHECK:    jmp direct_branch
+
+t1:
+call [fn_ref]
+jmp [fn_ref]
+// CHECK-LABEL: t1:
+// CHECK-64: callq *fn_ref
+// CHECK-64: jmpq *fn_ref
+// CHECK-32: calll *fn_ref
+// CHECK-32: jmpl *fn_ref
+
+.ifdef X64
+
+  t2:
+  call qword ptr [fn_ref]
+  jmp qword ptr [fn_ref]
+  // CHECK-64-LABEL: t2:
+  // CHECK-64: callq *fn_ref
+  // CHECK-64: jmpq *fn_ref
+
+  t3:
+  call qword ptr [rip + fn_ref]
+  jmp qword ptr [rip + fn_ref]
+  // CHECK-64-LABEL: t3:
+  // CHECK-64: callq *fn_ref(%rip)
+  // CHECK-64: jmpq *fn_ref(%rip)
+
+.else
+
+  t4:
+  call dword ptr [fn_ref]
+  jmp dword ptr [fn_ref]
+  // CHECK-32-LABEL: t4:
+  // CHECK-32: calll *fn_ref
+  // CHECK-32: jmpl *fn_ref
+
+  t5:
+  call dword ptr fn_ref
+  jmp dword ptr fn_ref
+  // CHECK-32-LABEL: t5:
+  // CHECK-32: calll *fn_ref
+  // CHECK-32: jmpl *fn_ref
+
+  t6:
+  call dword ptr [offset fn_ref]
+  jmp dword ptr [offset fn_ref]
+  // CHECK-32-LABEL: t6:
+  // CHECK-32: calll *fn_ref
+  // CHECK-32: jmpl *fn_ref
+
+.ifdef ERR
+
+  call offset fn_ref
+  // ERR-32: {{.*}}.s:[[#@LINE-1]]:3: error: invalid operand for instruction
+  jmp offset fn_ref
+  // ERR-32: {{.*}}.s:[[#@LINE-1]]:3: error: invalid operand for instruction
+
+.endif
+
+.endif