560769 - arm xptinvoke is broken on debug builds depending on compiler and flags

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Dave Townsend [:mossop]]

Since bug 532198 landed builds for the Palm Pre have been broken. They get partway through startup then fail as soon as the first JS component attempts to get registered with a segmentation fault with the following stack. It looks like the aFile passed to frame 2 is incorrect.

#0  0x370c3f84 in nsLocalFile::Contains (this=0x54110, inFile=0x377a9ad4, recur=1, 
    _retval=0x9ed10fe0) at /Users/dave/mozilla/source/palm/xpcom/io/nsLocalFileUnix.cpp:1520
#1  0x370d3328 in nsComponentManagerImpl::RegistryLocationForFile (this=0x48df0, aFile=0x377a9ad4, 
    aRegistryName=...)
    at /Users/dave/mozilla/source/palm/xpcom/components/nsComponentManager.cpp:2331
#2  0x370d3688 in nsComponentManagerImpl::RegisterFactoryLocation (this=0x48df0, aClass=..., 
    aClassName=0x2ed3b8 "INIProcessorFactory", 
    aContractID=0x2ed3d0 "@mozilla.org/xpcom/ini-processor-factory;1", aFile=0x377a9ad4, 
    loaderStr=0x0, aType=0xb8288 "\220`z\244az7\n")
    at /Users/dave/mozilla/source/palm/xpcom/components/nsComponentManager.cpp:3497
#3  0x37110fc4 in NS_InvokeByIndex_P (that=0x48df8, methodIndex=7, paramCount=6, params=0x9ed11418)
    at /Users/dave/mozilla/source/palm/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:176
#4  0x358d3c40 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
    at /Users/dave/mozilla/source/palm/js/src/xpconnect/src/xpcwrappednative.cpp:2750
#5  0x358dfea0 in XPC_WN_CallMethod (cx=0xc80f8, obj=0x39809980, argc=6, argv=0x2b41f0, 
    vp=0x9ed11750)
    at /Users/dave/mozilla/source/palm/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1770
#6  0x37c8d720 in js_Invoke (cx=0xc80f8, argc=6, vp=0x2b41e8, flags=2)
    at /Users/dave/mozilla/source/palm/js/src/jsinterp.cpp:834
#7  0x37c755d8 in js_Interpret (cx=0xc80f8)
    at /Users/dave/mozilla/source/palm/js/src/jsops.cpp:2251
#8  0x37c8d780 in js_Invoke (cx=0xc80f8, argc=4, vp=0x2b41c0, flags=0)
    at /Users/dave/mozilla/source/palm/js/src/jsinterp.cpp:842
#9  0x358c2338 in nsXPCWrappedJSClass::CallMethod (this=0x2c0d10, wrapper=0x2ecf20, methodIndex=4, 
    info=0x132c98, nativeParams=0x9ed123d8)
    at /Users/dave/mozilla/source/palm/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1696
#10 0x358b4cc8 in nsXPCWrappedJS::CallMethod (this=0x2ecf20, methodIndex=4, info=0x132c98, 
    params=0x9ed123d8) at /Users/dave/mozilla/source/palm/js/src/xpconnect/src/xpcwrappedjs.cpp:570
#11 0x37111b44 in PrepareAndDispatch (self=0x2ecf20, methodIndex=<value optimized out>, 
    args=<value optimized out>)
    at /Users/dave/mozilla/source/palm/xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:132
#12 0x3711104c in SharedStub ()
#13 0x370d40f0 in nsComponentManagerImpl::AutoRegisterComponent (this=0x48df0, 
    aComponentFile=0xb8288, aDeferred=..., minLoader=0)
    at /Users/dave/mozilla/source/palm/xpcom/components/nsComponentManager.cpp:3129
#14 0x370d484c in nsComponentManagerImpl::AutoRegisterComponentsList (this=0x48df0, inDir=0x54110, 
    fd=0x64db0, aLeftovers=..., aDeferred=...)
    at /Users/dave/mozilla/source/palm/xpcom/components/nsComponentManager.cpp:3022
