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

[Eric Astor via Phabricator via llvm-commits]

epastor added inline comments.


================
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)) {
----------------
epastor wrote:
> 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.
Fixed; we now get the same errors in this scenario as we do in the current LLVM trunk, reporting "expected identifier" and "use of undeclared label '3'".

On the other hand, I'm still working on finding a scenario that DOES trigger this corner case.


================
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");
+  }
----------------
epastor wrote:
> 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.
Turns out Sema wasn't successfully recognizing inline-assembly references to enum values, so we were processing them through as labels, and only recognizing the problem late in processing. Fixed now, with accurate error reporting; test added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436