[ms] [llvm-ml] Use default RIP-relative addressing for x64 MASM.

[epastor]

Summary

When parsing 64-bit MASM, treat memory operands with unspecified base register as RIP-based.

Documented in several places, including https://software.intel.com/en-us/articles/introduction-to-x64-assembly: "Unfortunately, MASM does not allow this form of opcode, but other assemblers like FASM and YASM do. Instead, MASM embeds RIP-relative addressing implicitly."

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[epastor]

Rebase

[epastor]

Rebase

[epastor]

Rebase on top of corrected predecessors

[epastor]

Add basic regression test

[thakis]

Yuck.

What's the plan for 64-bit inline asm?

[epastor]

In D73227#1891874, @thakis wrote:

Yuck.

There's a reasonable debate that this reflects the x64 instruction set better, but yeah.

What's the plan for 64-bit inline asm?

It should probably be the same, admittedly. To clarify: isParsingInlineAsm will only be true when parsing inline MS-style assembly? If so, I'll rename it to isParsingInlineMSAsm in this patch, and let the handling trigger off of both.

[mstorsjo]

In D73227#1891896, @epastor wrote:

In D73227#1891874, @thakis wrote:

What's the plan for 64-bit inline asm?

It should probably be the same, admittedly. To clarify: isParsingInlineAsm will only be true when parsing inline MS-style assembly? If so, I'll rename it to isParsingInlineMSAsm in this patch, and let the handling trigger off of both.

Are there any cases where MS style assembly can be injected inline on x86_64? As far as I know, MSVC didn't support any inline assembly at all on platforms other than i386.

If that's the case, I think the sane thing would be to keep handling inline asm like it always has been, as gcc style inline assembly. There could, concievably, be projects that contains gcc style inline assembly within #ifdef __clang__ (as this probably worked so far).

[thakis]

In D73227#1891917, @mstorsjo wrote:

In D73227#1891896, @epastor wrote:

In D73227#1891874, @thakis wrote:

What's the plan for 64-bit inline asm?

It should probably be the same, admittedly. To clarify: isParsingInlineAsm will only be true when parsing inline MS-style assembly? If so, I'll rename it to isParsingInlineMSAsm in this patch, and let the handling trigger off of both.

Are there any cases where MS style assembly can be injected inline on x86_64? As far as I know, MSVC didn't support any inline assembly at all on platforms other than i386.

If that's the case, I think the sane thing would be to keep handling inline asm like it always has been, as gcc style inline assembly. There could, concievably, be projects that contains gcc style inline assembly within #ifdef __clang__ (as this probably worked so far).

I'd argue that the sane thing would be if x86_64 inline asm when targeting the msvc abi worked like llvm-ml (and ml64) works. Anything else seems extremely surprising to me.

Anyhoo, if that's the long-term plan I'd try starting with the inline asm part of that change, and then make llvm-ml match that.

[epastor]

In D73227#1891917, @mstorsjo wrote:

In D73227#1891896, @epastor wrote:

In D73227#1891874, @thakis wrote:

What's the plan for 64-bit inline asm?

It should probably be the same, admittedly. To clarify: isParsingInlineAsm will only be true when parsing inline MS-style assembly? If so, I'll rename it to isParsingInlineMSAsm in this patch, and let the handling trigger off of both.

Are there any cases where MS style assembly can be injected inline on x86_64? As far as I know, MSVC didn't support any inline assembly at all on platforms other than i386.

If that's the case, I think the sane thing would be to keep handling inline asm like it always has been, as gcc style inline assembly. There could, concievably, be projects that contains gcc style inline assembly within #ifdef __clang__ (as this probably worked so far).

It's true, MSVC doesn't support inline assembly at all on non-i386 platforms.

However, LLVM currently does support MS-style inline assembly on x86-64, if you use the MS-style asm blocks... it just doesn't quite work as expected. (We've seen a few bug reports to this effect, including http://llvm.org/PR44272 - several of which have said the option of MS-style inline assembly on x86-64 is a big bonus.) Handling that here would move it closer to fully working.