#15 0x370d4a94 in nsComponentManagerImpl::AutoRegisterDirectory (this=0x48df0, inDirSpec=0x54110, 
    aLeftovers=..., aDeferred=...)
    at /Users/dave/mozilla/source/palm/xpcom/components/nsComponentManager.cpp:2944
#16 0x370d4f00 in nsComponentManagerImpl::AutoRegisterImpl (this=0x48df0, inDirSpec=0x54110, 
    aLeftovers=..., aDeferred=...)
    at /Users/dave/mozilla/source/palm/xpcom/components/nsComponentManager.cpp:2912

Comment 1

[Mike Hommey [:glandium]]

What is the toolchain on palm pre ?

Comment 2

[Mike Hommey [:glandium]]

Could you generate xptcinvoke_arm.s and attach it here ?

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #1)
> What is the toolchain on palm pre ?

arm-none-linux-gnueabi-gcc (GCC) 4.3.3
Copyright (C) 2008 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I know on Windows they just use the Codesourcery builds, I imagine the same is true for OSX where I am building too.

Comment 4

[Dave Townsend [:mossop]]

Attachment: xptcinvoke_arm.s

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 440501 [details]
xptcinvoke_arm.s

Actually that was with your patch backed out, let me regenerate it from current trunk.

Comment 6

[Mike Hommey [:glandium]]

Your .s isn't built from the xptcinvoke_arm.cpp as modified by bug 532198, as can be seen from the line numbers in the debug information (the new file is 175 lines long, while your .s goes up to line 245), and as can be seen by the presence of the invoke_count_words function, which was removed.

Comment 7

[Mike Hommey [:glandium]]

Oh I hadn't seen comment 5.

Comment 8

[Dave Townsend [:mossop]]

Attachment: xptcinvoke_arm.s

Sorry for hte confusion, here is the one from your code.

Comment 9

[Mike Hommey [:glandium]]

From the stack trace, it looked like the 4 first arguments (which are passed by registers) are okay, and the first one to get a wrong value is on stack. I would have thought the stack pointer somehow was being messed with, but from a quick glance at the assembly, it doesn't look like so.

Is that the very first NS_InvokeByIndex_P call or are some calls not failing before that ? Did you try a -O0 build of the xptcinvoke file ?

Comment 10

[Mike Hommey [:glandium]]

If it also breaks with -O0 (which is likely), I would be interested to have the corresponding assembly.

Comment 11

[Mike Hommey [:glandium]]

I found out what is wrong in the assembly and makes it fail, but i haven't figured why it is generated that way yet.

Comment 12

[Dave Townsend [:mossop]]

Attachment: xptcinvoke_arm.s with -O0

It still fails at the same point when compiled with -O0. It isn't the first call to NS_InvokeByIndex_P however all of the previous calls that appeared to work had paramCount=1.

Comment 13

[Mike Hommey [:glandium]]

