Closed Bug 269538 Opened 16 years ago Closed 12 years ago

jscpucfg not cross compile friendly

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jordan.crouse, Assigned: benjamin)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 3 obsolete files)

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:
Status: UNCONFIRMED → NEW
Ever confirmed: true
-> default qa
QA Contact: pschwartau → general
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
Blocks: 432792
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.
OS/Platform -> All/All based on comment 3.
OS: Linux → All
Hardware: PC → All
Yeah, we want that as a series of configure tests with AC_TRY_COMPILE
Attached patch Neuter most of jscpucfg, rev. 1 (obsolete) — Splinter Review
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)
Attachment #357026 - Flags: review?(jim)
Attachment #357026 - Flags: review?(crowder) → review+
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 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
This is in x86-specific code, so I think it's ok.
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.
And possibly bug 467453, too.
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.
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 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.
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.
Ah, you're right.  Endianness tests require actually running
code, so they can't be reimplemented with AC_TRY_COMPILE.
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 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?
nm, I see now.
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 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+
Attachment #357168 - Attachment is obsolete: true
jslong.cpp and even jslong.h can go away -- separate bug better?

/be
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+
Yeah, that's 470791, not on my short-list (I want to get this in for 1.9.1)
http://hg.mozilla.org/mozilla-central/rev/525e42406396
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Resolution: --- → FIXED
backed out due to Windows TUnit failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #358221 - Flags: review? → review?(crowder)
Attachment #358221 - Flags: review?(crowder) → review+
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
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?
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: 12 years ago12 years ago
Resolution: --- → FIXED
We pulled it over a little while ago. Thanks.
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 → ---
#33 refers to a macosx shell build.
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: 12 years ago12 years ago
Resolution: --- → WORKSFORME
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)...
Resolution: WORKSFORME → FIXED
Depends on: 475027
Pushed to 1.9.1 as a part of the sequence of patches for bug 470071
Keywords: fixed1.9.1
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.