From c449460d8a657e46500ad12be457928207207e3e Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Mon, 27 Nov 2023 07:27:50 -0800 Subject: [PATCH] ms inline asm: Fix {call,jmp} fptr (#73207) https://reviews.llvm.org/D151863 (2023-05) removed `BaseReg = BaseReg ? BaseReg : 1` (introduced in commit 175d0aeef3725ce17032e9ef76e018139f2f52f0 (2013)) and caused a regression: ensuring a non-zero `BaseReg` was to treat `static void (*fptr)(); __asm { call fptr }` as non-`AbsMem` `AsmOperandClass` and generate `call $0`/`callq *fptr(%rip)` instead of `call ${0:P}`/`callq *fptr` (The asm template argument modifier `P` is for call targets, while no modifier is used by other instructions like `mov rax, fptr`) This patch reinstates the BaseReg-setting statement but places it inside `if (IsGlobalLV)` for clarify. The special case is unfortunate, but we also have special case in similar places (https://reviews.llvm.org/D149920). Fix: #73033 --- clang/test/CodeGen/ms-inline-asm-64.c | 7 +++- .../lib/Target/X86/AsmParser/X86AsmParser.cpp | 38 +++++++++++-------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/clang/test/CodeGen/ms-inline-asm-64.c b/clang/test/CodeGen/ms-inline-asm-64.c index 313d380e121b..c7e4c1b603bd 100644 --- a/clang/test/CodeGen/ms-inline-asm-64.c +++ b/clang/test/CodeGen/ms-inline-asm-64.c @@ -60,17 +60,22 @@ int t4(void) { } void bar() {} +static void (*fptr)(); void t5(void) { __asm { call bar jmp bar + call fptr + jmp fptr } // CHECK: t5 // CHECK: call void asm sideeffect inteldialect // CHECK-SAME: call ${0:P} // CHECK-SAME: jmp ${1:P} - // CHECK-SAME: "*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(void (...)) @bar, ptr elementtype(void (...)) @bar) + // CHECK-SAME: call $2 + // CHECK-SAME: jmp $3 + // CHECK-SAME: "*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(void (...)) @bar, ptr elementtype(void (...)) @bar, ptr elementtype(ptr) @fptr, ptr elementtype(ptr) @fptr) } void t47(void) { diff --git a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp index 6c6ef95f9275..32ede8558efe 100644 --- a/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp +++ b/llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp @@ -1143,8 +1143,8 @@ private: bool ParseIntelMemoryOperandSize(unsigned &Size); bool CreateMemForMSInlineAsm(unsigned SegReg, const MCExpr *Disp, unsigned BaseReg, unsigned IndexReg, - unsigned Scale, SMLoc Start, SMLoc End, - unsigned Size, StringRef Identifier, + unsigned Scale, bool NonAbsMem, SMLoc Start, + SMLoc End, unsigned Size, StringRef Identifier, const InlineAsmIdentifierInfo &Info, OperandVector &Operands); @@ -1744,10 +1744,13 @@ bool X86AsmParser::parseOperand(OperandVector &Operands, StringRef Name) { return parseATTOperand(Operands); } -bool X86AsmParser::CreateMemForMSInlineAsm( - unsigned SegReg, const MCExpr *Disp, unsigned BaseReg, unsigned IndexReg, - unsigned Scale, SMLoc Start, SMLoc End, unsigned Size, StringRef Identifier, - const InlineAsmIdentifierInfo &Info, OperandVector &Operands) { +bool X86AsmParser::CreateMemForMSInlineAsm(unsigned SegReg, const MCExpr *Disp, + unsigned BaseReg, unsigned IndexReg, + unsigned Scale, bool NonAbsMem, + SMLoc Start, SMLoc End, + unsigned Size, StringRef Identifier, + const InlineAsmIdentifierInfo &Info, + OperandVector &Operands) { // If we found a decl other than a VarDecl, then assume it is a FuncDecl or // some other label reference. if (Info.isKind(InlineAsmIdentifierInfo::IK_Label)) { @@ -1772,11 +1775,15 @@ bool X86AsmParser::CreateMemForMSInlineAsm( } // It is widely common for MS InlineAsm to use a global variable and one/two // registers in a mmory expression, and though unaccessible via rip/eip. - if (IsGlobalLV && (BaseReg || IndexReg)) { - Operands.push_back(X86Operand::CreateMem(getPointerWidth(), Disp, Start, - End, Size, Identifier, Decl, 0, - BaseReg && IndexReg)); - return false; + if (IsGlobalLV) { + if (BaseReg || IndexReg) { + Operands.push_back(X86Operand::CreateMem(getPointerWidth(), Disp, Start, + End, Size, Identifier, Decl, 0, + BaseReg && IndexReg)); + return false; + } + if (NonAbsMem) + BaseReg = 1; // Make isAbsMem() false } Operands.push_back(X86Operand::CreateMem( getPointerWidth(), SegReg, Disp, BaseReg, IndexReg, Scale, Start, End, @@ -2620,9 +2627,12 @@ bool X86AsmParser::parseIntelOperand(OperandVector &Operands, StringRef Name) { CheckBaseRegAndIndexRegAndScale(BaseReg, IndexReg, Scale, is64BitMode(), ErrMsg)) return Error(Start, ErrMsg); + bool IsUnconditionalBranch = + Name.equals_insensitive("jmp") || Name.equals_insensitive("call"); if (isParsingMSInlineAsm()) - return CreateMemForMSInlineAsm(RegNo, Disp, BaseReg, IndexReg, Scale, Start, - End, Size, SM.getSymName(), + return CreateMemForMSInlineAsm(RegNo, Disp, BaseReg, IndexReg, Scale, + IsUnconditionalBranch && is64BitMode(), + Start, End, Size, SM.getSymName(), SM.getIdentifierInfo(), Operands); // When parsing x64 MS-style assembly, all non-absolute references to a named @@ -2630,8 +2640,6 @@ bool X86AsmParser::parseIntelOperand(OperandVector &Operands, StringRef Name) { unsigned DefaultBaseReg = X86::NoRegister; bool MaybeDirectBranchDest = true; - bool IsUnconditionalBranch = - Name.equals_insensitive("jmp") || Name.equals_insensitive("call"); if (Parser.isParsingMasm()) { if (is64BitMode() && SM.getElementSize() > 0) { DefaultBaseReg = X86::RIP;