[epastor]

Support default RIP-relative addressing on x64 MS-style assembly (including inline).

[mstorsjo]

In D73227#1891931, @epastor wrote:

It's true, MSVC doesn't support inline assembly at all on non-i386 platforms.

However, LLVM currently does support MS-style inline assembly on x86-64, if you use the MS-style asm blocks... it just doesn't quite work as expected. (We've seen a few bug reports to this effect, including http://llvm.org/PR44272 - several of which have said the option of MS-style inline assembly on x86-64 is a big bonus.) Handling that here would move it closer to fully working.

Oh, ok - I see. In that case, keeping the gcc style inline assembly behaviour for asm() blocks and ms style for __asm {} probably is best - if it isn't a huge extra effort.

[epastor]

Rebase

[epastor]

MS-style inline assembly on x64 should also benefit now.

[rnk]

llvm/include/llvm/MC/MCParser/MCAsmParser.h

168–169

I guess I'd prefer isParsingMSInlineAsm. Do you mind uploading that and committing it separately?

171

We already have ASMLexer::LexMasmIntegers. Can this be collapsed with that so that we don't have two indicators of "masm-ness"?

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

2072–2073

Does this not make the check for isParsingInlindMSAsm() below redundant? If it's true, we always return here.

2078–2080

What ends up going wrong if we set the basereg to RIP in these conditions when it is not already set? I'm wondering if it would be cleaner to do the defaulting here, and then adjust the downstream code to treat RIP-based memory operands the same as no-base memory operands.

llvm/lib/Target/X86/AsmParser/X86Operand.h

61

I'd really like to get by without adding more operand members that don't correspond to parts of the x86 addressing mode.

[epastor]

Responses to rnk's comments

[epastor]

llvm/include/llvm/MC/MCParser/MCAsmParser.h

168–169

Rename split out as a prerequisite differential: https://reviews.llvm.org/D75198

171

Since that's a Lexer control property, we might need to stick to setting it based on isParsingMasm() instead...

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

2072–2073

Whoops. Yep, revised to set the default base register to RIP in CreateMemForInlineAsm instead.

2078–2080

I started by doing that, and we ended up with RIP as the BaseReg in many inappropriate situations. The issue is that this only applies when the Operand is matched as a non-absolute reference... which we can't tell at parse time.

It would have been much cleaner, but since the interpretation is context-dependent, I couldn't find a better option. I'm open to suggestions, though!

llvm/lib/Target/X86/AsmParser/X86Operand.h

61

Agreed. Unfortunately, the only obvious way to do that would be to match the x64 addressing mode more closely, and assume that all non-absolute memory references lacking a BaseReg are RIP-relative... which would break GNU-style assembly, since they made the decision to enforce explicit RIP-relative referencing.

[epastor]

Rebasing to HEAD

[thakis]

llvm/lib/Target/X86/AsmParser/X86Operand.h

61

It sounds like ms-style 64-bit assembly with a msvc triple will already be incompatible with gnu-style assembly, due to the rip-relative differences. Is that incorrect?

If not, I think we should have the GNU mode and the msvc mode here, and not a third kinda-hybrid mode. It sounds like this is currently some hybrid mode.

If needed, we could have the full commandline flag / pragma dance for picking a mode for inline asm and only setting the default mode off the triple if use cases for that emerge, but that can happen later.

