Closed Bug 944975 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: gkw, Assigned: pnkfelix)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase)

Attachments

(3 files)

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?
Flags: needinfo?(shu)
PA was removed.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(shu)
Resolution: --- → INVALID
Array.prototype.scatterPar was not removed.
Status: RESOLVED → REOPENED
Flags: needinfo?(shu)
Resolution: INVALID → ---
(In reply to Jesse Ruderman from comment #2)
> Array.prototype.scatterPar was not removed.

Doh, I thought this was PA.p.scatterPar
Flags: needinfo?(shu)
Flags: needinfo?(shu)
I am looking at this.  I was not able to reproduce on my current head:

  changeset:   166605:e020404995da
  user:        Carsten "Tomcat" Book <cbook@mozilla.com>
  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.)
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 <cbook@mozilla.com>
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.)
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
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 });
(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.)
Assignee: nobody → pnkfelix
Attachment #8369973 - Flags: review?(shu)
(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."
(patch "T" for "test"; this can be applied independently of whether patches A+B are applied.)
Attachment #8369978 - Flags: review?(shu)
Attachment #8369976 - Flags: review?(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.
Attachment #8369973 - Flags: review?(shu) → review+
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?
Attachment #8369976 - Flags: review?(shu)
Attachment #8369978 - Flags: review?(shu) → review+
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...
Flags: needinfo?(shu)
And good catch and noticing that this is a callsite cloning bug.
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 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.
https://hg.mozilla.org/mozilla-central/rev/414676ce184c
https://hg.mozilla.org/mozilla-central/rev/6df8e0c99c78
https://hg.mozilla.org/mozilla-central/rev/24ad87cccbe5
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(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.
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(shu)
You need to log in before you can comment on or make changes to this bug.