Closed Bug 106864 Opened 23 years ago Closed 20 years ago

Strange string behavior on ARM Linux systems

Categories

(Core :: JavaScript Engine, defect)

Other
Linux
defect
Not set
normal

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+
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.
Formally confirming bug for consideration - 
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is a beaut.
Status: NEW → ASSIGNED
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.
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. :|
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.
TestXPTCInvoke just tests the invoke part. The stubs part looks to likely not 
match what the compiler is doing.
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.
Ran the xpcshell test in gdb5

New attachment is the backtrace.  And it just gets weirder from here...
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.
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.
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.

How many arm cpp predefines does it take...?

Is __arm__ defined now for all ARM targets, by any chance?

/be
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.
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
wtc: is there any way NSPR could provide us with a single define that would 
mean ARM?
timeless: no, sorry.  You can do this in mozilla/configure.in though.
how many arms does it take to screw in a light bulb?

dauphin: wanna help configure arms reduction?
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
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.

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.
Attached file xptcstubs_arm.coo (obsolete) —
New xptcstubs_arm.cpp with proper contributors
This is the patch, described in comment #16.
It is needed in order to get mozilla working on arm-linux.
This patch allows new architectures like armv5l.
It also allows xpcom to work with gcc-3.0.4. on arm.
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?
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
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.
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?
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
Attached patch Patch to fix various ARM defines (obsolete) — Splinter Review
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.
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])
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
The problem with the jar files is probably bug 87965.
Attachment #72386 - Attachment mime type: application/octet-stream → text/plain
Attached file Blank Attachment (obsolete) —
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
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])
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
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 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 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+
Checked in on the 0.9.9 branch, I'll hit the trunk tomorrow morning when things
are (hopefully) green.
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.

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.
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)
No problem with that !

The only differences are in the comments ;)
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.
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.
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
Blocks: 322806
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: