944975 - Differential Testing: Different output message involving Array.prototype.scatterPar

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

try {
    Array.prototype.scatterPar.toSource = (function() {
        print("FOO")
    });
    new Array.prototype.scatterPar;
} catch (e) {}

when run with --fuzzing-safe --no-baseline --no-ion --no-ti, shows "FOO" 1 time in stdout, but with only --fuzzing-safe, shows nothing at all.

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

My configure flags are:

sh ./configure --enable-optimize --enable-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/23dda916c3d0
user:        Shu-yu Guo
date:        Sat May 11 22:39:46 2013 -0700
summary:     Bug 860965 - Part 1: Copy 1D ParallelArray operations to Array. (r=luke,nmatsakis)

Shu-yu, is bug 860965 a possible regressor?

Comment 1

[Shu-yu Guo [:shu]]

PA was removed.

Comment 2

[Jesse Ruderman]

Array.prototype.scatterPar was not removed.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Jesse Ruderman from comment #2)
> Array.prototype.scatterPar was not removed.

Doh, I thought this was PA.p.scatterPar

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

I am looking at this.  I was not able to reproduce on my current head:

  changeset:   166605:e020404995da
  user:        Carsten "Tomcat" Book <[e-mail]>
  date:        Mon Feb 03 13:02:58 2014 +0100
  summary:     merge b2g-inbound to mozilla-central

but I *was* able to reproduce back on 23dda916c3d0.

The relevant flag that toggles the behavior on/off is --no-ti.

I am going to figure out which changeset caused the behavior to go away.  (Hopefully this was fixed and the fixer just did not know about this ticket.)

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Hmm.  Now I *can* reproduce on my current head: 

% hg log -r $(hg id | cut -d ' ' -f 1)
changeset:   166605:e020404995da
tag:         tip
parent:      166591:822677d8f943
parent:      166604:d16af1ac5eef
user:        Carsten "Tomcat" Book <[e-mail]>
date:        Mon Feb 03 13:02:58 2014 +0100
summary:     merge b2g-inbound to mozilla-central

(I'm not sure why I wasn't able to reproduce earlier.  The only thing I think I changed between then and now is my configure invocation.)

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Here's a reduced configure invocation I used for reproduction after starting with the original one that gkw posted:

% ../js/src/configure --with-ccache --enable-type-inference --enable-more-deterministic
% make
% ./js/src/shell/js ../bug944975.js
% ./js/src/shell/js --no-ti ../bug944975.js
FOO

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

So as one might have guessed, this problem is not specific to |Array.prototype.scatterPar|.  The same phenomenon arises for other PJS API methods; in particular, I tested it against |mapPar| as well.

But it does not occur for all self-hosted methods.  In particular, the |Array.prototype.map| method does not suffer from this problem.

The main difference I can identify between the PJS methods and the non-PJS methods is that we set the cloneAtCallsite script hint for the PJS methods.  And a quick hack of commenting *out* the hint brings the behavior back in line; i.e. the problem goes away for |mapPar| after commenting out this line in js/src/builtin/Array.js:

  SetScriptHints(ArrayMapPar,         { cloneAtCallsite: true });

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(I'm betting the reason that the  --enable-more-deterministic flag to configure is relevant is because DecompileValueGenerator's call to DecompileExpressionFromStack bombs out early in that mode, and DecompileValueGenerator thus falls back on ValueToSource.)

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: fix bug via helper method to get original fcn

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch B v1: code cleanup, rename JSScript::originalFunction to donorFunction

(apply B after patch A.)

This is some code cleanup.  I decided that it was confusing to have JSFunction::originalFunction return |this| if this is not a call-site clone while JSScript::originalFunction returns null in that case.  The easiest way to resolve this conflict in my mind was a slight renaming, so now:

 - donorFunction: "assert this is callsite clone, and return the donor"

 - originalFunction: "return donor if this is callsite clone; o/w return this."

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch T v1: regression test for the bug

(patch "T" for "test"; this can be applied independently of whether patches A+B are applied.)

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8369973 [details] [diff] [review]
patch A v1: fix bug via helper method to get original fcn

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

