[PATCH] D71436: [X86][AsmParser] re-introduce 'offset' operator

[Eric Astor via Phabricator via cfe-commits]

epastor added a comment.

Thanks for feedback!



================
Comment at: clang/test/CodeGen/ms-inline-asm-64.c:18
 // CHECK: call void asm sideeffect inteldialect
-// CHECK-SAME: mov [eax], $0
+// CHECK-SAME: mov qword ptr [eax], $0
 // CHECK-SAME: "r,~{dirflag},~{fpsr},~{flags}"(i32* %{{.*}})
----------------
rnk wrote:
> I guess this has the same semantics as it did before.
It does; this previously resolved to a "qword ptr" mov automatically, since $0 unpacked as a (forced) 64-bit register name. Now we need to specify the size of the move, since the operand is an immediate rather than a register.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1821
+        getParser().parsePrimaryExpr(Val, End))
+      return Error(Start, "unexpected token!");
+  } else if (ParseIntelInlineAsmIdentifier(Val, ID, Info, false, End, true)) {
----------------
rnk wrote:
> Please test this corner case, I imagine it looks like:
>   mov eax, offset 3
Interesting. This corner case didn't trigger in that scenario; we get an "expected identifier" error message with good source location, followed by another error "use of undeclared label '3'" in debug builds... and in release builds, we instead get a crash. On tracing the crash, it's a AsmStrRewrite applying to a SMLoc not coming from the same string...

As near as I can tell, the issue is that we end up trying to parse "3" as a not-yet-declared label, as such expand it to `__MSASMLABEL_.${:uid}__3`, and then end up in a bad state because the operand rewrite is applying to the expanded symbol... which isn't in the same AsmString. If you use an actual undeclared label, you hit the same crash in release builds.

This is going to take some work; I'll get back to it in a day or two.


================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:1825
+  } else if (Info.isKind(InlineAsmIdentifierInfo::IK_EnumVal)) {
+    return Error(Start, "offset operator cannot yet handle constants");
+  }
----------------
rnk wrote:
> Please add a test for this corner case, I'm curious to see if the error reporting really works.
This error reporting doesn't work, in fact. We instead get "cannot take the address of an rvalue of type 'int'", with bad source location. Will investigate why we end up there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71436/new/

https://reviews.llvm.org/D71436