1131988 - jstests builtin-no-construct.js and new-with-non-constructor.js fails with "--ion-eager --ion-offthread-compile=off" due to inconsistent error message.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Running jstests.py with --tbpl flag hits Assertion failure in following files:
  ecma_5/misc/new-with-non-constructor.js
  ecma_5/Function/builtin-no-construct.js
with following options:
  --ion-eager --ion-offthread-compile=off
  --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --no-sse3 --no-threads

In ecma_5/misc/new-with-non-constructor.js, exception with message "undefined is not a function" is thrown for `proxiedBuiltin`, where we expect "thing is not a constructor".
https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_5/misc/new-with-non-constructor.js

In ecma_5/Function/builtin-no-construct.js, exception with message "function atan() {\n    [native code]\n} is not a constructor" is thrown for `Math.atan`, where we expecte "method is not a constructor".
https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_5/Function/builtin-no-construct.js

Comment 1

[Tooru Fujisawa [:arai]]

1. ecma_5/misc/new-with-non-constructor.js

https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_5/misc/new-with-non-constructor.js

Inside `ReportIsNotFunction` for `new thing()`, `FindStartPC` fails because of `iter.isIon()`, and `ValueToSource` is called in `DecompileValueGenerator`.

https://hg.mozilla.org/mozilla-central/file/2cb22c058add/js/src/jsopcode.cpp#l1852
>          fallback = ValueToSource(cx, v);

Then, to get `toSource` property from the Proxy object, it tries to call `getPropertyDescriptor` trap, but it's is undefined, and `ReportIsNotFunction` is called again in `Trap1`.

https://hg.mozilla.org/mozilla-central/file/2cb22c058add/js/src/proxy/ScriptedIndirectProxyHandler.cpp#l177
>    return GetFundamentalTrap(cx, handler, cx->names().getPropertyDescriptor, &fval) &&
>           Trap1(cx, handler, fval, id, &value) &&


2. ecma_5/Function/builtin-no-construct.js

https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_5/Function/builtin-no-construct.js

Almost same as new-with-non-constructor.js, `FindStartPC` fails because of `iter.isIon()`, and `ValueToSource` is called in `DecompileValueGenerator`.


Are they a kind of test bug, or we shouldn't run them with --tbpl?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> Are they a kind of test bug, or we shouldn't run them with --tbpl?

We should fix these tests to accept both errors, I think. Different error messages in Ion is a known issue; DecompileValueGenerator is an heuristic.

Having all tests pass with --tbpl without false positives would be great.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> DecompileValueGenerator is an heuristic.

...which is the real issue here, if you're feeling ambitious.  :-)

It seems to me that, as long as we're not folding code together such that a particular opcode or JIT instruction corresponds to multiple locations, it should be possible to re-determine the exact producing expression by recompiling an AST, re-emitting bytecode, then determining correspondence between reality and its reconstruction.  Then we could have deterministic error messages.

But of course such is a fair chunk of work, perhaps finicky work, so unsurprisingly it (or some other fix for the ultimate problem) hasn't happened yet.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Fix function/constructor call error tests to work in Ion execution.

(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Tooru Fujisawa [:arai] from comment #1)
> > Are they a kind of test bug, or we shouldn't run them with --tbpl?
> 
> We should fix these tests to accept both errors, I think. Different error
> messages in Ion is a known issue; DecompileValueGenerator is an heuristic.
> 
> Having all tests pass with --tbpl without false positives would be great.

Okay, I fixed those tests for now.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> (In reply to Jan de Mooij [:jandem] from comment #2)
> > DecompileValueGenerator is an heuristic.
> 
> ...which is the real issue here, if you're feeling ambitious.  :-)
> 
> It seems to me that, as long as we're not folding code together such that a
> particular opcode or JIT instruction corresponds to multiple locations, it
> should be possible to re-determine the exact producing expression by
> recompiling an AST, re-emitting bytecode, then determining correspondence
> between reality and its reconstruction.  Then we could have deterministic
> error messages.
> 
> But of course such is a fair chunk of work, perhaps finicky work, so
> unsurprisingly it (or some other fix for the ultimate problem) hasn't
> happened yet.

Indeed. But now this is blocking bug 1101662 (sorry, forgot to fill the field), and it would be better to make jstests runnable in try server before that, to avoid overlooking regressions in shell-only tests.

Comment 5

[Tooru Fujisawa [:arai]]

> to avoid overlooking regressions in shell-only tests.

In addition to that, GC-related or JIT-related regressions, which does not happens in jsreftest.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8563571 [details] [diff] [review]
Fix function/constructor call error tests to work in Ion execution.

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

Thanks!

Comment 7

[Tooru Fujisawa [:arai]]

Thank you! :D
https://hg.mozilla.org/integration/mozilla-inbound/rev/f395713a1178

Comment 8

[Carsten Book [:Tomcat]]

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