Closed
Bug 269538
Opened 20 years ago
Closed 16 years ago
jscpucfg not cross compile friendly
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jordan.crouse, Assigned: benjamin)
References
Details
(Keywords: fixed1.9.1)
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
crowderbt
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041018 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; rv:1.7.3) Gecko/20041018 Firefox/0.10.1
Cross compiling Firefox on a AMD64 machine for a 32 bit target. jscpucfg is
compiled as a build host application. FIrst issue, it seemd to me that
-DCROSS_COMPILE wasn't being passed in to compile jscpucfg, which would also
result in the symptoms listed below. However, I hacked manually hacked
CROSS_COMPILE into the Makefile to ensure that the #ifdef CROSS_COMPILE portions
of jscpucfg.c were being compiled.
Moving on, jscpucfg.c includes prtypes.h which includes MDCPUCFG if MDCPUCFG is
defined. In this case MDCPUCFG is defined as md/_linux.cfg. In md/_linux.cfg,
the settings for each arcitecture are surrounded by the standard platform
defines, such as:
#if defined(__i386_), #else if defined(__x86_64__), etc...
The problem is, since these symbols are set by the compiler configuration, *and*
jscpucfg is always built as a build system host tool, the definitions in
_linux.cfg will be used for the build system, and not the target system, which
is the exact opposite of what was intended. End result is that a binary cross
compiled in this situation will crash while parsing .js files during startup.
Copying in a edited jsautocfg.h file fixes the problem.
Reproducible: Always
Steps to Reproduce:
Updated•20 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•19 years ago
|
Flags: testcase-
This is hurting arm (and all other) cross-compiles as well.
Flags: wanted1.9.1?
Summary: jscpucfg not 64 -> 32 bit cross compile friendly → jscpucfg not cross compile friendly
Comment 3•16 years ago
|
||
This bug is the root cause of the Mac OS X x86<->ppc cross compilation bug 466531,
which prevented me from upgrading the NSPR tag to NSPR_4_7_3_RTM in 1.9.0.5.
The CROSS_COMPILE code in jscpucfg.c (renamed jscpucfg.cpp in mozilla-central) is
broken. It works in some cases only by coincidence. JavaScript needs to use
pre-generated jscpucfg.h files, or redesign jscpucfg.c for cross compilation.
One idea I have is to use a shell script that tries to compile, with the target C compiler
and C flags, a series of C tests containing PR_STATIC_ASSERTs to discover the endianness
and sizes and alignments of various types. For example, we would try to compile
PR_STATIC_ASSERT(sizeof(int) == 2);
If the compilation fails, we would then try to compile
PR_STATIC_ASSERT(sizeof(int) == 4);
etc., until the compilation succeeds.
Comment 4•16 years ago
|
||
OS/Platform -> All/All based on comment 3.
OS: Linux → All
Hardware: PC → All
Assignee | ||
Comment 5•16 years ago
|
||
Yeah, we want that as a series of configure tests with AC_TRY_COMPILE
Assignee | ||
Comment 6•16 years ago
|
||
This patch neuters most of jscpucfg. The only bits that remain are the endian checks and the stack-growth direction. This fixes all of the cross-compile cases that I currently care about.
Assignee: general → benjamin
Attachment #357026 -
Flags: review?(crowder)
Assignee | ||
Updated•16 years ago
|
Attachment #357026 -
Flags: review?(jim)
Updated•16 years ago
|
Attachment #357026 -
Flags: review?(crowder) → review+
Assignee | ||
Comment 7•16 years ago
|
||
This includes one additional change in jslock.cpp since (long*) and (jsword*) are no longer compatible types in C++.
Attachment #357026 -
Attachment is obsolete: true
Attachment #357034 -
Flags: review?(jim)
Attachment #357026 -
Flags: review?(jim)
Comment 8•16 years ago
|
||
Comment on attachment 357034 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.1
>+JS_STATIC_ASSERT(sizeof(jsword) == sizeof(long));
This is not what the old code did. See jstypes.h. For some nutty reason, VS targeting AMD64 (at least) uses LLP64 as its integral type model instead of LP64.
/be
Assignee | ||
Comment 9•16 years ago
|
||
This is in x86-specific code, so I think it's ok.
Comment 10•16 years ago
|
||
Comment on attachment 357034 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.1
Deletions are so pretty. :)
Could we call MOZ_BYTES_PER_TYPE MOZ_SIZE_OF_TYPE, for symmetry, and change the cache variable name to match?
If you're going to delete the long long stuff, do you want to take bug 470791, too?
Could we call the TYPENAME parameters VARIABLE? It isn't a type name, and VARIABLE is what autoconf calls the arguments to AC_DEFINE and AC_SUBST.
Comment 11•16 years ago
|
||
And possibly bug 467453, too.
Assignee | ||
Comment 12•16 years ago
|
||
Not sure what you want with bug 470791. I'm not particularly interested in a global search/replace for those macros, and the rest of it is already included in this patch.
I could take 467453 but the remaining bits (endianness and stack growth) are less important to me, so I'm not likely to get to it soon.
Assignee | ||
Comment 13•16 years ago
|
||
Fixed macro/parameter names as requested.
As for Brendan's note, that hunk is in x86-specific code: x86-64 code would need to use InterlockedCompareExchange64 in any case.
Attachment #357034 -
Attachment is obsolete: true
Attachment #357168 -
Flags: review?(jim)
Attachment #357034 -
Flags: review?(jim)
Comment 14•16 years ago
|
||
Comment on attachment 357168 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.2
Ben, I'd appreciate it if you could also neuter
the endianness macro definitions in jscpucfg.cpp.
Assignee | ||
Comment 15•16 years ago
|
||
I am not going to do it in this bug, because I don't know how to write a configure test for it. Let's leave bug 467453 for that task, because I think it involves hardcoded lists of CPU/OS combinations, like you've got in the NSPR headers.
Comment 16•16 years ago
|
||
Ah, you're right. Endianness tests require actually running
code, so they can't be reimplemented with AC_TRY_COMPILE.
Comment 17•16 years ago
|
||
For what it's worth, the current autoconf does know how to detect endianness without running anything (link provided by ted.mielczarek; danger, excessive cleverness):
http://git.savannah.gnu.org/gitweb/?p=autoconf.git;a=blob;f=lib/autoconf/c.m4;h=beaf0b151396f0dbb4bfadf8099b06a36cc5e608;hb=HEAD#l1287
Go, Ben, go!!! :)
Comment 18•16 years ago
|
||
Comment on attachment 357168 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.2
This is a diff relative to the result of applying first patch, not an alternative patch, right?
Comment 19•16 years ago
|
||
nm, I see now.
Comment 20•16 years ago
|
||
Comment on attachment 357168 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.2
nit: the documentation comment for MOZ_ALIGN_OF_TYPE still refers to "size" when it means "alignment".
In js/src/configure.in: You should be using plain old AC_DEFINE in the text quoted below, not AC_DEFINE_UNQUOTED. (The latter is only needed when you want to cite a shell variable to produce the value of the macro, and it's got corner cases related to m4 and Bourne shell quoting levels to make you itch.)
if test "$moz_cv_size_of_JS_BYTES_PER_WORD" -eq "4"; then
AC_DEFINE_UNQUOTED(JS_BITS_PER_WORD_LOG2, 5)
elif test "$moz_cv_size_of_JS_BYTES_PER_WORD" -eq "8"; then
AC_DEFINE_UNQUOTED(JS_BITS_PER_WORD_LOG2, 6)
There's no longer anything which would #define JS_BYTES_PER_LONG, but you've left a reference to it in jslong.h. It seems like we should just cut out all but the 'LL' case there, since we're assuming we have 'long long'.
Should the patch also actually delete jslong.cpp?
Comment 21•16 years ago
|
||
Comment on attachment 357168 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.2
r=me, with the changes suggested.
Attachment #357168 -
Flags: review?(jim) → review+
Assignee | ||
Comment 22•16 years ago
|
||
Attachment #357168 -
Attachment is obsolete: true
Comment 23•16 years ago
|
||
jslong.cpp and even jslong.h can go away -- separate bug better?
/be
Comment 24•16 years ago
|
||
Comment on attachment 357984 [details] [diff] [review]
Neuter most of jscpucfg, rev. 1.3
I regret that I can no longer block your progress on the basis of this patch.
Attachment #357984 -
Flags: review+
Assignee | ||
Comment 25•16 years ago
|
||
Yeah, that's 470791, not on my short-list (I want to get this in for 1.9.1)
Assignee | ||
Comment 26•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Resolution: --- → FIXED
Assignee | ||
Comment 27•16 years ago
|
||
backed out due to Windows TUnit failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 28•16 years ago
|
||
Attachment #358221 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #358221 -
Flags: review? → review?(crowder)
Updated•16 years ago
|
Attachment #358221 -
Flags: review?(crowder) → review+
Comment 29•16 years ago
|
||
It's clear (not from my memory; too long ago) that (lo) must be unsigned. Is the cast necessary, or would it be sufficient to not cast at all?
Not that any of this matters if we get rid of jslong.h. We are getting rid of it, right?
/be
Comment 30•16 years ago
|
||
Whats the status of this bug? Its kinda holding up our valgrind work. Is there maybe a workaround someone could describe if this is not going to land today?
Assignee | ||
Comment 31•16 years ago
|
||
The following changsets landed on mozilla-central and appear to have stuck green:
http://hg.mozilla.org/mozilla-central/rev/cf34c874a23e (main patch)
http://hg.mozilla.org/mozilla-central/rev/e4a1b76f93d0 (followup)
http://hg.mozilla.org/mozilla-central/rev/406d8b7240df (operator precedence fix for the followup: + binds tighter than <<)
The mozilla-central tree looks pretty green at the moment, so you can probably safely merge it to tracemonkey.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 32•16 years ago
|
||
We pulled it over a little while ago. Thanks.
Comment 33•16 years ago
|
||
cd js/src
mkdir Darwin_DBG.OBJ
../configure --enable-debug --disable-optimize
make
c++ -o jsapi.o -c -I./dist/include/system_wrappers_js -include ../config/gcc_hidden.h -DOSTYPE=\"Darwin9.6.0\" -DOSARCH=Darwin -DEXPORT_JS_API -DJS_USE_SAFE_ARENA -I.. -I. -I./dist/include -I./dist/include/js -I/sdk/include -I.. -fPIC -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -pipe -DDEBUG -D_DEBUG -DDEBUG_gal -DTRACING -g -I/usr/X11/include -DMOZILLA_CLIENT -include ./mozilla-config.h -Wp,-MD,.deps/jsapi.pp ../jsapi.cpp
In file included from ../jsapi.cpp:57:
../jsatom.h:77:3: error: #error "Unsupported configuration"
In file included from ../jsgc.h:48,
from ../jscntxt.h:52,
from ../jstracer.h:47,
from ../jsbuiltins.h:46,
from ../jsapi.cpp:59:
../jsbit.h:229:3: error: #error "NOT SUPPORTED"
../jsatom.h:69: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
../jsatom.h:69: error: declaration of C function ‘void js_static_assert()’ conflicts with
../jsatom.h:68: error: previous declaration ‘void js_static_assert(int*)’ here
../jsatom.h:278: error: declaration of C function ‘void js_static_assert(int*)’ conflicts with
../jsatom.h:69: error: previous declaration ‘void js_static_assert()’ here
../jsapi.cpp: In function ‘void JS_PrintTraceThingInfo(char*, size_t, JSTracer*, void*, uint32, JSBool)’:
../jsapi.cpp:2110: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
../jsapi.cpp: In function ‘JSString* JS_NewExternalString(JSContext*, jschar*, size_t, intN)’:
../jsapi.cpp:2625: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
../jsapi.cpp: In function ‘jschar* JS_GetStringChars(JSString*)’:
../jsapi.cpp:5557: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
../jsapi.cpp: In function ‘size_t JS_GetStringLength(JSString*)’:
../jsapi.cpp:5578: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
../jsapi.cpp: In function ‘JSString* JS_NewGrowableString(JSContext*, jschar*, size_t)’:
../jsapi.cpp:5596: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
../jsapi.cpp: In function ‘char* JS_EncodeString(JSContext*, JSString*)’:
../jsapi.cpp:5658: error: ‘JS_BYTES_PER_WORD’ was not declared in this scope
make[1]: *** [jsapi.o] Error 1
make: *** [default] Error 2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 34•16 years ago
|
||
#33 refers to a macosx shell build.
Comment 35•16 years ago
|
||
Never mind. autoconf-213 must be re-run in js/src to update configure, which then correctly seems to set JS_BYTES_PER_WORD. WFM now.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → WORKSFORME
Comment 36•16 years ago
|
||
I was not amused to see the title of this bug when I discovered that it was the cause of the bustage of my cross-compile build (even after running autoconf)...
Updated•16 years ago
|
Resolution: WORKSFORME → FIXED
Assignee | ||
Comment 37•16 years ago
|
||
Pushed to 1.9.1 as a part of the sequence of patches for bug 470071
Keywords: fixed1.9.1
Comment 38•16 years ago
|
||
Potentially caused PPC builds to crash on startup (see bug 475199)
You need to log in
before you can comment on or make changes to this bug.
Description
•