Closed Bug 945512 Opened 6 years ago Closed 6 years ago

Differential Testing: Different output message involving RegExp

Categories

(Core :: JavaScript Engine: JIT, defect, major)

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: gkw, Assigned: h4writer)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

this.x += ''
try {
    (function() {
        for each(y in x) {
            r = RegExp("2", "")
            print("".replace(r, /x/.__proto__ = Proxy.createFunction((function() {
                return {}
            }), Uint32Array), ""))
        }
    })()
} catch (e) {}

when run with --fuzzing-safe --ion-eager, shows 3 newlines in stdout, but with only --fuzzing-safe, shows 10 newlines.

Tested on a 32-bit js dbg threadsafe more-deterministic shell on m-c changeset 5eb1c89fc2bc.

My configure flags are:

AR=ar sh ./configure --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe <other NSPR flags>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/738733ab166d
user:        Hannes Verschore
date:        Wed Mar 13 19:15:36 2013 +0100
summary:     Bug 848319: IonMonkey: Enable hoisting of MRegExp, r=sstangl

Hannes, is bug 848319 a possible regressor?
Flags: needinfo?(hv1989)
Slightly more sane testcase:

var handler = {
    has: function (name) {
        assertEq(1, 2);
    }
};

for (var i=0; i<10; i++) {
    var regex = /undefined/;
    regex.__proto__ = Proxy.createFunction(handler, function(){})
}


Yes, that is the correct regressor. So the issue is that regexp (MRegExp) get hoisted out of the loop. Afterwards we adjust the __proto__ property. So the 'regexp' in the next loop isn't a newly created version, but a tainted version. And that's why we have the difference in execution. Very unfortunate, since when using normally hoisting isn't observable. Only at these edge cases.

Solution would be
1) Mark MRegExp by default not movable.
2) Add a pass that marks MRegExp movable, when used by "safe" functions. Like "MustCloneRegExp", but much earlier, making sure the different passes can use this knowledge.
Flags: needinfo?(hv1989)
Assigning to Hannes since he has an analysis in comment 1, please feel free to change this if necessary.
Assignee: nobody → hv1989
Status: NEW → ASSIGNED
Needinfo? from Hannes since he requested this in-person. :)
Flags: needinfo?(hv1989)
Only decide to make MRegExp movable after IonBuilder and after inspecting if the uses are not doing something strange with it.
Attachment #8402792 - Flags: review?(sstangl)
Flags: needinfo?(hv1989)
Comment on attachment 8402792 [details] [diff] [review]
bug945512-regexp-fix

Review of attachment 8402792 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/IonAnalysis.cpp
@@ +1012,5 @@
>  bool
> +jit::MakeMRegExpHoistable(MIRGraph &graph)
> +{
> +    for (ReversePostorderIterator block(graph.rpoBegin()); block != graph.rpoEnd(); block++) {
> +        for (MDefinitionIterator iter(*block); iter; iter++) {

Prefer ++block and ++iter for Iterators.

@@ +1020,5 @@
> +            MRegExp *regexp = iter->toRegExp();
> +
> +            // Test if MRegExp is hoistable by looking at all uses.
> +            bool hoistable = true;
> +            for (MUseIterator i = regexp->usesBegin(); i != regexp->usesEnd(); i++) {

Prefer ++i for Iterators.

@@ +1023,5 @@
> +            bool hoistable = true;
> +            for (MUseIterator i = regexp->usesBegin(); i != regexp->usesEnd(); i++) {
> +                // Ignore resume points. At this point all uses are listed.
> +                // No DCE or GVN or something has happened.
> +                if (!i->consumer()->isDefinition())

Clearer as isResumePoint().

@@ +1026,5 @@
> +                // No DCE or GVN or something has happened.
> +                if (!i->consumer()->isDefinition())
> +                    continue;
> +
> +                // All MRegExp* MIR's don't adjust the regexp.

This appears to be true for the moment. It would be nice if this were asserted in the helper functions used by the MIR.

@@ +1035,5 @@
> +                    continue;
> +                if (use->isRegExpTest())
> +                    continue;
> +
> +                hoistable = false;

MPhi uses aren't handled. Is this intentional? Does it just not matter for benchmarks?
Attachment #8402792 - Flags: review?(sstangl) → review+
(In reply to Sean Stangl [:sstangl] from comment #5)
> Prefer ++foo instead of foo++

I actually don't agree on this.
1) All code in this file use foo++ instead of ++foo. So if we would what this we need to change all iterators in this file.
2) MDefinitionIterator even doesn't have "++iter". So we need to do iter++


> @@ +1026,5 @@
> > +                // No DCE or GVN or something has happened.
> > +                if (!i->consumer()->isDefinition())
> > +                    continue;
> > +
> > +                // All MRegExp* MIR's don't adjust the regexp.
> 
> This appears to be true for the moment. It would be nice if this were
> asserted in the helper functions used by the MIR.

It would be nice indeed, but I don't see how we can assert this?

> MPhi uses aren't handled. Is this intentional? Does it just not matter for
> benchmarks?

I didn't handle MPhi, since it adds extra complexity. And as far as I see, not a lot of scripts would have a MPhi in between MRegexp and MRegexp*. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1ae5ef4cc1
https://hg.mozilla.org/mozilla-central/rev/ae1ae5ef4cc1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
QA Whiteboard: [qa-]
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.