848743 - BaselineCompiler: Differential Testing: Setter not called with baseline enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase shows different behavior with options --ion-eager vs. --ion-eager --no-baseline on ionmonkey revision 8565e1fcdf91 (baseline branch):


var gTestcases = new Array();
var gTc = gTestcases.length;
var setterCalled = false;
function TestCase() {
  gTestcases[gTc++] = this;
}
for(var i = 0; i < 13; ++i) {
  var testcase = new TestCase();
}
Array.prototype.__defineSetter__(32, function() { setterCalled = true; });
for(var i = 0; i < 20; ++i) {
  var testcase = new TestCase();
}
setterCalled ? print("PASSED") : print("FAILED");


$ debug64/js --ion-eager test.js
FAILED

$ debug64/js --ion-eager --no-baseline test.js
PASSED

Comment 1

[Jan de Mooij [:jandem]]

Great testcase. The problem is that the SetElem_DenseAdd stub has to check for indexed properties on the prototype before adding an element.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

The teleporting optimization does not work for indexes, so this patch guards on all shapes on the proto chain. Also fixes some very noisy Clang warnings (with this patch there are no new warnings compared to inbound).

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 727191 [details] [diff] [review]
Patch

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

Looks good.

::: js/src/ion/BaselineIC.cpp
@@ +4340,5 @@
>      Register walker = regs.takeAny();
>      Register scratch = regs.takeAny();
>  
> +    // Use a local to silence Clang tautological-compare warning if NumHops is 0.
> +    size_t numHops = NumHops;

Would it be possible to make this 'const' and still eliminate the warnings?

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/def96e89be7e

(In reply to Kannan Vijayan [:djvj] from comment #3)
> 
> Would it be possible to make this 'const' and still eliminate the warnings?

I tried it but if I add 'const' Clang starts to complain again.