::: js/src/jsfun.h
@@ +345,5 @@
> +        if (this->hasScript() && this->nonLazyScript()->isCallsiteClone()) {
> +            return this->nonLazyScript()->originalFunction();
> +        } else {
> +            return this;
> +        }

Style nit: SpiderMonkey style is no else-after-return and no braces around single-line conditional bodies.

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8369976 [details] [diff] [review]
patch B v1: code cleanup, rename JSScript::originalFunction to donorFunction

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

I don't quite get the donor terminology. What's the thing that was given up?

Comment 14

[Shu-yu Guo [:shu]]

The patches look good to me, naming notwithstanding.

A medium term solution is to look into only cloning the typesets according to callsite location instead of the entire script. Might be much harder than it seems, though...

Comment 15

[Shu-yu Guo [:shu]]

And good catch and noticing that this is a callsite cloning bug.

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

shu: the function donated its source text and lexical context to the clone?

I'm open to suggestions for something else; I spent a little while looking at a thesaurus and didn't find anything that struck me as much better.

The important thing is that I want there to be some terminological distinction between the case where one assumes the receiver is a clone ("getDonor", which can never be me), versus the case where one is ignorant of whether the receiver is a clone or not ("getOriginal", which may be me if I am an original).

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 8369978 [details] [diff] [review]
patch T v1: regression test for the bug

Hmm, I think this regression test as written will only work on builds that have --enable-more-deterministic turned on in their configure settings.  Without that flag, we don't have a guarantee that toSource will be invoked in either case.

I'll revise the test so that instead of asserting that toSource is always called in all context, instead I'll assert that map_toSource_called if and only if mapPar_toSource_called.

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

try run:

  https://tbpl.mozilla.org/?tree=Try&rev=c01931318110

Comment 19

[Felix S. Klock II [:pnkfelix, :fklock]]

pushed to inbound:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/414676ce184c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6df8e0c99c78
   https://hg.mozilla.org/integration/mozilla-inbound/rev/24ad87cccbe5

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/414676ce184c
https://hg.mozilla.org/mozilla-central/rev/6df8e0c99c78
https://hg.mozilla.org/mozilla-central/rev/24ad87cccbe5

Comment 21

[Shu-yu Guo [:shu]]

(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #16)
> shu: the function donated its source text and lexical context to the clone?
> 
> I'm open to suggestions for something else; I spent a little while looking
> at a thesaurus and didn't find anything that struck me as much better.
> 
> The important thing is that I want there to be some terminological
> distinction between the case where one assumes the receiver is a clone
> ("getDonor", which can never be me), versus the case where one is ignorant
> of whether the receiver is a clone or not ("getOriginal", which may be me if
> I am an original).

Okay, I see where you're coming from and the dichotomy is useful, though the 'donor' terminology is still unintuitive to me.

Comment 22

[Shu-yu Guo [:shu]]

Also Felix, I would've preferred to r+ the renaming patch before you pushed it. Maybe I used the word "notwithstanding" incorrectly. I meant something like: I'm fine with patches A and T, let's discuss B.

Comment 23

[Felix S. Klock II [:pnkfelix, :fklock]]

shu: Oh, I'm sorry, I misinterpreted comment 14.

So lets decide on what to do here.

* I could revert patch B entirely, which would remove the dichotomy entirely, or

* We could bikeshed a better name than "donor", or

* We could use the word "Original" in both cases, and find some other way to encode the dichotomy.  E.g. instead of |donorFunction|, I could rename that method that could return null to |originalFunctionIfCloned| (and leave the current |originalFunction| that always returns a usable pointer alone).


Thoughts?

Comment 24

[Shu-yu Guo [:shu]]

(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #23)
> shu: Oh, I'm sorry, I misinterpreted comment 14.
> 
> So lets decide on what to do here.
> 
> * I could revert patch B entirely, which would remove the dichotomy
> entirely, or
> 
> * We could bikeshed a better name than "donor", or
> 
> * We could use the word "Original" in both cases, and find some other way to
> encode the dichotomy.  E.g. instead of |donorFunction|, I could rename that
> method that could return null to |originalFunctionIfCloned| (and leave the
> current |originalFunction| that always returns a usable pointer alone).
> 
> 
> Thoughts?

Nah, no need to bikeshed more. I'm fine with the name "donor" now that you explained the intention.