Microsoft compatibility – add support for “relaxation” of memory operands in inline assembly.

[m_zuckerman]

Summary

Microsoft compiler performs “relaxation” of the X86 instructions and expands the set of legal instruction forms that can be used in the X86 inline assembly.
Microsoft achieves this by transforming instructions that have an illegal form according to X86 spec into a legal one.
For example, the illegal instruction form:
mov r16, m32
Is transformed into the following legal form (based on the type of the register operand):
mov r16, m16

There are many instructions that have such “relaxations” and they are all needed in order for llvm to be consistent with Microsoft compiler.

[t.p.northover]

I don't think this should be supported outside a specific Microsoft mode (and not there if we can possibly avoid it). Those instructions are simply wrong, and should be diagnosed. Many of them even have two equally likely valid interpretations.

Tim.

[myatsina]

I agree with you that there are several possible interpretations for many of the instructions, and we chose only one interpretation - the one that allows Microsoft compatibility.

We found a very large number of instructions (>500) for which Microsoft compiler performs such "relaxations", therefore, patching the X86AsmParser with all these special cases seems as an unreasonable solution for such a wide-spread problem. We chose to use the already exiting td infrastructure and refine the instructions already defined in it.

The changes in this review affect only the intel syntax assembly, so the relaxations are somewhat contained (in at&t syntax you do not specify the sizes of each pointer operand, you just add a size suffix to the instruction, therefore, you don’t encounter such situations where there is a mismatch between the operands).

Do you have a suggestion how the definitions in the td files can be further contained to "Microsoft only" syntax? Or perhaps, do you have another suggestion for an infrastructure that will allow adding support for so many “relaxations” without the need to repeat the instruction definitions that already exist in the td files?

[t.p.northover]

I agree with you that there are several possible interpretations for many of the instructions, and we chose only one interpretation - the one that allows Microsoft compatibility.

It goes beyond a simple ambiguity. This patch takes an explicitly self-contradictory instruction, discards an arbitrary part of what the programmer wrote and ploughs on regardless. That's simply a buggy assembler, not a "relaxation" in any conventional sense. I'd hope even most programmers targeting Microsoft platforms would prefer to be told that they'd made that kind of mistake. I certainly would on Linux or OSX.

As for achieving compatibility without breaking other OSs, I'm not sure. Maybe introduce a third syntax, beyond AT&T and Intel (I'd be as snarky as possible about the name, personally "ms_buggy" perhaps. "ms_compat" would be a more diplomatic choice). A modified ParseIntelMemOperand could then put a field in MemOp effectively saying "this was parsed from potentially buggy source", which isBuggyMem8 could then check for.

It's really ugly, and I wouldn't be surprised if there was a better way. But then I wouldn't be surprised if there wasn't, either.

Tim.

[rnk]

Is this something that masm does, or something that cl does?

Is the goal here to make LLVM's assembler accept everything masm accepts, or to help it accept inline assembly coming from Clang?

[myatsina]

We've been checking the inline assembly against cl.exe and cl.exe accepts these expansions and turns them into legal instructions.

The goal is Microsoft compatibility - make llvm accept what Microsoft accepts.

We're looking into Tim's proposal of introducing a third syntax.

[rnk]

In D12399#237111, @myatsina wrote:

We've been checking the inline assembly against cl.exe and cl.exe accepts these expansions and turns them into legal instructions.

The goal is Microsoft compatibility - make llvm accept what Microsoft accepts.

I want to know if masm behaves differently from MSVC here. I would like to make this logic conditional on parsing inline intel assembly if possible.

For a standalone assembly files, we have a bit more leeway to remain strict. There aren't as many cargo-culted assembly system headers as there are for C/C++.

We're looking into Tim's proposal of introducing a third syntax.

[m_zuckerman]

This comment was removed by m_zuckerman.

[m_zuckerman]

First, we all agree this is a point of view issue , Strict sence vs wide sence .

I'd hope even most programmers targeting Microsoft platforms would prefer to be told that they'd made that kind of mistake.

I agree with you that the compiler should alert the programmers when they use these “exapnsions”.

I want to know if masm behaves differently from MSVC here.

Yes masm behaves differently. In MSVC you can write illegal memory size (by spec) and get the correct asm. In llvm this is not supported , Only spec instrucations .

I would like to make this logic conditional on parsing inline intel assembly if possible.

I checked t.p.northover suggestion.

a. To apply third syntax we need to create third table matcher (att,intel,MS_compat):
To do so with the exist infrastructure. We need to change all the 20 X86instr td file and more than 10000 instructions to accepting the third syntax (Change {att|intel} to {att|instel|MS_comp} ).
We can do it , but this is the cost. The impact is bigger than before.

