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

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, thanks!



================
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:
> 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.
I see. Well, it sounds like it was a useful test. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71436