Closed
Bug 625600
Opened 14 years ago
Closed 13 years ago
Update Yarr import
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: dmandelin, Assigned: dmandelin)
References
Details
(Whiteboard: fixed-in-tracemonkey wanted-standalone-js)
Attachments
(9 files, 8 obsolete files)
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/x-zip-compressed
|
Details |
I don't think this should block for now; we should only block on things that are proven to affect the web. But it's fairly important and we should do as much of this as time permits before Fx4. We should probably start by surveying all the changes that landed since Chris's last import, seeing if they fix things that look important and if they caused regressions, and go from there.
Assignee | ||
Comment 1•14 years ago
|
||
Preliminary report:
Currently, our import of yarr is:
rev 68100
disable the yarr interpreter
plus cherry-picked rev 72781
The revs that change yarr that we have not taken are:
* Fixes
72813 + 73594 - don't match bogus char class + back off a bit for web compat
73617 - something with backtracking
74309 - crash on Yelp
75408 - make syntax errors early errors
75796 - backtracking for nested alternatives
75991 - fuzzer crash
76275 - crash
76407 - hang
* Regression Fixes
73640
73866
73962
76076 + 76133 (followup for regression introduced by 76076)
* Refactorings
72999 - typos
73124
74412
74600
75421
[[75595 + 75597]] (backed-out patch)
75602
* Optimizations
68207 - improve v8-regexp by 2.7%
72180
73307
73903
74441 - reduce cache mem use
76502
* Interpreter
68127
68771
69781
69842 + 69847 (build fix for 69842)
70764
72140
72186
[[72197 + 72781]] (backed-out patch)
Assignee | ||
Comment 2•14 years ago
|
||
I think the next step is to go through the fixes and regression fixes and see which we are currently affected by. Then we can try to import the patches that fix those. But we need to check for regressions from them, and see which additional patches we would need to import in order to import those patches.
Assignee | ||
Comment 3•14 years ago
|
||
For all the fixes (not including regression fixes) that have obvious test cases, we currently pass those tests. So I think we are pretty good here and don't need to do anything else before Fx4.
Assignee | ||
Updated•14 years ago
|
Assignee: general → dmandelin
Assignee | ||
Comment 4•14 years ago
|
||
I'm working on this now. It's probably going to take a week or two, partly because it's been a few months since we've updated so there's a lot to go through, and partly because I'll be gone for a day or two for a conference next week.
Assignee | ||
Comment 5•13 years ago
|
||
Starting this up again. In order to make future merging easier and upstreaming possible, I want to make minimal changes to the WebKit code, and instead add a bridge that does things like implement the WebKit Vector interface on top of our implementation. Most of this should not be too hard, but there is a sticking point:
The JSC/Yarr code uses refcounting smart pointers quite a bit: a RefCounted template, RefPtr and PassRefPtr pointers, etc. Ideally, we would just provide our own implementation of enough of this stuff to make it work.
The problem is that the refcounting template code in WebKit is LGPL 2+, which, IIUC, is not compatible with the tri-license and cannot be imported into our tree. What can I do?
Can I reimplement that functionality myself? I have looked at those files a little bit, e.g., to understand what they are for and to check on their license.
Comment 6•13 years ago
|
||
I can confirm that LGPL2+ is not permitted by Mozilla licensing policy (and won't be even when/if we move to MPL 2).
Gerv
Assignee | ||
Updated•13 years ago
|
Blocks: CVE-2011-3232
Assignee | ||
Comment 7•13 years ago
|
||
Builds on Windows but asserts if you try to run a regexp--lots of fixmes left. The good news is that I was able to get it to build making relatively few changes to the Yarr code. I think I will make Yarr use our hash tables, though--they only use a hash table in one place, and the API impedance mismatch is pretty bad.
Assignee | ||
Comment 8•13 years ago
|
||
This works for the most part on Windows. There are some jit-test failures. One is bug 576837, which I had thought was fixed upstream, but apparently not. I can just take our fix and hopefully upstream that as well. There are some other failures that seem to work correctly in the latest WebKit, so I'll have to update to that. There are also about 20 other issues to check on, such as Mac/Linux testing and header file cleanups.
Attachment #532027 -
Attachment is obsolete: true
Assignee | ||
Comment 9•13 years ago
|
||
Passes jit-tests on Windows. Now I just need to crank through 20 or so engineering issues. The only hard one is memory management--it probably leaks right now and needs some replacement for the refcounting smart pointers.
Attachment #532278 -
Attachment is obsolete: true
Assignee | ||
Comment 10•13 years ago
|
||
What's left is to be fixed up for Mac and Linux, update license files, and fix memory managements.
Attachment #532352 -
Attachment is obsolete: true
Assignee | ||
Comment 11•13 years ago
|
||
Almost done. I just need to check on Linux compat.
Attachment #532379 -
Attachment is obsolete: true
Assignee | ||
Comment 12•13 years ago
|
||
I found a few bugs with jstests, so I had to refresh Yarr, readapt that (which turned out to be very easy with my approach of keeping a separate "adapt Yarr to Moz" patch), and fix another bug that cdleary might have fixed earlier, or else WebKit decided to unfix.
This still needs Linux/Mac testing, but hopefully is really really close now.
Attachment #532764 -
Attachment is obsolete: true
Assignee | ||
Comment 13•13 years ago
|
||
I'm going to put this up as a series of patches, so that each part of the adaptation is documented. This should also make it easier to review.
Question about landing, though: Do you think I should land it in parts, so that it is permanently documented in the tree, or as one patch, so that it's easier to back out later if that's required?
Attachment #532816 -
Attachment is obsolete: true
Attachment #532830 -
Flags: review?(cdleary)
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #532831 -
Flags: review?(cdleary)
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #532832 -
Flags: review?(cdleary)
Assignee | ||
Comment 16•13 years ago
|
||
I kept this separate because it's more likely to change than the rest.
Attachment #532833 -
Flags: review?(cdleary)
Assignee | ||
Comment 17•13 years ago
|
||
This bug was latent in Yarr rev 86639. It is not present in our initial import; I don't know if cdleary fixed it or if Yarr had it right at that point.
Attachment #532834 -
Flags: review?(cdleary)
Assignee | ||
Comment 18•13 years ago
|
||
cdleary fixed this in the original import; this is just an update of his fix. Yarr previously fixed this bug, but they fixed it too hard (too restrictive), so they undid the fix.
Attachment #532835 -
Flags: review?(cdleary)
Assignee | ||
Comment 19•13 years ago
|
||
This is optional; it's not needed to fix this bug correctly. We had previously added 'isValid' methods to some Nitro asm jump holder classes. Since then, they have added 'isSet'. This just renames one of our 'isValid' to match their 'isSet', in a class that would otherwise need both APIs.
Attachment #532837 -
Flags: review?(dvander)
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #532838 -
Flags: review?(cdleary)
Assignee | ||
Updated•13 years ago
|
Attachment #532831 -
Attachment is obsolete: true
Attachment #532831 -
Flags: review?(cdleary)
Assignee | ||
Comment 21•13 years ago
|
||
Attachment #532832 -
Attachment is obsolete: true
Attachment #532841 -
Flags: review?(cdleary)
Attachment #532832 -
Flags: review?(cdleary)
Blocks: 657804
Updated•13 years ago
|
Attachment #532837 -
Flags: review?(dvander) → review+
Comment 22•13 years ago
|
||
Comment on attachment 532830 [details] [diff] [review]
Part 1: Remove old yarr files
A few comments:
- I'm guessing that all of the FIXMEs are imported and left in for ease of merging from upstream.
- I would make sure this all builds with --disable-methodjit (I see you added the include of "methodjit/MethodJIT.h" in the inlines file).
- I'd prefer it if we could keep the android bug-reference comment -- ideally that EnableYarrJIT wouldn't be there at all.
- executeInternal has an awkwardly expressed JS_ASSERT(result >= 0) with JS_ASSERT(0)s in it.
- "const size_t notFound = static_cast<size_t>(-1);" Does the static cast buy you anything here? I think signed -1 should just signex to fill the size_t.
- In compileHelper you have to JS_ASSERT(0) if !ENABLE_YARR_JIT. Maybe this will be changed in the rest of the patches?
I'm not sure this is something we can commit piecemeal, since all the prior YARR files are ripped out. Making changesets which can compile and run the full gamut of tests is the rule of thumb I'm familiar with.
Attachment #532830 -
Flags: review?(cdleary) → review+
Comment 23•13 years ago
|
||
Comment on attachment 532838 [details] [diff] [review]
Part 2 (fixed): Import Yarr rev 86639 unmodified
Not having reviewed all the constituent changesets on the webkit side, rs=me.
Attachment #532838 -
Flags: review?(cdleary) → review+
Comment 24•13 years ago
|
||
Comment on attachment 532841 [details] [diff] [review]
Part 3 (fixed): Adapt Yarr rev 86639 to build in Spidermonkey
These char-error interfaces introduce some unfortunate merge-from-upstream issues -- maybe we can get those a little more loosely coupled upstream in the future.
One note: it looks like some of your Makefiles are replacing tabstops with 4 softabstop, in the prior patches as well, which I forgot to mention.
Attachment #532841 -
Flags: review?(cdleary) → review+
Comment 25•13 years ago
|
||
Comment on attachment 532833 [details] [diff] [review]
Part 4: Fix Yarr memory management to work with SpiderMonkey
There's one "// YYY" comment that may need to be removed. When I first implemented the CharTable ownership there were a few tricky leaks -- this doesn't look different from the way it was, but if it happens to be or you're feeling extra cautious I would give the /regexp/i reftests a run through valgrind.
Attachment #532833 -
Flags: review?(cdleary) → review+
Updated•13 years ago
|
Attachment #532834 -
Flags: review?(cdleary) → review+
Updated•13 years ago
|
Attachment #532835 -
Flags: review?(cdleary) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Landed with a few nit-fixes added.
http://hg.mozilla.org/tracemonkey/rev/de6dfe16fd91
Assignee | ||
Comment 27•13 years ago
|
||
Backed out: http://hg.mozilla.org/tracemonkey/rev/6422857800fe
I think I bumped into the bug changing the new ctors. Will push a corrected version soon.
Assignee | ||
Comment 28•13 years ago
|
||
Assignee | ||
Comment 29•13 years ago
|
||
Rebacked out. I already bounced this off TM a few times today, thinking that maybe there was just a small issue or 2. But there is some jsvector re-entrance issue, and some ARM build stuff, that for some reason didn't pop on try. So I'm not sure if this will be able to land before tomorrow. I'm not sure that a 2MB that lands just before the branch would be likely to get through Aurora without getting backed out, anyway. But we'll see.
Assignee | ||
Comment 30•13 years ago
|
||
Takes care of issues introduced by bug 657384 and works correctly on no-jit builds. There are still some Android build issues to be sorted out.
Assignee | ||
Comment 31•13 years ago
|
||
Looks like it finally stuck:
http://hg.mozilla.org/tracemonkey/rev/cc36a234d0d6
http://hg.mozilla.org/tracemonkey/rev/c8695a65e1e7
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 32•13 years ago
|
||
This is a zip of the corrected patch queue I used to do this Yarr import. It has these patches, in this order:
del-old-yarr.diff
Deletes the old Yarr files, from the first import, to get a clean tree.
import-yarr-rev-86639.diff
Copies in Yarr files from the given WebKit rev, and nothing more.
adapt-yarr-rev-86639.diff
Adapts Yarr and SpiderMonkey to each other, except for memory management
issues, because those are more likely to change.
fix-mem.diff
Adapt memory management.
fix-number.diff
Fix a Yarr bug with detecting out-of-range numeric inputs.
fix-cc.diff
Fix a Yarr bug with being too permissive on character classes
opt-isset.diff
Refactor a bit of our imported Yarr assembler to more closely match WebKit.
The idea here is that the next refresh of Yarr rev R can be done like this:
1. Delete the current Yarr files. Name this patch del-yarr-rev-86639.diff.
2. Copy in Yarr files for rev R. Name this patch import-yarr-rev-R.diff.
3. Apply the rest of the patches in order, fixing them as necessary and updating.
I actually had to do this during my work on this bug, because I discovered a bug in Yarr that had been fixed a few days after my import. I found that the patch queue applied pretty cleanly.
Comment 39•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/cc36a234d0d6
http://hg.mozilla.org/mozilla-central/rev/c8695a65e1e7
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 41•13 years ago
|
||
Where is this fixed? Please set status flags and Target Milestone
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla8
Updated•13 years ago
|
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey wanted-standalone-js
You need to log in
before you can comment on or make changes to this bug.
Description
•