Closed
Bug 106864
Opened 23 years ago
Closed 20 years ago
Strange string behavior on ARM Linux systems
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: crichton, Assigned: shaver)
References
Details
Attachments
(9 files, 6 obsolete files)
87.27 KB,
application/octet-stream
|
Details | |
1.21 KB,
text/plain
|
Details | |
3.24 KB,
text/plain
|
Details | |
6.33 KB,
text/plain
|
Details | |
1.90 KB,
text/plain
|
Details | |
8.55 KB,
text/plain
|
Details | |
6.30 KB,
text/plain
|
Details | |
2.95 KB,
patch
|
brendan
:
review+
shaver
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
9.25 KB,
patch
|
Details | Diff | Splinter Review |
When starting up a clean build of the mozilla CVS tree on Debian ARM potato, I see a massive problem with registering JS components (see attached log). Trapping the strings set to libreg and printing them out, I see some corruption in the certain strings. TestXPTCInvoke passes, but mozilla bombs. You will probably need more info, let me know how/what you need.
Reporter | ||
Comment 1•23 years ago
|
||
Comment 2•23 years ago
|
||
Formally confirming bug for consideration -
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•23 years ago
|
||
Am I dumb? I don't know what I'm supposed to make of that massive dump. How about running: xpcshell mozilla/js/src/xpconnect/tests/js/old/xpctest_echo.js The 'echo' tests include calling in both directions and do data conersions of all primative types. It is the best test for the xptcall stubs code and for data conversion (short of running the browser). You can remove the .JS files from the components directory if they are giving you problems when trying to run other tests.
Reporter | ||
Comment 5•23 years ago
|
||
Well. I added a few functions to the nr_IsValidUTF8 function in libreg to track the strings being thrown around. As one can see (grep for ****), every time one of the JS components is registered, some bogus string is passed to ns_IsValidUTF8. I've also run TextXPC, thinking that the compiler behavior has changed for ARM targets, but the tests produce the same results as my x86... Also, even removing the JS components, nuking component.reg, and restarting mozilla doesn't lead to a moz that starts (it simply segfaults). And *another* thing, I get a *lot* of: WARNING: Trying old-style manifest.rdf. Please update to contents.rdf., file nsChromeRegistry.cpp, line 2035 *** Chrome Registration of package: Checking for contents.rdf at jar:resource:/chrome/toolkit.jar!/content/global/ type errors on startup as well. Dunno of this a build bug, or part of the problem I'm seeing. All in all, a lot of work left to be done to get this bad boy working on ARM machines. :|
Reporter | ||
Comment 6•23 years ago
|
||
Reporter | ||
Comment 7•23 years ago
|
||
Attachment is a backtrace from gdb5 on the armv4l box. I have no idea if it is related to this bug, but every time I start moz on the ARM box, mozilla segfaults here. What's odd is the pointer. Either the assembly code is horked (but TestXPTCInvoke works...), or something else is amiss. Throwing around a pointer to 0x75 isn't good.
Comment 8•23 years ago
|
||
TestXPTCInvoke just tests the invoke part. The stubs part looks to likely not match what the compiler is doing.
Comment 9•23 years ago
|
||
Then again, maybe xpcom/proxy is munging your strings somehow. Please someone try the test I suggested using xpcshell. This might help to discover if this is a problem in xptcall or not. I'd think that watching this live in a debugger would clear up a lot also.
Reporter | ||
Comment 10•23 years ago
|
||
Ran the xpcshell test in gdb5 New attachment is the backtrace. And it just gets weirder from here...
Reporter | ||
Comment 11•23 years ago
|
||
Reporter | ||
Comment 12•23 years ago
|
||
Reporter | ||
Comment 13•23 years ago
|
||
Well, more on this bug, in an effort to try to jog some memories. I got a new copy of the xptcstubs_arm.cpp from Russel King (the Linux-ARM maintainer). Now, instead of a pure segfault, I get an assertion from the code. ###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../dist/include/xpcom/nsCOMPtr.h, line 650 ###!!! Break: at file ../../dist/include/xpcom/nsCOMPtr.h, line 650 New copy of the xptcstubs_arm.cpp submitted.
Reporter | ||
Comment 14•23 years ago
|
||
Interesting tidbit. The assertion does NOT occur if component.reg exists/up to date. If component.reg is "correct", there is just a segfault. Backtrace is attached.
Reporter | ||
Comment 15•23 years ago
|
||
Reporter | ||
Comment 16•23 years ago
|
||
I think I have the problem (kinda) nailed. Thanks to Russel King, I think the last of the problem is dead. In js/src/jsnum.h, there is the following line: #if defined(__arm) || defined(__arm32__) || defined(_arm26__) change it to #if defined(__arm) || defined(__arm32__) || defined(_arm26__) || defined(__arm__) and it appears to work (newer GCCs seem to define __arm__ for certain ARM targets). Still have yet to test a full mozilla build. I'm currently going through looking for any other instances of __arm, though.
Comment 17•23 years ago
|
||
How many arm cpp predefines does it take...? Is __arm__ defined now for all ARM targets, by any chance? /be
Reporter | ||
Comment 18•23 years ago
|
||
I wish. __arm__ is Linux, __arm26__ and __arm32__ come from NetBSD (I think, since NetBSD can run on the 26 bit version), and __arm is from hell-knows-where. It might be from RiscOS or something.
Reporter | ||
Comment 19•23 years ago
|
||
With the new xptcstubs code, it appears to work. However, JAR files won't work, and there are still some makefile issues. But I think *this* bug is dead: http://www-unix.ecs.umass.edu/~mcrichto/arm-moz.png
Comment 20•23 years ago
|
||
wtc: is there any way NSPR could provide us with a single define that would mean ARM?
Comment 21•23 years ago
|
||
timeless: no, sorry. You can do this in mozilla/configure.in though.
Comment 22•23 years ago
|
||
how many arms does it take to screw in a light bulb? dauphin: wanna help configure arms reduction?
Comment 23•23 years ago
|
||
JS standalone builds don't use mozilla/configure* foo, alas -- I'm ok with lots of #if || clauses, for now. timeless, Mark, anyone: can you hack up a patch and attach it? I'll help it get reviewed and approved for 0.9.8. /be
Reporter | ||
Comment 24•23 years ago
|
||
I can try to do this, but in the case of JS, I don't think it is necessary. For the *one* ARM arch reference in JavaScript (jsnum.h), the predefines are from the compiler (__arm__, __arm26__, and __arm32__ all come from different GCC flavors). Which also brings up *another* bug in jsnum.h (it's defined as _arm26__ and not __arm26__), but I don't know if that's right. I don't know of anyone who uses a THUMB anymore.
Reporter | ||
Comment 25•23 years ago
|
||
Ok Here is the final version of the xptcstubs_arm.cpp file. Only thing that has changed is that Russell King is now listed as a contributor.
Reporter | ||
Comment 26•23 years ago
|
||
New xptcstubs_arm.cpp with proper contributors
Comment 27•22 years ago
|
||
This is the patch, described in comment #16. It is needed in order to get mozilla working on arm-linux.
Comment 28•22 years ago
|
||
This patch allows new architectures like armv5l. It also allows xpcom to work with gcc-3.0.4. on arm.
Comment 29•22 years ago
|
||
Jeroen Dobbelaere, Please see my patch (submitted by Mark) which cleans up completely the ARM assembly code, and makes it more correct to the APCS standard. This code should work no matter which compiler or ARM architecture you choose to build for. It is attachment ID 66000, and appears in the attachment list above as "xptcstubs_arm.coo". Shaver - if the replacement xptcstubs_arm.coo code isn't in CVS yet, what needs to be done to get it submitted?
Comment 30•22 years ago
|
||
Russel King, the xptcstubs_arm.cpp is also needed ,along the other patches ! Makefile.in needs patching in order to recognize armv5l. xtpcinvoke_arm.cpp needs patching in order to get the correct vtable entry with gcc-3.0.4
Comment 31•22 years ago
|
||
attachment 66000 [details] has problems with gcc-3.0.4, resulting in :
[...]
../../../dist/bin/libxpcom.so: undefined reference to
`nsXPTCStubBase::Stub170()'
../../../dist/bin/libxpcom.so: undefined reference to
`nsXPTCStubBase::Stub115()'
../../../dist/bin/libxpcom.so: undefined reference to
`nsXPTCStubBase::Stub126()'
[..]
This version of the xptcstubs_arm removes those problems with gcc-3.0.4.
It also compiles fine with gcc-2.95.3.
(both times with at least -O, don't know if it works without optimization).
Another drawback is, that, with optimization, it add's two dummy instructions
after each stub.
Assignee | ||
Comment 32•22 years ago
|
||
If I had to pick a patch to land for 0.9.9 and unbust ARM -- I'll expect you to provide builds, Crichton =) -- which one should it be?
Reporter | ||
Comment 33•22 years ago
|
||
As of right now, attachment 71668 [details] [diff] [review] doesn't appear to work well with my gcc-2.95.4 from Emdebian. The old xptcstubs_arm.coo Will work for 2.95.4. GCC 3.0 support can be a simple #if defined() check to change the name mangling (it changed from 2.95 -> 3.0). Better yet, would be a generic way to do it (the 71668 attachment is close, but it seems that the function prolog might be hroking things up..any way to turn it OFF in GCC?). I'm also going to atach a patch that fixes jsnum.h and the xptcall Makefile.in to catch more of the ARM variants. But as of right now, xptcstubs_arm.coo + upcoming patch runs on my Netwinder and passes the xpcshell tests. I'd say review and ship it. GCC 3.0 support can be for 1.0
Reporter | ||
Comment 34•22 years ago
|
||
Hopefully final patch for getting xptcall/JavaScript going on the ARM arch. Also needs the xptcstubs_arm.coo to work. This is now in a reviewable state.
Comment 35•22 years ago
|
||
This version moves some gcc 3 name mangling encoding into the assembler file. It should work with gcc-2.95 and gcc-3.0, with or without optimization. Tested with gcc-3.0.4 and '-O' (obsoletes attachment 71668 [details] [diff] [review] and attachment 66000 [details])
Reporter | ||
Comment 36•22 years ago
|
||
Ok. Another patch. The bonus to this one is that we don't have to depend on a lot of #if defined() preprocessor work. It should also work if GCC changes the name mangling again. Also a tad easier to read. Downside is that it now uses GCC extensions (but that's not a problem). It also emits a few warnings (GCC does the right thing here complaining about a non-void function with no return, and the definition outside the class is also an issue) Tested on the Netwinder. Passes the xpcshell test. Still some bugs w.r.t. ARM floating about. One is bug 9519. The other appears to be a problem with handling JAR files. Compile with --enable-chrome-format=flat and mozilla works. Without it, none of the XUL fines appear to be found. Will submit a proper bug when I do capture enough information.
Attachment #66000 -
Attachment is obsolete: true
Attachment #71668 -
Attachment is obsolete: true
Comment 37•22 years ago
|
||
The problem with the jar files is probably bug 87965.
Reporter | ||
Updated•22 years ago
|
Attachment #72386 -
Attachment mime type: application/octet-stream → text/plain
Reporter | ||
Comment 38•22 years ago
|
||
Ok. This is just a request to attempt to clean up the attachment mess on this bug. I'm now getting slight nudging to get this bug resolved for 0.9.9. Patch 72351 is a definite needs-to-be-there patch. Now for 72386 and 72415, we have 2 different implementations of the same thing. 72386's name mangling looks right, but I'm not a fan of the __asm__("_PrepareAndDispatch") at the beginning of the code. It would be better to declare PrepareAndDispatch as extern "C" and avoid the whole name mangling mess. 72415 does this, but I used some really ugly GCC hacks to do it, namely with how I play with __attribute__ ((naked)). This works on 2.95.4 and 3.0. However, it'll emit warnings like mad. Jeroen, comments?
Attachment #71658 -
Attachment is obsolete: true
Attachment #71659 -
Attachment is obsolete: true
Comment 39•22 years ago
|
||
Attachment 72351 [details] [diff] should indeed be fine. The extern "C" stuff is also fine for me. I first thought of also using the 'naked' attribute, but finally, I didn't go for it because of all the warnings it creates. Then I thought it would be easier to check the compiler version and emit the right names based on the version, until I saw that for g++3 the length of the method must also be taken into account :( This finally resulted in attachment 72386 [details]. Not taking into account the warnings, I would rather take attachment 72415 [details], as the code itself is cleaner. Also don't forget the last part of attachment 71659 [details] [diff] [review] which is also needed for gcc-3 (other stuff in it is now also available in attachment 72351 [details] [diff] [review])
Reporter | ||
Comment 40•22 years ago
|
||
Ok. This attachment includes all the fixes from 72351, and includes the xptcinvoke_arm.cpp changes from 71659. Also, I think that warning avoidance would be better as well, and we should use 72386 for the xptcstubs_arm.cpp rewrite. This is off the assumption the build people would rather have some #if defined() code over 500 (yes, 500) warnings heaved up at compile time. So, unless I've missed something, and if Jeroen is happy with the patch, any chance we can get an r/sr for this?
Attachment #72351 -
Attachment is obsolete: true
Attachment #72452 -
Attachment is obsolete: true
Assignee | ||
Comment 41•22 years ago
|
||
Comment on attachment 72466 [details] [diff] [review] Patch to get moz running on ARM sr=shaver. We'll get this baby in 0.9.9.
Attachment #72466 -
Flags: superreview+
Comment 42•22 years ago
|
||
Comment on attachment 72466 [details] [diff] [review] Patch to get moz running on ARM r=brendan@mozilla.org for the jsnum.h stuff, the rest is port work and requires r= only, so shaver's sr= is more than enough. Some day maybe we can remove the __arm test(?). /be
Attachment #72466 -
Flags: review+
Comment 43•22 years ago
|
||
Comment on attachment 72466 [details] [diff] [review] Patch to get moz running on ARM a=asa (on behalf of drivers) for checkin to the 0.9.9 branch and the 1.0 trunk
Attachment #72466 -
Flags: approval+
Assignee | ||
Comment 44•22 years ago
|
||
Checked in on the 0.9.9 branch, I'll hit the trunk tomorrow morning when things are (hopefully) green.
Comment 45•22 years ago
|
||
After a night of sleep, I have to come back to the 'extern "C"' stuff in
attachment 72415 [details] : The reason I used the 'asm(_PrepareAndBranch)' thing, is that
this is also supported in gcc 2.95.3, and it leaves the function static, not
adding another symbol to the global namespace. This is not the case for
'extern "C"'.
I also agree that we should try to avoid the flood of warnings.
Assignee | ||
Comment 46•22 years ago
|
||
Your timing, good Jeroen, is not optimal. I've landed 72466 and 72386 on the trunk and branch. Unless it's going to cause the death of a major political figure, I don't think I want to churn another patch onto the 0.9.9 branch -- I'd rather devote my energies to zip-struct and other bugs. Take your time (but not too much!) for 1.0, though.
Comment 47•22 years ago
|
||
This patch creates basically attachment 72386 [details]. Differences are : - contributors added - alternative version of the stub from attachment 72415 [details] added as comment. - reason to not use 'extern "C"' is to avoid global namespace bloat. (We are already depending on gcc extensions for arm)
Comment 48•22 years ago
|
||
No problem with that ! The only differences are in the comments ;)
Comment 49•22 years ago
|
||
has 72466 landed yet? if so, or if it is no longer applicable, can you please mark it as obsolete so it doesn't look like an approved change that hasn't landed? thanks.
Comment 50•22 years ago
|
||
Now that 1.0 has come and gone, is there the possibility of landing this patch on the trunk (and/or 1.0 branch)? My personal interest in this bug is that I'm trying to get mozilla to run on the iPaq (an ARM machine). I suppose I should mention that, yes, this patch does work for me. I'm cross-compiling using gcc-2.95.3.
Assignee | ||
Comment 51•20 years ago
|
||
The minimo people have shown, time and again, that we now work on ARM just fine.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•