Closed Bug 503449 Opened 11 years ago Closed 9 years ago

lirasm: always build it with the js shell, and fix some breakage


(Core :: JavaScript Engine, defect)

Not set





(Reporter: njn, Assigned: graydon)



(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
This patch integrates lirasm so it's always built with the js shell.  (I
want this because I keep changing LIR and breaking lirasm without
realising.)  It can be turned off with --disable-lirasm at configure-time.

It also fixes some breakage I caused recently:
- loads now must have an immediate as their second operand
- setImm() no longer exists -- this isn't fixed properly, generally
  assemble_general() is dodgy with respect to recent LIR changes and I plan to
  fix this;  I just put an assertion in this case, it's not triggered by any of
  the tests so doesn't seem important right now.

And it gets rid of some compiler warnings. still passes so I can't have broken anything too badly.
Attachment #387796 - Flags: review?(jorendorff)
This patch fixes some errors and warnings that occur when you do an optimized build, as is done in tinderbox.
Attachment #387796 - Attachment is obsolete: true
Attachment #387806 - Flags: review?(jorendorff)
Attachment #387796 - Flags: review?(jorendorff)
Attached patch better patch (obsolete) — Splinter Review
This version works if you specify --disable-jit (ie. lirasm isn't built in that case).  It also fixes a Windows compile error relating to the position of JS_FASTCALL annotations, but there's another Window error I haven't fixed yet, to do with the overloading of 'sin', apparently.
Attachment #387806 - Attachment is obsolete: true
Attachment #387806 - Flags: review?(jorendorff)
This is the Windows error:

e:/builds/slave/win32-hg/build/js/src/lirasm/lirasm.cpp(121) : error C2440: 'type cast' : cannot convert from 'overloaded-function' to 'uintptr_t'
        Context does not allow for disambiguation of overloaded function

I guess it's because there is sin(double) and sin(float), but I'm enough of a C++ and MSVC newbie that I don't know how to fix it.  Suggestions welcome!
Ugh. Maybe you can fix it with an intermediate cast?

   (uintptr_t) (double (*)(double)) sin

That works for overloaded functions in gcc anyway.
>+if test "$ENABLE_JIT"; then

I think you can just write:


>+                         verbose_only( , func->first.c_str() ) };

I think this will fail if the compiler doesn't support __VA_ARGS__. Since we still support MSVC 7.1, which doesn't have __VA_ARGS__, that would mean this must be reworded.

We really shouldn't have verbose_only definitions that use __VA_ARGS__. For us, they serve only to mask problems like this.
I used the casting hack to fix the ambiguity on Windows.  I used DEBUG_ONLY_NAME (rejigging it along the way) to fix the verbose_only thing.  I did the ENABLE_LIRASM shortening you suggested.  And I refreshed to the hg tip.  I think it's ready for a proper review now!
Attachment #387828 - Attachment is obsolete: true
Attachment #389912 - Flags: review?(jorendorff)
Comment on attachment 389912 [details] [diff] [review]
updated patch, fixes all known problems

Great! Push it!
Attachment #389912 - Flags: review?(jorendorff) → review+
Whiteboard: fixed-in-tracemonkey
Sayre, because a whole new file (lirasm.cpp) is now compiled by default as a result of this commit, you should probably let it marinate in TM for a while before merging to m-c.
had to back this out

test doesn't run on windows
Whiteboard: fixed-in-tracemonkey
More info... Windows log looks like this:


command timed out: 300 seconds without output
program finished with exit code 1
TinderboxPrint: check<br/><em class="testfail">T-FAIL</em>
buildbot.slave.commands.TimeoutError: command timed out: 300 seconds without
TinderboxPrint: check<br/><em class="testfail">timeout</em>
=== Output ended ===

And this output on Mac (although it doesn't cause tinderbox to go orange):

dyld: Library not loaded: @executable_path/libplds4.dylib
  Referenced from:
  Reason: image not found
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/ line
7: 71956 Trace/BPT trap          ./lirasm --execute $infile > testoutput.txt
incorrect output for
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/ ===
actual output ===
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/ ===
expected output ===
Output is: 5

No idea about either of them.  I'm seeing a couple of failures on my own Mac, but not traps.  Tinderbox linux is fine.  Sigh.
Ah. This is one I know how to fix. What's happening is that the libs built by the *browser configuration* of the shell will be multithreaded, so have a runtime dependency on NSPR, and by connecting the lirasm/Makefile into the main build, you're going to have it attempt to run the lirasm 'check' target, which tries to run

This in turn will fail when it cannot find the NSPR libs, and windows will pop up a dialog box asking for the missing DLL. It's running headless, of course, so that's the timeout.

Had the same problem getting trace-test running as part of 'make check'. It requires a careful invocation of $(RUN_TEST_PROGRAM) to set up the environment.

If you like I'll try to fix this and land it, I've got a windows VM with the correct sort of build environment running here.
Assignee: nnethercote → graydon
(In reply to comment #12)
> This in turn will fail when it cannot find the NSPR libs

Why can't it find the NSPR libs?

> If you like I'll try to fix this and land it, I've got a windows VM with the
> correct sort of build environment running here.

Yes please!  I'll give you a gold star when it's done :)
Oh, just something to do with the invocation directory and the windows DLL path-searching algorithm. It's not a "bug", we have makefile support for setting it up right, it just requires using that support.

Of course, now that I've said all this, I'm not able to reproduce the bug in my VM just now :) But it's the exact same symptom as the other case, so I'm sure I'll work it out.
This version:

  - Fixes the test execution DLL issue on win32
  - Strips test output CRs on win32 so they can be 'cmp'ed properly
  - Moves the test-running logic from a shell script into the
  - Standardizes the test output so that the tinderboxes will parse it
  - Changes tests that relied on the return value of puts(), which is only
    spec'ed to return a non-negative value, not any particular one (and 
    differs on win32).

I'll try-server it presently. Could use some style feedback on the Makefile, I never know what sort of GNU-make-isms are kosher here.
Attachment #389912 - Attachment is obsolete: true
Attachment #390575 - Flags: review?(jorendorff)
Sigh, I *wish* that caught it. Of course it still times out on win32 (and fails to build altogether on windows mobile). Another round of fun will be required.
(In reply to comment #16)
> (and fails to build altogether on windows mobile).

Windows Mobile tinderbox has been mostly red for the past few days, AFAICT it's broken somehow.
Comment on attachment 390575 [details] [diff] [review]
various win32 and check fixes

r=me on the changes so far, though comment 16 implies more work before this goes in.

I'd prefer to keep a separate test-running shell script than to move that into the Makefile.  No great reason, I just like troubleshooting bash better than troubleshooting a makefile that employs the backslash so liberally.
Attachment #390575 - Flags: review?(jorendorff) → review+
These bugs are all part of a search I made for js bugs that are getting lost in transit:

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.
Now that Nanojit has its own tinderbox, any lirasm breakage will be detected there, so there's no need for it to be built with the JS shell.  As for the other minor fixes, they've almost all been fixed independently or the code has changed enough that they're not relevant.
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.