(In reply to comment #12)
> Created an attachment (id=440547) [details]
> xptcinvoke_arm.s with -O0

This doesn't look like -O0, all the functions are inlined.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 440547 [details]
xptcinvoke_arm.s with -O0

Sorry, didn't spot that xptcall overrides the optimisation, I'll rebuild it

Comment 15

[Dave Townsend [:mossop]]

Attachment: xptcinvoke_arm.s with -O0

This should be the right file, and this one actually works on the device, no crashes

Comment 16

[Mike Hommey [:glandium]]

Can you also try a non debug build (optimized, this time) ? I think I know what is happening, but I still need to verify my theory.

Comment 17

[Mike Hommey [:glandium]]

I could verify my theory. The problem lies in the NS_ASSERTION, which ends up calling NS_DebugBreak_P, which takes 5 arguments. That means to call this function, the stack is being used. Considering this, gcc does some dirty tricks in the generated assembly and always allocates this stack, which means the stack space we allocate is not where the stack pointer points anymore.

An quick and dirty way to fix this would be to set invoke_copy_to_stack static when #ifndef DEBUG only, to avoid its inlining and the gcc dirty tricks. This is also the simplest. Would that sound okay for you ? If so, I'll attach a patch with this change, with a comment explaining on what trick we rely that require some care in what kind of functions are called from this code.

Comment 18

[Mike Hommey [:glandium]]

FWIW, I could generate a broken build by changing some flags, while plain debug flags and non-debug flags both work properly with my toolchain. (basically, adding the DEBUG defines on a normal build, while the debug build also adds "-arith")

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #17)
> I could verify my theory. The problem lies in the NS_ASSERTION, which ends up
> calling NS_DebugBreak_P, which takes 5 arguments. That means to call this
> function, the stack is being used. Considering this, gcc does some dirty tricks
> in the generated assembly and always allocates this stack, which means the
> stack space we allocate is not where the stack pointer points anymore.

Yeah, a release build works fine.

> An quick and dirty way to fix this would be to set invoke_copy_to_stack static
> when #ifndef DEBUG only, to avoid its inlining and the gcc dirty tricks. This
> is also the simplest. Would that sound okay for you ? If so, I'll attach a
> patch with this change, with a comment explaining on what trick we rely that
> require some care in what kind of functions are called from this code.

bsmedberg or someone really needs to decide that

Comment 20

[Brian Crowder]

I'm hitting this same crash in a debug build on my N900, fwiw.

Comment 21

[Mike Hommey [:glandium]]

Attachment: Patch

bsmedberg agreed with this approach on irc. Dave, can you try this patch ?

Comment 22

[Dave Townsend [:mossop]]

(In reply to comment #21)
> Created an attachment (id=440589) [details]
> Patch
> 
> bsmedberg agreed with this approach on irc. Dave, can you try this patch ?

Seems to work for me.

Comment 23

[Brian Crowder]

Yeah, making this non-static in debug builds fixes the issue for me, as well.

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 440589 [details] [diff] [review]
Patch

I'd be interested in native speaker review of the comments, too, because they might not be as readable as they should.

Comment 25

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/7e9e156fe64f

Comment 26

[Zack Weinberg (:zwol)]

__attribute__((noinline)) would maybe be more appropriate than disabling the 'static', here?  If we care about MSVC on ARM, the equivalent is __declspec(noinline), and it shouldn't be hard to come up with appropriate #ifdeffage, although I dunno where it should live (is nscore.h available to xpconnect code?)

Comment 27

[Mike Hommey [:glandium]]

(In reply to comment #26)
> __attribute__((noinline)) would maybe be more appropriate than disabling the
> 'static', here?

The result is similar, and doesn't rely on a compiler specific property.

>  If we care about MSVC on ARM, the equivalent is
> __declspec(noinline), and it shouldn't be hard to come up with appropriate
> #ifdeffage, although I dunno where it should live (is nscore.h available to
> xpconnect code?)

Note I doubt MSVC on ARM uses xptcinvoke from the md/unix directory.

Comment 28

[Zack Weinberg (:zwol)]

(In reply to comment #27)
> (In reply to comment #26)
> > __attribute__((noinline)) would maybe be more appropriate than disabling the
> > 'static', here?
> 
> The result is similar, and doesn't rely on a compiler specific property.

It's not guaranteed to do what you want, though.  We are seriously looking at whole-program optimization (see e.g. http://blog.mozilla.com/tglek/2010/04/12/squeezing-every-last-bit-of-performance-out-of-the-linux-toolchain/ ) and in whole-program mode the compiler is going to notice that this function is only called from one place and inline it regardless of the absence of 'static'.

I suppose since this is only a problem in DEBUG mode, we don't have to worry about the compiler being *that* aggressive anyway, but I'd still feel more comfortable with an annotation that actually *meant* "don't inline this" rather than just having that side-effect.

Comment 29

[Mike Hommey [:glandium]]

I really doubt this kind of optimization really results in inlining. At most I think it will avoid to go through the PLT for function calls because of the hidden visibility.

Comment 30

[Mike Hommey [:glandium]]

But feel free to put a noinline instruction if you want to.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 440589 [details] [diff] [review]
Patch

This patch blocks a patch in bug 532198 that I would like to see on m-192

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 440589 [details] [diff] [review]
Patch

dropping for fennec 1.1

Comment 33

[(no longer active)]

The changeset for bug 559592 accidentally referenced this bug in its changeset message. Apologies -- anyone who ends up here from clicking the bug link in that changeset should instead go to bug 559592.