[rnk]

If this is really an MSVC inline assembly compatibility issue, then lets try to fix it in the inline assembly parser. Can you give examples of the inline assembly that users are actually writing? I imagine that the inline assembly parser may be incorrectly infering memory operand sizes here, like in this code:

int g;
void f() {
  __asm mov g, ax
}

I think somtimes clang/llvm says that 'r' is a dword ptr even though the user didn't write that. If users are actually writing mov ax, dword ptr g I'm pretty comfortable with diagnosing it.

[m_zuckerman]

In D12399#237755, @rnk wrote:

If this is really an MSVC inline assembly compatibility issue, then lets try to fix it in the inline assembly parser. Can you give examples of the inline assembly that users are actually writing? I imagine that the inline assembly parser may be incorrectly infering memory operand sizes here, like in this code:

int g;
void f() {
  __asm mov g, ax
}

I think somtimes clang/llvm says that 'r' is a dword ptr even though the user didn't write that. If users are actually writing mov ax, dword ptr g I'm pretty comfortable with diagnosing it.

I ran some tests to compare the MASM and cl.exe ( MSVC ) . As I wrote MASM behaves differently from MSVC . For example, I use BTC instruction. This is for MSVC
char test_mem[16];
int main(void)
{
__asm {

btc xmmword ptr test_mem, 8
 btc xmmword ptr test_mem, eax
 btc xmmword ptr test_mem, eax

}
return 0;
}
This is the asm output:
char test_mem[16];

int main(void)
{
push ebp
mov ebp,esp
sub esp,0C0h
push ebx
push esi
push edi
lea edi,[ebp-0C0h]
mov ecx,30h
mov eax,0CCCCCCCCh
rep stos dword ptr es:[edi]

__asm {
  btc xmmword ptr test_mem, 8

btc word ptr ds:[3B8130h],8

btc xmmword ptr test_mem, eax

btc dword ptr ds:[3B8130h],eax

btc xmmword ptr test_mem, eax

btc dword ptr ds:[3B8130h],eax

}
return 0;

xor eax,eax
}

When I tried to run it on MASM. The MASM returned with an error massage

C:\masm32\bin>ml C:\work\mzuckerm\RunProj\run.asm
Microsoft (R) Macro Assembler Version 6.14.8444
Copyright (C) Microsoft Corp 1981-1997. All rights reserved.

Assembling: C:\work\mzuckerm\RunProj\run.asm
C:\work\mzuckerm\RunProj\run.asm(17) : error A2006: undefined symbol : xmmword
C:\work\mzuckerm\RunProj\run.asm(18) : error A2006: undefined symbol : xmmword
C:\work\mzuckerm\RunProj\run.asm(19) : error A2006: undefined symbol : xmmword

As you can see the MASM behaves differently from MSVC

[myatsina]

I want to emphasize that there are hundreds of instructions that have these "relaxations", therefore, I think that fixing all of them in the X86AsmParser is not a desired option.

The question remains, what options do we have to support this? Adding a new "MS_compt" syntax to the td files has a high cost on all the td files and all the instructions.

[rnk]

In D12399#240721, @myatsina wrote:

I want to emphasize that there are hundreds of instructions that have these "relaxations", therefore, I think that fixing all of them in the X86AsmParser is not a desired option.

The question remains, what options do we have to support this? Adding a new "MS_compt" syntax to the td files has a high cost on all the td files and all the instructions.

It's not clear to me that we need to enumerate all of the instructions to solve this problem. This seems like it's really about ignoring memory operand sizes of source-level identifiers. That seems like a pretty non-invasive change to the assembly parser.

[m_zuckerman]

In D12399#237685, @m_zuckerman wrote:

a. To apply third syntax we need to create third table matcher (att,intel,MS_compat):
To do so with the exist infrastructure. We need to change all the 20 X86instr td file and more than 10000 instructions to accepting the third syntax (Change {att|intel} to {att|instel|MS_comp} ).
We can do it , but this is the cost. The impact is bigger than before.

We need support from the table matcher to do mapping from not support instruction to support instruction.

For example BTC:
This the spec of BTC- bit test and complement ( intel spec )

Opcode	Instruction
1. 0F BB /r	BTC r/m16,r16
2. 0F BB /r	BTC r/m32,r32
3. Rex.w+0F BB /r	BTC r/m64,r64
4. 0F BA /7 ib	BTC r/m16,imm8
5. 0F BA /7 ib	BTC r/m32,imm8
6. REX.W + 0F BA /7 ib	BTC r/m64,imm8

