Closed
Bug 61314
Opened 24 years ago
Closed 23 years ago
Makefile rules for "jsmath.o" and "jsmathtemp.o" cause stale object files, obscure bugs
Categories
(Core :: JavaScript Engine, defect, P3)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
Future
People
(Reporter: buhr+mozilla2, Assigned: mang)
Details
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
brendan
:
review+
brendan
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
Damn it. I just spent an hour and half chasing after a crashing page to
discover that I was using a stale copy of "jsmath.o".
Why? Because "jsmath.o" is automagically generated from "jsmathtemp.o", but the
dependencies for "jsmathtemp.o" are wrong. A ".deps/jsmathtemp.pp" is created,
but it has "jsmath.o: ..." as its target, and the Makefile specifies that
"jsmathtemp.o" is only dependent on "jsmath.c". Guess what happens when, say,
"jscntxt.h" changes but "jsmath.c" doesn't? "jsmath.o" is regenerated from the
stale copy of "jsmathtemp.o"!
This has probably generated plenty of false bug reports about random Javascript
crashes. I crashed on Netcenter, of all things, because a stale
jsmath.c::random_init() wrote into a totally random part of the JSRuntime
context (it wrote over "contextList" instead of "rngDscale". If I hadn't spent
the hour and a half working on this, I would have filed a bogus bug report that
half the developers (using an incremental build) could reproduce and the other
half (using full builds or a pre-built snapshot) couldn't.
As a quick fix, someone could check in *any* kind of change to "js/src/jsmath.c"
to kick the existing, bogus Makefile rule.
As a better fix, the following patch to Makefile.in works for me for a Linux
build (for example, jsmathtemp.o/jsmath.o and jsapi.o are rebuilt when jsmath.h
is touched).
Reporter | ||
Comment 1•24 years ago
|
||
Comment 2•24 years ago
|
||
Reassigning to mccabe -
Assignee: rogerl → mccabe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•24 years ago
|
||
With this fix, does JSMATH_PRELINK (mathtemp.o) get built for sure?
Also, if you can reproduce this problem, any chance you could confirm whether it
happens with the JS engine standalone build system? (It's defined in
Makefile.ref, and builds a js library and accompanying commandline shell - it's
what we use for testing.)
CC'ing mang; mang - could you take a peek?
Reporter | ||
Comment 4•24 years ago
|
||
Ugh. You're quite right.
For GNU make, it's still broken with respect to changes to files in the "fdlibm"
subdirectory. If you modify "fdlibm/s_cos.c", it will rebuild the library, but
it won't rebuild "jsmathtemp.o" unless you re-make a second time. Of course,
the original Makefile.in code is *ALSO* broken with respect to this. It doesn't
relink "jsmath.o" against the new library unless you "make" a second time.
I've been bitten by this GNU make behavior before. I'll figure out how to get
it to work for all cases, but someone else will have to verify it works under
Windows (and with any non-GNU "make"s we might be supporting).
Reporter | ||
Comment 5•24 years ago
|
||
The problem is that the dependency "jsmathtemp.o: $(FDLIBM_LIBRARY)" is tested
at the *beginning* of the "install" pass. However, as long as $(FDLIBM_LIBRARY)
exists, it won't yet have been modified (based on changes to files in
"fdlibm/*") until the *end* of the "install" pass. That's why "make" will never
know that "jsmathtemp.o" needs to be rebuilt in this particular case.
The following patch (against a clean Makefile.in) may be ugly, but damn it, it
works. It forces a remake of $(FDLIBM_LIBARY) at the end of the "export" pass,
so it's always been updated by the beginning of the "install" pass which then
builds "jsmathtemp.o" and "jsmath.o" reliably *if* $(FDLIBM_LIBRARY) or any of
"jsmath.c"'s dependencies have actually changed.
I've tested the following scenarios:
1. "make clean" and "make": makes "libfdm.a" at end of export, then starts
"install" pass and compiles "jsmathtemp.o", links "jsmath.o", installs
"libmozjs.so", and installs "libfdm.a".
2. Another "make": recompiles/relinks nothing (hooray!) and does the usual
"nsinstall"s in the correct order.
3. Modify "jsmath.h" and "make" (was original bug): at end of export pass,
finds "libfdm.a" is up-to-date; in "install" pass, recompiles "jsapi.c" and
"jsmathtemp.c" (which depend on "jsmath.h", then relinks "jsmath.o" against
"libfdm.a"; finally, relinks and installs "libmozjs.so" and installs "libfdm.a".
4. Another "make": hooray! Doesn't recompile anything!
5. Modify "fdlibm/s_cos.c" and "make" (the second bug): recompiles "s_cos.c",
links "libfdm.a", compiles "jsmathtemp.c" (not really necessary, but it's not
worth optimizing this case), relinks "jsmath.o" against new "fdlibm/libfdma";
finally relinks and installs "libmozjs.so" and reinstalls "libfdm.a".
6. Another "make" compiles nothing! I'm ecstatic!
7. Modify "jsmath.h" and "fdlibm/s_cos.c": recompiles "s_cos.c", relinks
"libfdm.a", recompiles "jsapi.c" and "jsmathtemp.c", relinks "jsmath.o", relinks
and installs "libmozjs.so", and install "libfdm.a".
8. Another "make" does nothing.
It looks good, unless you can think of a missing test case. Someone please
check these cases under Windows!!
Someone else will have to test "Makefile.ref". I simply get an error when
"config.mk" can't include "config/Linux_All.mk". I can construct a sample patch
that should work (because of the PREDIRS scheme, my original patch for
"Makefile.in" will work correctly for "Makefile.ref"; there will be no second
bug), but I can't test it.
You can reproduce the original bug easily enough. "make" your "Makefile.ref".
Now, modify "jsmath.h" and *only* "jsmath.h". If you rebuild and "jsmath.c"
isn't recompiled, something went horribly wrong.
Two patches follow.
Reporter | ||
Comment 6•24 years ago
|
||
Reporter | ||
Comment 7•24 years ago
|
||
Reporter | ||
Comment 8•24 years ago
|
||
I should add that I believe adding this make at the end of the "export" pass is
safe because, as far as I can see, the code in "fdlibm" depends on no headers or
other generated files except "fdlibm.h" in that same directory, so there's no
other directory's "export" results needed to build "libfdm.a". Am I right?
Assignee | ||
Comment 9•24 years ago
|
||
Thanks for finding this. The dependencies here are a little tricky. I'm pretty
sure all binaries that get sent out are built clobber, so at least that's not an
issue.
I'm currently looking at the patch for Makefile.ref. If you want to pick up the
config files and editline library you can do "cvs up -d config editline" in the
js/src/ directory.
When building with Makefile.ref there are currently *no* dependencies on header
files at all (guess it hasn't been an issue). When rebuilding jsmath.o, you
also need to depend on jslibmath.h. I'm tweaking your patch to add this, since
it could be a real source of confusion.
fdlibm shouldn't have any external dependencies.
Assignee: mccabe → mang
Assignee | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•24 years ago
|
||
Assignee | ||
Comment 11•24 years ago
|
||
My patch for JSRef has Kevin's stuff and adds
- dependency on jslibmath.h
- rule for .h dependencies (doesn't cover all .h files)
- NPL/GPL boilerplate for rules.mk
Tested on Linux. I added an *UNTESTED* rule for Windows. mccabe, can you maybe
try this out?
Kevin, my patch has you in the contributors section of Makefile.ref. Pls let me
know if the info is incorrect.
Comment 12•24 years ago
|
||
Get a review, and we'll check this in for you.
Target Milestone: --- → Future
Comment 13•23 years ago
|
||
Can someone review this patch ?
pschwartau : Can you help or is this patch already bitrotten ?
Comment 14•23 years ago
|
||
Matti: thanks. cc'ing reviewers to get this moving again -
Comment 15•23 years ago
|
||
r=rogerl. I had a little trouble applying the previous patch (bitrot?) so I'm
attaching this newer version. I tested it out on Windows, but I think someone
else now needs to r= it.
Comment 16•23 years ago
|
||
Comment on attachment 71767 [details] [diff] [review]
Updated to trunk
Recording rogerl's r=.
I don't mind the %.o: %.c %.h inference rules, but they're nowhere near enough
to recompile without clobbering safely. I'm happy to approve this patch, but
I'm hoping some kind soul steps up and integrates gcc -MD and so on support, so
we have automatic and complete dependency inference.
sr=brendan@mozilla.org
/be
Attachment #71767 -
Flags: superreview+
Attachment #71767 -
Flags: review+
Comment 17•23 years ago
|
||
Comment on attachment 71767 [details] [diff] [review]
Updated to trunk
a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #71767 -
Flags: approval+
Comment 18•23 years ago
|
||
Comment on attachment 71767 [details] [diff] [review]
Updated to trunk
fix checked into trunk
Comment 19•23 years ago
|
||
marking fixed...
Thanks for the reviews !
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Flags: testcase-
You need to log in
before you can comment on or make changes to this bug.
Description
•