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)

defect

Tracking

()

VERIFIED FIXED
Future

People

(Reporter: buhr+mozilla2, Assigned: mang)

Details

Attachments

(5 files)

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).
Reassigning to mccabe -
Assignee: rogerl → mccabe
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
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).
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.
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?
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
Status: NEW → ASSIGNED
Attached patch Proposed patch for JSRef (deleted) — Splinter Review
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.
Get a review, and we'll check this in for you.
Target Milestone: --- → Future
Can someone review this patch ? pschwartau : Can you help or is this patch already bitrotten ?
Keywords: patch, review
Matti: thanks. cc'ing reviewers to get this moving again -
Attached patch Updated to trunk (deleted) — Splinter Review
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 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 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 on attachment 71767 [details] [diff] [review] Updated to trunk fix checked into trunk
marking fixed... Thanks for the reviews !
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Marking Verified -
Status: RESOLVED → VERIFIED
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: