482958 - TM: We keep deep-bailing from non-_FAIL builtin functions

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

We have had several cases of this now.  I see two ways to solve it:

1. Make every non-trivial builtin function _FAIL and get rid of _RETRY altogether.

2. Annotate all non-_FAIL builtins with a new annotation, JS_STAYS_ON_TRACE, and change jsstack.js to detect when one of them does anything that could end up deep-bailing (like calling a non-JS_STAYS_ON_TRACE function).

At the moment I favor hacking together option 1 to see if it regresses perf.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Approach 1

Here is a patch for approach 1.

Good: bench.sh showed no change in speed.

Bad: Note that diffstat shows a net increase in code.

Bad: The patch does not fix holes where we emit calls to specific builtins via lir->insCall().  In those cases the _FAIL/bailExit machinery still isn't being used, and some of them can reach error handlers (especially OOM, see js_NewInstance).

I need to sleep on it.  I don't think I like _FAIL this much, but we need to kill this class of bugs.

Comment 2

[Brendan Eich [:brendan]]

Alternative 2 keeps RETRY so we can have faster calls to builtins whose only way to "fail" is OOM on trace, without unwanted effects to clean up. Looks like ~22 so far in js*.cpp -- more to come? Anyway, sleep well!

/be

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.