[PATCH] ms-inline-asm: Add facilities for rewriting labels in X86AsmParser

[Reid Kleckner]

lgtm

================
Comment at: lib/MC/MCParser/AsmParser.cpp:4620
@@ +4619,3 @@
+    case AOK_Identifier:
+      OS << AR.Identifier;
+      break;
----------------
ehsan wrote:
> rnk wrote:
> > This should really be making an local label that doesn't make it into the object file.  MC should know what the local label prefix is.  It's usually 'L', I think.
> > 
> > If you add the prefix here, then maybe this should be AOK_Label or something.
> What do you mean by a local label?  I don't think that something that doesn't make it into the object file is what we want here.  For example, MSVC creates a relocation to $label$1 in this test case:
> 
> void f() {
>   __asm {
>     label:
>     mov eax, label
>   }
> }
Hm, true. I think MC will make a relocation for this as well even if you use the private prefix.

================
Comment at: lib/Target/X86/AsmParser/X86AsmParser.cpp:1305
@@ +1304,3 @@
+
+  // If the identifier lookup was unsuccessful, assume that we are dealing with
+  // a label.
----------------
ehsan wrote:
> rnk wrote:
> > What does MSVC do in this case?  I assume it uses the opposite logic for this code:
> > 
> >   void foo();
> >   void __declspec(naked) bar() {
> >     __asm {
> >     foo:
> >       jmp foo ; probably infinite loops instead of tail calling bar.
> >     }
> >   }
> > 
> > I guess this is the hard case:
> > 
> >   void foo();
> >   void __declspec(naked) bar() {
> >     __asm {
> >       test ecx, ecx
> >       xor eax, eax
> >       jz foo
> >       mov eax 1
> >   foo:
> >      ret
> >   }
> The first case is turned into:
> 
> _bar:
>        0:       e9 00 00 00 00                                  jmp     0
>                         00000001:  IMAGE_REL_I386_REL32 _foo
> 
> The second case is turned into:
> 
> _bar:
>        0:       85 c9                                           testl   %ecx, %ecx
>        2:       33 c0                                           xorl    %eax, %eax
>        4:       0f 84 00 00 00 00                               je      0
>                         00000006:  IMAGE_REL_I386_REL32 _foo
>        a:       b8 01 00 00 00                                  movl    $1, %eax
>        f:       c3                                              retl
> 
> My patch compiles the first case into:
> 
> _bar:
>        0:	b8 00 00 00 00                               	movl	$0, %eax
> 			       1: IMAGE_REL_I386_DIR32	_foo
> 
> __MSASMLABEL_.0__:
>        5:	ff e0                                        	jmpl	*%eax
>        7:	c3                                           	ret
> 
> And the second case into:
> 
> _bar:
>        0:	85 c9                                        	testl	%ecx, %ecx
>        2:	31 c0                                        	xorl	%eax, %eax
>        4:	0f 84 00 00 00 00                            	je	0
> 			       6: IMAGE_REL_I386_REL32	foo
>        a:	b8 01 00 00 00                               	movl	$1, %eax
> 
> __MSASMLABEL_.0__:
>        f:	c3                                           	ret
>       10:	c3                                           	ret
> 
> Is that good (enough)?
Huh, so they pick global functions before local labels, and so do we.  Seems fine then.

http://reviews.llvm.org/D4587