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

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

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.

testlirc.sh still passes so I can't have broken anything too badly.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch that works with optimised builds

This patch fixes some errors and warnings that occur when you do an optimized build, as is done in tinderbox.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: better patch

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.

Comment 3

[Nicholas Nethercote [inactive]]

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!

Comment 4

[Jason Orendorff [:jorendorff]]

Ugh. Maybe you can fix it with an intermediate cast?

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

That works for overloaded functions in gcc anyway.

Comment 5

[Jason Orendorff [:jorendorff]]

>+if test "$ENABLE_JIT"; then
>+    ENABLE_LIRASM=1
>+else
>+    ENABLE_LIRASM=
>+fi

I think you can just write:

  ENABLE_LIRASM=$ENABLE_JIT

>+                         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.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: updated patch, fixes all known problems

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!

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 389912 [details] [diff] [review]
updated patch, fixes all known problems

Great! Push it!

Comment 8

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/8f6d8ce284ff

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Robert Sayre]

had to back this out

http://hg.mozilla.org/tracemonkey/rev/8c77a292faf8

test doesn't run on windows

Comment 11

[Nicholas Nethercote [inactive]]

More info... Windows log looks like this:

/e/builds/moz2_slave/tracemonkey-win32-unittest/build/js/src/lirasm/testlirc.sh

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


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

/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/testlirc.sh
dyld: Library not loaded: @executable_path/libplds4.dylib
  Referenced from:
/builds/slave/tracemonkey-macosx-unittest/build/objdir/js/src/lirasm/./lirasm
  Reason: image not found
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/testlirc.sh: line
7: 71956 Trace/BPT trap          ./lirasm --execute $infile > testoutput.txt
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/testlirc.sh:
incorrect output for
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/tests/add.in
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/testlirc.sh: ===
actual output ===
/builds/slave/tracemonkey-macosx-unittest/build/js/src/lirasm/testlirc.sh: ===
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.

Comment 12

[Graydon Hoare :graydon]

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 testlirc.sh.

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.

Comment 13

[Nicholas Nethercote [inactive]]

(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 :)

Comment 14

[Graydon Hoare :graydon]

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.

Comment 15

[Graydon Hoare :graydon]

Attachment: various win32 and check fixes

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 Makefile.in
  - 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.

Comment 16

[Graydon Hoare :graydon]

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.

Comment 17

[Nicholas Nethercote [inactive]]

(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 18

[Jason Orendorff [:jorendorff]]

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.

Comment 19

[Nochum Sossonko [:Natch]]

These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

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.

Comment 20

[Nicholas Nethercote [inactive]]

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.