Page MenuHomePhabricator

ms-inline-asm: Add facilities for rewriting labels in X86AsmParser
ClosedPublic

Authored by ehsan on Jul 18 2014, 12:15 PM.

Details

Summary

This will be used in order to make it possible to rewrite asm labels
to use internal names, so that we can scope the label names to
functions as opposed to LLVM inline asm blocks.

ms-inline-asm: Add a sema callback for looking up label names

The implementation of the callback in clang's Sema will return an
internal name for labels.

Diff Detail

Repository
rL LLVM

Event Timeline

ehsan updated this revision to Diff 11658.Jul 18 2014, 12:15 PM
ehsan retitled this revision from to ms-inline-asm: Add facilities for rewriting labels in X86AsmParser.
ehsan updated this object.
ehsan edited the test plan for this revision. (Show Details)
ehsan added a reviewer: rnk.
ehsan added a subscriber: Unknown Object (MLST).
rnk added inline comments.Jul 18 2014, 12:39 PM
include/llvm/MC/MCParser/MCAsmParser.h
57 ↗(On Diff #11658)

It's generally nicer to avoid having API breakage between LLVM in clang if it can be avoided. In this case, I'd give this a no-op implementation that clang overrides instead of a pure virtual method which will break the clang build for a few revisions when this lands.

lib/MC/MCParser/AsmParser.cpp
4620 ↗(On Diff #11658)

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.

lib/Target/X86/AsmParser/X86AsmParser.cpp
1305 ↗(On Diff #11658)

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
}

I'll address the rest of the comments in the next iteration of this and the LLVM side patch.

lib/MC/MCParser/AsmParser.cpp
4620 ↗(On Diff #11658)

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
}

}

lib/Target/X86/AsmParser/X86AsmParser.cpp
1305 ↗(On Diff #11658)

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)?

ehsan updated this revision to Diff 11686.Jul 19 2014, 2:13 PM

Addressed the review comments.

ehsan updated this revision to Diff 13792.Sep 17 2014, 11:15 AM

Updated the patch to use the LabelDecl machinery on the clang side.

Can you please review this too? Thanks!

rnk accepted this revision.Sep 18 2014, 9:07 AM
rnk edited edge metadata.

lgtm

lib/MC/MCParser/AsmParser.cpp
4620 ↗(On Diff #11658)

Hm, true. I think MC will make a relocation for this as well even if you use the private prefix.

lib/Target/X86/AsmParser/X86AsmParser.cpp
1305 ↗(On Diff #11658)

Huh, so they pick global functions before local labels, and so do we. Seems fine then.

This revision is now accepted and ready to land.Sep 18 2014, 9:07 AM
ehsan closed this revision.Sep 21 2014, 7:31 PM
ehsan updated this revision to Diff 13916.

Closed by commit rL218229 (authored by @ehsan).