945512 - Differential Testing: Different output message involving RegExp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 1

[Hannes Verschore [:h4writer]]

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.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Assigning to Hannes since he has an analysis in comment 1, please feel free to change this if necessary.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Needinfo? from Hannes since he requested this in-person. :)

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: bug945512-regexp-fix

Only decide to make MRegExp movable after IonBuilder and after inspecting if the uses are not doing something strange with it.

Comment 5

[Sean Stangl [:sstangl]]

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?

Comment 6

[Hannes Verschore [:h4writer]]

(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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ae1ae5ef4cc1