1028613 - Modify the x86 MSVC implementation of NS_InvokeByIndex to make it compatible with clang-cl

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

clang-cl currently has a couple of bugs that hit us in the inline assembly in this bug, and it seems harmless to rewrite what we have here so that clang-cl can understand it.

The relevant bugs are:
http://llvm.org/bugs/show_bug.cgi?id=17201
http://llvm.org/bugs/show_bug.cgi?id=20091

Comment 1

[(no longer active)]

Attachment: Modify the x86 MSVC implementation of NS_InvokeByIndex to make it compatible with clang-cl

Comment 2

[Benjamin Smedberg]

Comment on attachment 8444023 [details] [diff] [review]
Modify the x86 MSVC implementation of NS_InvokeByIndex to make it compatible with clang-cl

I'm a little surprised by this.

Because this is not a naked function, MSVC emits both the prologue and epilog for this function which includes the pop/ret. This early return bypasses that (harmless, but weird).

What does clang-cl do differently here?

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Comment on attachment 8444023 [details] [diff] [review]
> Modify the x86 MSVC implementation of NS_InvokeByIndex to make it compatible
> with clang-cl
> 
> I'm a little surprised by this.
> 
> Because this is not a naked function, MSVC emits both the prologue and
> epilog for this function which includes the pop/ret. This early return
> bypasses that (harmless, but weird).
> 
> What does clang-cl do differently here?

clang-cl transforms the end of the function into unreachable because it doesn't realize the implicit clobbering of eax and return.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8444023 [details] [diff] [review]
Modify the x86 MSVC implementation of NS_InvokeByIndex to make it compatible with clang-cl

Please add a comment before the final pop/ret explaining why it's needed and linking to this and http://llvm.org/bugs/show_bug.cgi?id=17201

Alternately, put the final pop/ret in another __asm__ block #ifdef __clang__

Comment 5

[(no longer active)]

I added a comment:

https://hg.mozilla.org/integration/mozilla-inbound/rev/240408f52150

Comment 6

[Stephen Crane]

Doesn't this also need:

add esp, 0x10
pop esi

in the epilogue? clang-cl seems to emit a register save and allocates stack space which needs to be cleaned up before returning, or the stack will be misaligned... Here's the disassembled object after applying your patch on my system:

_NS_InvokeByIndex:
  00000000: 55                 push        ebp
  00000001: 89 E5              mov         ebp,esp
  00000003: 56                 push        esi
  00000004: 83 EC 10           sub         esp,10h
  00000007: 8B 45 08           mov         eax,dword ptr [ebp+8]
  0000000A: 8B 4D 0C           mov         ecx,dword ptr [ebp+0Ch]
  0000000D: 8B 55 10           mov         edx,dword ptr [ebp+10h]
  00000010: 8B 75 14           mov         esi,dword ptr [ebp+14h]
  00000013: 89 75 F8           mov         dword ptr [ebp-8],esi
  00000016: 89 55 F4           mov         dword ptr [ebp-0Ch],edx
  00000019: 89 4D F0           mov         dword ptr [ebp-10h],ecx
  0000001C: 89 45 EC           mov         dword ptr [ebp-14h],eax
  0000001F: BE 00 00 00 00     mov         esi,offset ?invoke_copy_to_stack@@YIXPAIIPAUnsXPTCVariant@@@Z
  00000024: 8B 55 F4           mov         edx,dword ptr [ebp-0Ch]
  00000027: 85 D2              test        edx,edx
  00000029: 74 0E              je          noparams
  0000002B: 89 D0              mov         eax,edx
  0000002D: C1 E0 03           shl         eax,3
  00000030: 29 C4              sub         esp,eax
  00000032: 89 E1              mov         ecx,esp
  00000034: FF 75 F8           push        dword ptr [ebp-8]
  00000037: FF D6              call        esi
noparams:
  00000039: 8B 4D EC           mov         ecx,dword ptr [ebp-14h]
  0000003C: 51                 push        ecx
  0000003D: 8B 11              mov         edx,dword ptr [ecx]
  0000003F: 8B 45 F0           mov         eax,dword ptr [ebp-10h]
  00000042: FF 14 82           call        dword ptr [edx+eax*4]
  00000045: 5D                 pop         ebp
  00000046: C3                 ret


As an alternative, what about creating a temporary return variable, and assign eax to that? I'm not sure how performance critical this code is.

Comment 7

[Stephen Crane]

Attachment: Alternative patch to work around clang-cl's lack of inline assembly return in NS_InvokeByIndex

This is an alternative workaround which should be more stable in the face of possibly variable function prologue/epilogue, but might result in worse assembly.

Comment 8

[Stephen Crane]

Nevermind, just realized this won't work at all. The inlined asm doesn't actually reset the stack correctly without using the base pointer. The original patch works great.

What about making this entire function __declspec(naked) and providing a prologue? Clang emits somewhat bad code here because it creates unnecessary stack slots.

Comment 9

[Benjamin Smedberg]

Might as well make the entire function in assembly. AIUI naked doesn't work in clang-cl.

Comment 10

[Stephen Crane]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Might as well make the entire function in assembly. AIUI naked doesn't work
> in clang-cl.

Yeah, I just tried this out and it doesn't really work. Clang correctly did not emit a prologue, but still assumed there was stack space to save regs (which it didn't really need to do...).

Comment 11

[[e-mail]]

This make the -GL build crash. vs2013u2 with -GL. Same as bug950323，but adding -GL- to XPCWrappedNative.cpp won't make the crash gone.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/240408f52150

Comment 13

[(no longer active)]

(In reply to sjcrane from comment #10)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> > Might as well make the entire function in assembly. AIUI naked doesn't work
> > in clang-cl.
> 
> Yeah, I just tried this out and it doesn't really work. Clang correctly did
> not emit a prologue, but still assumed there was stack space to save regs
> (which it didn't really need to do...).

Yeah, clang's support for naked functions is pretty broken...  I chatted with Stephen about this on IRC yesterday and he confirmed that he thinks the patch landed here is fine.  I think going forward we really want to just implement this all in assembly.

(In reply to zhoubcfan from comment #11)
> This make the -GL build crash. vs2013u2 with -GL. Same as bug950323，but
> adding -GL- to XPCWrappedNative.cpp won't make the crash gone.

Do you mind please filing another bug about that and attach the generated code for this function in those builds?  I'd be happy to take a look at that.  Thanks!