bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
9 years ago
8 years ago

People

(Reporter: njn, Assigned: graydon)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 387796 [details] [diff] [review]
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.
Attachment #387796 - Flags: review?(jorendorff)
(Reporter)

Comment 1

9 years ago
Created attachment 387806 [details] [diff] [review]
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.
Attachment #387796 - Attachment is obsolete: true
Attachment #387806 - Flags: review?(jorendorff)
Attachment #387796 - Flags: review?(jorendorff)
(Reporter)

Comment 2

9 years ago
Created attachment 387828 [details] [diff] [review]
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.
Attachment #387806 - Attachment is obsolete: true
Attachment #387806 - Flags: review?(jorendorff)
(Reporter)

Comment 3

9 years ago
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
>+    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.
(Reporter)

Comment 6

9 years ago
Created attachment 389912 [details] [diff] [review]
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!
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+
(Reporter)

Comment 8

9 years ago
http://hg.mozilla.org/tracemonkey/rev/8f6d8ce284ff
Whiteboard: fixed-in-tracemonkey
(Reporter)

Comment 9

9 years ago
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

9 years ago
had to back this out

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

test doesn't run on windows
Whiteboard: fixed-in-tracemonkey
(Reporter)

Comment 11

9 years ago
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.
(Assignee)

Comment 12

9 years ago
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.
Assignee: nnethercote → graydon
(Reporter)

Comment 13

9 years ago
(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 :)
(Assignee)

Comment 14

9 years ago
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.
(Assignee)

Comment 15

9 years ago
Created attachment 390575 [details] [diff] [review]
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.
Attachment #389912 - Attachment is obsolete: true
Attachment #390575 - Flags: review?(jorendorff)
(Assignee)

Comment 16

9 years ago
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.
(Reporter)

Comment 17

9 years ago
(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:

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.
(Reporter)

Comment 20

8 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.