(Maybe I misunderstood what you're saying, though.)

[epastor]

llvm/lib/Target/X86/AsmParser/X86Operand.h

61

What I'm saying is that we can't know whether or not to assume RIP-relative (in MSVC mode) until AFTER the Operand has been matched to an instruction, as only MASM instructions that allow relative addressing will default to RIP-relative if no other base register is specified. However, once the Operand is matched, we lose information on whether it's come from MASM or GNU-style assembly. (We do allow GNU-style assembly for MSVC platforms already.) This means that when we're finally about to emit our instruction and need the base register, we don't know whether to translate a NoReg base as actually implying RIP.

To handle that, I opted to extend the Operand here with a DefaultBaseReg, which we use as our Base if no base register has been specified. When we parse GNU-style assembly, this defaults to NoReg... when we parse MASM-style assembly, it gets set to RIP.

[epastor]

Any further thoughts here?

[epastor]

Rebase to HEAD

[epastor]

llvm/lib/Target/X86/AsmParser/X86Operand.h

61

One more try at clarifying this: MS-style assembly with an MSVC triple and GNU-style assembly with an MSVC triple both work now, and will both work in the future. The only difference is that MS-style assembly instructions that allow relative addressing will use a default RIP-relative address, while GNU-style assembly forces the user to always be explicit about RIP-relative addressing.

Since only relative-addressing contexts in MS-style assembly use the implicit RIP-relative convention, we can't just pick a mode and stick with it. Instead, we label our Operands with how their source assembly wants them treated if used in relative-addressing context without a base register. After we've matched in relative-addressing context, if the base register is not present, we implement the approach given by the source assembly.

[epastor]

Rebase against HEAD

[epastor]

Rebasing to HEAD

[thakis]

I think being compatible with ml64 by default is good, and making progress here seems better than holding this up for even longer.

Going forward, we likely will want to have the full suite of toggles for this (pragma, compiler flag, accepting an alternative form with an explicit RIP, etc), but let's start with this.

[epastor]

Rebase to HEAD

llvm/include/llvm/MC/MCParser/MCAsmParser.h

public:
   virtual void setAssemblerDialect(unsigned i) { }
 
   bool getShowParsedOperands() const { return ShowParsedOperands; }
   void setShowParsedOperands(bool Value) { ShowParsedOperands = Value; }
 
   /// Run the parser on the input source buffer.
   virtual bool Run(bool NoInitialTextSection, bool NoFinalize = false) = 0;
 
   virtual void setParsingMSInlineAsm(bool V) = 0;
   virtual bool isParsingMSInlineAsm() = 0;
 
+  virtual bool isParsingMasm() const { return false; }
+
   /// Parse MS-style inline assembly.
   virtual bool parseMSInlineAsm(
       void *AsmLoc, std::string &AsmString, unsigned &NumOutputs,
       unsigned &NumInputs, SmallVectorImpl<std::pair<void *, bool>> &OpDecls,
       SmallVectorImpl<std::string> &Constraints,
       SmallVectorImpl<std::string> &Clobbers, const MCInstrInfo *MII,
       const MCInstPrinter *IP, MCAsmParserSemaCallback &SI) = 0;
 

llvm/lib/MC/MCParser/MasmParser.cpp

public:
   void setParsingMSInlineAsm(bool V) override {
     ParsingMSInlineAsm = V;
     // When parsing MS inline asm, we must lex 0b1101 and 0ABCH as binary and
     // hex integer literals.
     Lexer.setLexMasmIntegers(V);
   }
   bool isParsingMSInlineAsm() override { return ParsingMSInlineAsm; }
 
+  bool isParsingMasm() const override { return true; }
+
   bool parseMSInlineAsm(void *AsmLoc, std::string &AsmString,
                         unsigned &NumOutputs, unsigned &NumInputs,
                         SmallVectorImpl<std::pair<void *,bool>> &OpDecls,
                         SmallVectorImpl<std::string> &Constraints,
                         SmallVectorImpl<std::string> &Clobbers,
                         const MCInstrInfo *MII, const MCInstPrinter *IP,
                         MCAsmParserSemaCallback &SI) override;
 

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

std::unique_ptr<X86Operand> X86AsmParser::CreateMemForMSInlineAsm(
   if (IsGlobalLV && (BaseReg || IndexReg)) {
     return X86Operand::CreateMem(getPointerWidth(), Disp, Start, End);
   // Otherwise, we set the base register to a non-zero value
   // if we don't know the actual value at this time.  This is necessary to
   // get the matching correct in some cases.
   } else {
     BaseReg = BaseReg ? BaseReg : 1;
     return X86Operand::CreateMem(getPointerWidth(), SegReg, Disp, BaseReg,
-                                 IndexReg, Scale, Start, End, Size, Identifier,
-                                 Decl, FrontendSize);
+                                 IndexReg, Scale, Start, End, Size,
+                                 /*DefaultBaseReg=*/X86::RIP, Identifier, Decl,
+                                 FrontendSize);
   }
 }
 
 // Some binary bitwise operators have a named synonymous
 // Query a candidate string for being such a named operator
 // and if so - invoke the appropriate handler
 bool X86AsmParser::ParseIntelNamedOperator(StringRef Name,
                                            IntelExprStateMachine &SM,
std::unique_ptr<X86Operand> X86AsmParser::ParseIntelOperand() {
   // shared with att syntax where order matters.
   if ((BaseReg == X86::SI || BaseReg == X86::DI) &&
       (IndexReg == X86::BX || IndexReg == X86::BP))
     std::swap(BaseReg, IndexReg);
 
   if ((BaseReg || IndexReg) &&
       CheckBaseRegAndIndexRegAndScale(BaseReg, IndexReg, Scale, is64BitMode(),
                                       ErrMsg))
     return ErrorOperand(Start, ErrMsg);
   if (isParsingMSInlineAsm())
     return CreateMemForMSInlineAsm(RegNo, Disp, BaseReg, IndexReg, Scale, Start,
                                    End, Size, SM.getSymName(),
                                    SM.getIdentifierInfo());
+
+  // When parsing x64 MS-style assembly, all memory operands default to
+  // RIP-relative when interpreted as non-absolute references.
+  if (Parser.isParsingMasm() && is64BitMode())
+    return X86Operand::CreateMem(getPointerWidth(), RegNo, Disp, BaseReg,
+                                 IndexReg, Scale, Start, End, Size,
+                                 /*DefaultBaseReg=*/X86::RIP);
+
   if (!(BaseReg || IndexReg || RegNo))
     return X86Operand::CreateMem(getPointerWidth(), Disp, Start, End, Size);
   return X86Operand::CreateMem(getPointerWidth(), RegNo, Disp,
                                BaseReg, IndexReg, Scale, Start, End, Size);
 }
 
 std::unique_ptr<X86Operand> X86AsmParser::ParseATTOperand() {
   MCAsmParser &Parser = getParser();

llvm/lib/Target/X86/AsmParser/X86Operand.h

struct ImmOp {
     const MCExpr *Val;
     bool LocalRef;
   };
 
   struct MemOp {
     unsigned SegReg;
     const MCExpr *Disp;
     unsigned BaseReg;
+    unsigned DefaultBaseReg;
     unsigned IndexReg;
     unsigned Scale;
     unsigned Size;
     unsigned ModeSize;
 
     /// If the memory operand is unsized and there are multiple instruction
     /// matches, prefer the one with this size.
     unsigned FrontendSize;
struct X86Operand final : public MCParsedAsmOperand {
   unsigned getMemSegReg() const {
     assert(Kind == Memory && "Invalid access!");
     return Mem.SegReg;
   }
   unsigned getMemBaseReg() const {
     assert(Kind == Memory && "Invalid access!");
     return Mem.BaseReg;
   }
+  unsigned getMemDefaultBaseReg() const {
+    assert(Kind == Memory && "Invalid access!");
+    return Mem.DefaultBaseReg;
+  }
   unsigned getMemIndexReg() const {
     assert(Kind == Memory && "Invalid access!");
     return Mem.IndexReg;
   }
   unsigned getMemScale() const {
     assert(Kind == Memory && "Invalid access!");
     return Mem.Scale;
   }
case X86::K7:
       Reg = X86::K6_K7;
       break;
     }
     Inst.addOperand(MCOperand::createReg(Reg));
   }
 
   void addMemOperands(MCInst &Inst, unsigned N) const {
     assert((N == 5) && "Invalid number of operands!");
+    if (getMemBaseReg())
       Inst.addOperand(MCOperand::createReg(getMemBaseReg()));
+    else
+      Inst.addOperand(MCOperand::createReg(getMemDefaultBaseReg()));
     Inst.addOperand(MCOperand::createImm(getMemScale()));
     Inst.addOperand(MCOperand::createReg(getMemIndexReg()));
     addExpr(Inst, getMemDisp());
     Inst.addOperand(MCOperand::createReg(getMemSegReg()));
   }
 
   void addAbsMemOperands(MCInst &Inst, unsigned N) const {
     assert((N == 1) && "Invalid number of operands!");
struct X86Operand final : public MCParsedAsmOperand {
   static std::unique_ptr<X86Operand>
   CreateMem(unsigned ModeSize, const MCExpr *Disp, SMLoc StartLoc, SMLoc EndLoc,
             unsigned Size = 0, StringRef SymName = StringRef(),
             void *OpDecl = nullptr, unsigned FrontendSize = 0) {
     auto Res = std::make_unique<X86Operand>(Memory, StartLoc, EndLoc);
     Res->Mem.SegReg   = 0;
     Res->Mem.Disp     = Disp;
     Res->Mem.BaseReg  = 0;
+    Res->Mem.DefaultBaseReg = 0;
     Res->Mem.IndexReg = 0;
     Res->Mem.Scale    = 1;
     Res->Mem.Size     = Size;
     Res->Mem.ModeSize = ModeSize;
     Res->Mem.FrontendSize = FrontendSize;
     Res->SymName      = SymName;
     Res->OpDecl       = OpDecl;
     Res->AddressOf    = false;
     return Res;
   }
 
   /// Create a generalized memory operand.
   static std::unique_ptr<X86Operand>
   CreateMem(unsigned ModeSize, unsigned SegReg, const MCExpr *Disp,
             unsigned BaseReg, unsigned IndexReg, unsigned Scale, SMLoc StartLoc,
-            SMLoc EndLoc, unsigned Size = 0, StringRef SymName = StringRef(),
-            void *OpDecl = nullptr, unsigned FrontendSize = 0) {
+            SMLoc EndLoc, unsigned Size = 0,
+            unsigned DefaultBaseReg = X86::NoRegister,
+            StringRef SymName = StringRef(), void *OpDecl = nullptr,
+            unsigned FrontendSize = 0) {
     // We should never just have a displacement, that should be parsed as an
     // absolute memory operand.
-    assert((SegReg || BaseReg || IndexReg) && "Invalid memory operand!");
+    assert((SegReg || BaseReg || IndexReg || DefaultBaseReg) &&
+           "Invalid memory operand!");
 
     // The scale should always be one of {1,2,4,8}.
     assert(((Scale == 1 || Scale == 2 || Scale == 4 || Scale == 8)) &&
            "Invalid scale!");
     auto Res = std::make_unique<X86Operand>(Memory, StartLoc, EndLoc);
     Res->Mem.SegReg   = SegReg;
     Res->Mem.Disp     = Disp;
     Res->Mem.BaseReg  = BaseReg;
+    Res->Mem.DefaultBaseReg = DefaultBaseReg;
     Res->Mem.IndexReg = IndexReg;
     Res->Mem.Scale    = Scale;
     Res->Mem.Size     = Size;
     Res->Mem.ModeSize = ModeSize;
     Res->Mem.FrontendSize = FrontendSize;
     Res->SymName      = SymName;
     Res->OpDecl       = OpDecl;
     Res->AddressOf    = false;
     return Res;
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_X86_ASMPARSER_X86OPERAND_H

llvm/test/tools/llvm-ml/rip-relative-addressing.test

+# RUN: llvm-ml -m32 -filetype=asm %s | FileCheck %s --check-prefix=CHECK-32
+# RUN: llvm-ml -m64 -filetype=asm %s | FileCheck %s --check-prefix=CHECK-64
+
+.code
+mov eax, [4]
+; CHECK-32: mov eax, dword ptr [4]
+; CHECK-64: mov eax, dword ptr [rip + 4]
+END
 No newline at end of file