1. We want to continue to print the supported memory size with the correct opcode (as in spec). For memory size , word- qword still the print needed to be as spec.
2. For memory size that not supported in the spec (byte , tbyte, xmmword ) that not yet supported by inline asm. MSVC map tbye and xmmword to the lowest memory size spec ( word)
3. What happen if we use BTC xmmword ptr test_mem , imm8 which of the 4-6 instruction we will chose. MSVC again chose the minimum instruction (4).

MSVC ASM output
the bold is input
char test_mem[16];

void main (void)
{
push ebp
mov ebp,esp
sub esp,0C0h
push ebx
push esi
push edi
lea edi,[ebp+FFFFFF40h]
mov ecx,30h
mov eax,0CCCCCCCCh
rep stos dword ptr es:[edi]

__asm {

btc XMMWORD PTR test_mem, 4
btc word ptr ds:[00E38130h],4
btc TBYTE PTR test_mem, 4
btc word ptr ds:[00E38130h],4
btc BYTE PTR test_mem, 4
btc word ptr ds:[00E38130h],4
btc XMMWORD PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc QWORD PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc TBYTE PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc DWORD PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc BYTE PTR test_mem, ax
btc word ptr ds:[00E38130h],ax
btc XMMWORD PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax
btc QWORD PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax
btc TBYTE PTR test_mem,eax
btc dword ptr ds:[00E38130h],eax
btc WORD PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax
btc BYTE PTR test_mem, eax
btc dword ptr ds:[00E38130h],eax

}

}

From that I understand we need to create table. That looking for the minimum supported memory. I don’t ignore the memory size. Only not supported memory I ignore and mapping the "ignore" memory to the correct size. In each instruction the minimum size is something else. For that again we need table.

Another example is DEC instruction where the opcode is different from one memory to the other memory. And the minimum memory size here different than the btc.

Opcode	Instruction
1. FE /1	DEC r/m8
2. FF /1	DEC r/m16
3. FF /1	DEC r/m32
4. REX.W+ FF /1	DEC r/m64

bold is input
__asm {
dec QWORD PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]
dec DWORD PTR test_mem
FF 0D 30 81 1D 00 dec dword ptr ds:[001D8130h]
dec WORD PTR test_mem
66 FF 0D 30 81 1D 00 dec word ptr ds:[001D8130h]
dec BYTE PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]
dec XMMWORD PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]
dec TBYTE PTR test_mem
FE 0D 30 81 1D 00 dec byte ptr ds:[001D8130h]

}

}

In conclusion we need to store the information of the minimum memory size for each instruction. For that we will need a table.

[rnk]

In D12399#240955, @m_zuckerman wrote:

In conclusion we need to store the information of the minimum memory size for each instruction. For that we will need a table.

The table already has information about what memory operand sizes each instruction supports.

I'm suggesting that instead of modifying the data in the table, you modify the matcher. Take a look at how we deal with unsized memory operands in X86AsmParser::MatchAndEmitIntelInstruction(). It sounds that logic would solve your problem if we just dropped the user-provided memory operand size on the floor.

The next step is to narrow down the conditions under which we need to do this. I suspect that MSVC ignores user provided operand sizes when a source-level identifier is involved.

[majnemer]

lib/Target/X86/AsmParser/X86AsmParser.cpp

31–33

Would it make more sense to predicate this on whether or not we are doing inline assembly instead of whether or not our OS is Windows?

[rnk]

There are more than a few spelling mistakes in this patch, so I'd appreciate it if you ran spellcheck first. Also consider reformatting the C++ code with clang-format. The instructions for using it on your patch are here: http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

lib/Target/X86/AsmParser/X86AsmParser.cpp

33

I think the right condition here is not "are we targeting Windows" but "are we parsing Intel inline assembly".

test/MC/X86/relaxation.s

1 ↗

(On Diff #38422)

Given my comment above about making this conditional on inline assembly, you will have to make this a clang test for now.

2–7 ↗

(On Diff #38422)

This comment isn't needed, this test isn't different from the other tests in this directory.

[t.p.northover]

lib/Target/X86/AsmParser/X86AsmParser.cpp

31–33

I'm still keen to limit it to Windows somehow. I don't think people writing Intel inline assembly elsewhere will want wrong access size to be silently ignored: either of the contradictory specifiers could be the one intended.

[rnk]

lib/Target/X86/AsmParser/X86AsmParser.cpp

31–33

It seems like we really want this to be protected by -fms-compatibility. I wonder if we should thread that flag through to the assembler.

[m_zuckerman]

lib/Target/X86/AsmParser/X86AsmParser.cpp

31–33

I agree with you about the -fms-compatability. Regarding your saying, “It seems like we really want this to be protected by -fms-compatibility. I wonder if we should thread that flag through to the assembler”, I’ve checked it, and lowering this flag to LLVM is currently not supported by clang. If you know of any way to do it correctly, please let me know. I believe we should first commit the general logic that supports these MS “relaxations” using the triple, and then, at the second stage we should discuss (with clang guys as well) how to connect the “fms-compatability” to this logic instead of the triple.

Unlike “fms-compatability”, the triple is available in the X86AsmParser, and nobody except for users that use intel syntax inlineasm under the Microsoft triple will be affected by this.

[rnk]

Honestly, I don't see the point of doing this. Is there some popular library that needs this compatibility hack? If not, I think we should just tell users to fix their code.

[m_zuckerman]

I am not familiar with any libraries that contain these “relaxations”. Never the less Microsoft supports these “relaxations”. If we want LLVM to be compatible with Microsoft like GAS, we need to add this feature. I don’t think that “just tell users to fix their code” is the correct approach, but we definitely should warn them.
I’ve added support for –fms-compatibility for the “relaxations” to work. The user needs to pass two flags: fams-blocks and fms-compatibility.

lib/CodeGen/BackendUtil.cpp

TargetMachine *EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
   if (!CodeGenOpts.DebugPass.empty()) {
     BackendArgs.push_back("-debug-pass");
     BackendArgs.push_back(CodeGenOpts.DebugPass.c_str());
   }
   if (!CodeGenOpts.LimitFloatPrecision.empty()) {
     BackendArgs.push_back("-limit-float-precision");
     BackendArgs.push_back(CodeGenOpts.LimitFloatPrecision.c_str());
   }
+  if (LangOpts.MSVCCompat) {
+    BackendArgs.push_back("-fms-compatibility");
+  }
   for (const std::string &BackendOption : CodeGenOpts.BackendOptions)
     BackendArgs.push_back(BackendOption.c_str());
   BackendArgs.push_back(nullptr);
   llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,
                                     BackendArgs.data());
 
   std::string FeaturesStr =
       llvm::join(TargetOpts.Features.begin(), TargetOpts.Features.end(), ",");

lib/Target/X86/AsmParser/X86AsmParser.cpp

 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCAsmParser.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCTargetAsmParser.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <memory>
 
 using namespace llvm;
+static cl::opt<bool>
+    MSCompatibility("fms-compatibility", cl::Hidden,
+           cl::init(false));
 
 namespace {
 
 static const char OpPrecedence[] = {
   0, // IC_OR
   1, // IC_XOR
   2, // IC_AND
   3, // IC_LSHIFT
bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
   assert(Op.isToken() && "Leading operand should always be a mnemonic!");
   StringRef Mnemonic = Op.getToken();
   ArrayRef<SMRange> EmptyRanges = None;
 
   // First, handle aliases that expand to multiple instructions.
   MatchFPUWaitAlias(IDLoc, Op, Operands, Out, MatchingInlineAsm);
 
   MCInst Inst;
   
-  // Find one unsized memory operand, if present.
+  // Find one unsized memory operand, if present, or a memory operand that
+  // may need relaxation.
   X86Operand *UnsizedMemOp = nullptr;
+  X86Operand *RelaxedMemOp = nullptr;
   for (const auto &Op : Operands) {
     X86Operand *X86Op = static_cast<X86Operand *>(Op.get());
     if (X86Op->isMemUnsized())
       UnsizedMemOp = X86Op;
+    else if (MSCompatibility && X86Op->isMem())
+      RelaxedMemOp = X86Op;
   }
 
   // Allow some instructions to have implicitly pointer-sized operands.  This is
   // compatible with gas.
   if (UnsizedMemOp) {
     static const char *const PtrSizedInstrs[] = {"call", "jmp", "push"};
     for (const char *Instr : PtrSizedInstrs) {
       if (Mnemonic == Instr) {
         UnsizedMemOp->Mem.Size = getPointerWidth();
         break;
       }
     }
   }
+  // Set the pointer that we will want to strip of size information
+  uint64_t ErrorInfoIgnore;
+  X86Operand *StripSizeMemOp = nullptr;
+  bool DoRelaxation = false;
+  if (RelaxedMemOp) {
+    StripSizeMemOp = RelaxedMemOp;
+    // Determine if we'll need relaxation - if there are no matches with the
+    // original memory size
+    DoRelaxation =
+        !MatchInstructionImpl(Operands, Inst, ErrorInfoIgnore,
+                              MatchingInlineAsm, isParsingIntelSyntax());
+    // Erase Inst info
+    Inst.clear();
+    Inst.setOpcode(0);
+  } else {
+    StripSizeMemOp = UnsizedMemOp;
+  }
 
   // If an unsized memory operand is present, try to match with each memory
   // operand size.  In Intel assembly, the size is not part of the instruction
   // mnemonic.
   SmallVector<unsigned, 8> Match;
   uint64_t ErrorInfoMissingFeature = 0;
-  if (UnsizedMemOp && UnsizedMemOp->isMemUnsized()) {
+  if ((UnsizedMemOp && UnsizedMemOp->isMemUnsized()) || DoRelaxation) {
+    // Save the original memory size so that it can be restored
+    unsigned OrigSize = StripSizeMemOp->Mem.Size;
     static const unsigned MopSizes[] = {8, 16, 32, 64, 80, 128, 256, 512};
     for (unsigned Size : MopSizes) {
-      UnsizedMemOp->Mem.Size = Size;
-      uint64_t ErrorInfoIgnore;
+      StripSizeMemOp->Mem.Size = Size;
       unsigned LastOpcode = Inst.getOpcode();
       unsigned M =
           MatchInstructionImpl(Operands, Inst, ErrorInfoIgnore,
                                MatchingInlineAsm, isParsingIntelSyntax());
       if (Match.empty() || LastOpcode != Inst.getOpcode())
         Match.push_back(M);
+      // In case this is relaxation, we're only looking for the first match
+      if (DoRelaxation && M > 0)
+        break;
 
       // If this returned as a missing feature failure, remember that.
       if (Match.back() == Match_MissingFeature)
         ErrorInfoMissingFeature = ErrorInfoIgnore;
     }
 
-    // Restore the size of the unsized memory operand if we modified it.
-    if (UnsizedMemOp)
-      UnsizedMemOp->Mem.Size = 0;
+    // Restore the size of the relaxed/unsized memory 
+    // operand if we modified it.
+    if (StripSizeMemOp)
+      StripSizeMemOp->Mem.Size = OrigSize;
   }
 
   // If we haven't matched anything yet, this is not a basic integer or FPU
   // operation.  There shouldn't be any ambiguity in our mnemonic table, so try
   // matching with the unsized operand.
   if (Match.empty()) {
     Match.push_back(MatchInstructionImpl(Operands, Inst, ErrorInfo,
                                          MatchingInlineAsm,

test/CodeGen/not-Relaxation.c

+// RUN:%clang_cc1 -triple x86_64-pc-windows-msvc -fasm-blocks -fms-compatibility -mllvm --x86-asm-syntax=intel -S -o - %s \
+// RUN:| FileCheck %s -check-prefix FMSCOMP
+// RUN:not %clang_cc1 -triple x86_64-pc-windows-msvc -fasm-blocks -mllvm --x86-asm-syntax=intel -S -o /dev/null %s 2>&1 %s \
+// RUN | FileCheck %s -check-prefix REGULARCOMP
+
+char test_mem[16];
+
+void func(void)
+{
+  __asm {
+    //FMSCOMP:not cl
+    //REGULARCOMP:not cl    
+    not cl
+    //FMSCOMP:not ax 
+  //REGULARCOMP:not ax 
+    not ax
+    //FMSCOMP:not eax
+  //REGULARCOMP:not eax 
+    not eax
+    //FMSCOMP:not rax
+  //REGULARCOMP:not rax 
+    not rax
+    //FMSCOMP:not qword ptr [rip + test_mem] 
+  //REGULARCOMP:not qword ptr [rip + test_mem] 
+    not QWORD PTR test_mem
+    //FMSCOMP:not dword ptr [rip + test_mem] 
+  //REGULARCOMP:not dword ptr [rip + test_mem] 
+    not DWORD PTR test_mem
+    //FMSCOMP:not word ptr [rip + test_mem] 
+  //REGULARCOMP:not word ptr [rip + test_mem]   
+    not WORD PTR test_mem
+    //FMSCOMP:not byte ptr [rip + test_mem] 
+  //REGULARCOMP:not byte ptr [rip + test_mem] 
+    not BYTE PTR test_mem
+  }
+}
+void Zf_NOT(void)
+{
+  __asm {
+    //FMSCOMP:not     byte ptr [test_mem]
+  //REGULARCOMP: error: invalid operand for instruction
+    not XMMWORD PTR test_mem
+    //FMSCOMP:not     byte ptr [test_mem]
+  //REGULARCOMP: error: invalid operand for instruction
+    not TBYTE PTR test_mem
+ }
+}