636224 - rm cx->interpLevel

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

For a rainy day:

cx->interpLevel has only one use (suppressing errors in the scanner) and we have to inc/dec it for every RunScript call, giving its own stack guard etc.  Not the hottest path, but less state/code is better and cx->currentSegment ==/!= NULL should suffice.

Comment 1

[Brendan Eich [:brendan]]

Wow, ancient code. ISTM the scanner could just test JS_IsRunning instead.

/be

Comment 2

[Nicholas Nethercote [inactive]]

It's used to "Suppress any compile-time errors that don't occur at the top level."
Why do we do that?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> It's used to "Suppress any compile-time errors that don't occur at the top
> level."
> Why do we do that?

The engine is old, parts pre-date exceptions (ES3). Originally errors were reported eagerly and scripts fail-stopped at first report.

  eval("oops=42; eval('oops + `')")

would report the bad character (`) and fail inside the inner eval; failure then propagated inevitably.

With exceptions (catchable via try-catch), the eager reporting had to be deferred by wrapping the report up as an exception:

  eval("oops=42; try{eval('oops + `')}catch(e){}")

In general, any intervening interpreter activation could have an exception handler active (not only via try-catch, either -- the JS API allows recovery from inner failure by native code, no JS compiler or try-catch syntax needed).

Errors-as-exceptions was the phrase used to describe the implicit conversion of seemingly (and theretofore) eager error reports into pending exceptions. Fine folks who did this work are long gone, and I wasn't in on all the code review, so I'm reconstructing partly from memory, partly from reading modern code.

Errors-as-exceptions meant that only if an uncaught exception propagated with failure return codes up to the outermost API entry point (see LAST_FRAME_CHECKS in jsapi.cpp) would the pending exception be turned back into a report, to avoid silent failure with lost exception, thus keeping API compatibility where it still made sense to automate error reporting via the JS_SetERrorReporter-configured callback.

Reports via such a callback go to stderr or an error console, and cannot be recalled.

So this interpLevel check dates from the errors-as-exceptions conversion days, but I don't believe it is necessary. The code already does convert errors to exceptions:

    /*
     * Try to raise an exception only if there isn't one already set --
     * otherwise the exception will describe the last compile-time error,
     * which is likely spurious.
     */
    if (!(flags & TSF_ERROR)) {
        if (js_ErrorToException(cx, message, &report, NULL, NULL))
            onError = NULL;
    }

    /*
     * Suppress any compile-time errors that don't occur at the top level.
     * This may still fail, as interplevel may be zero in contexts where we
     * don't really want to call the error reporter, as when js is called
     * by other code which could catch the error.
     */
    if (cx->interpLevel != 0 && !JSREPORT_IS_WARNING(flags))
        onError = NULL;

    if (onError) {
        JSDebugErrorHook hook = cx->debugHooks->debugErrorHook;

        /*
         * If debugErrorHook is present then we give it a chance to veto
         * sending the error on to the regular error reporter.
         */
        if (hook && !hook(cx, message, &report,
                          cx->debugHooks->debugErrorHookData)) {
            onError = NULL;
        }
    }
    if (onError)
        (*onError)(cx, message, &report);

The comment for the interpLevel test alludes to failure-to-suppress (presumably due to multiple JSContexts), so this code was never completely correct. But the previos paragraph, which calls js_ErrorToException, should have deferred the report unless TSF_ERROR were set.

A bit further, on the way out we have [*]:

    if (!JSREPORT_IS_WARNING(flags)) {
        /* Set the error flag to suppress spurious reports. */
        flags |= TSF_ERROR;
    }

And the only other place that sets TSF_ERROR is getTokenInternal on a lexical grammar error (such as that ` char). But that TSF_ERROR set must have followed a report, which called js_ErrorToException, e.g.:

      default:
        ReportCompileErrorNumber(cx, this, NULL, JSREPORT_ERROR, JSMSG_ILLEGAL_CHARACTER);
        goto error;

from getTokenInternal a bit above the out/eol_out/error labeled tail code. And this ReportCompileErrorNumber call invokes reportCompileErrorNumberVA which sets TSF_ERROR (for non-warnings), so there is no need for getTokenInternal to set it again.

So the interpLevel mess in reportCompileErrorNumberVA seems to be worried about multiple reports of non-warnings on the same ts and cx. But that implies a failure to propagate failure (fail-stop in pre-ES3 days; pending exception for syntax errors with ES3-era try/catch and after).

That is, this interpLevel and TSF_ERROR code is covering up spurious error reporting bugs. (We've never done error recovery in our compiler).

This says we should rip out interpLevel completely and set TSF_ERROR in exactly one place: reportCompileErrorNumberVA for non-warnings (as shown above at [*]; this code is fine), and assert that TSF_ERROR is not set before calling js_ErrorToException from reportCompileErrorNumberVA, and then test hard.

/be

Comment 4

[Luke Wagner [:luke]]

Hah, get this: TokenStream::reportCompileErrorNumberVA has a formal arg 'flags' which shadows TokenStream::flags.  Thus, the (flags & TSF_ERROR) is bogus and flags |= TSF_ERROR isn't actually setting a bit.

I was looking at uses of TSF_ERROR and there only seems to be one:

      after_args:
        if (state == BAD_FORMAL && !ts.isError()) {

in jsfun.cpp in the Function constructor.  I don't really understand this stuff, but seems like maybe we can kill TSF_ERROR too?

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> Hah, get this: TokenStream::reportCompileErrorNumberVA has a formal arg 'flags'
> which shadows TokenStream::flags. 

Oh geez, we really need -Wshadow.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> Hah, get this: TokenStream::reportCompileErrorNumberVA has a formal arg 'flags'
> which shadows TokenStream::flags.  Thus, the (flags & TSF_ERROR) is bogus and
> flags |= TSF_ERROR isn't actually setting a bit.

FWIW, the formal arg 'flags' is meant to be composed from these values:

/*            
 * JSErrorReport flag values.  These may be freely composed.
 */           
#define JSREPORT_ERROR      0x0     /* pseudo-flag for default case */
#define JSREPORT_WARNING    0x1     /* reported via JS_ReportWarning */
#define JSREPORT_EXCEPTION  0x2     /* exception was thrown */
#define JSREPORT_STRICT     0x4     /* error or warning due to strict option */

Comment 7

[Luke Wagner [:luke]]

Attachment: kill kill

This patch kills a goodly amount of code involving interpLevel and TOK_ERROR and tidies up Function().  It also plugs yet another one of these "holding a pointer into the chars of an unrooted string" holes.

Comment 8

[Luke Wagner [:luke]]

Passes trace/ref tests, try'ing now.

Comment 9

[Luke Wagner [:luke]]

Oops, ignore the change to manifest.py, that was a temp hack so jstests.py works.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 521313 [details] [diff] [review]
kill kill

> static JSBool
> Function(JSContext *cx, uintN argc, Value *vp)
> {
>-    JSObject *obj = NewFunction(cx, NULL);
>-    if (!obj)
>-        return JS_FALSE;
>-
>-    /* N.B. overwriting callee with return value */
>-    JSObject *parent = vp[0].toObject().getParent();
>-    vp[0].setObject(*obj);
>+    JS::Anchor<JSObject *> obj(NewFunction(cx, NULL));
>+    if (!obj.get())
>+        return false;

Function() still returns a JSBool, so this should be JS_FALSE.

>     if (!fun)
>-        return JS_FALSE;
>+        return false;

Likewise.  And a couple of other cases below.

The rest seems fine but a lot of these changes are over my head so I'll ask
brendan for a second opinion.

Comment 11

[Nicholas Nethercote [inactive]]

Oh, I see this:

cc1plus: warnings being treated as errors
../jsfun.cpp: In function ‘JSBool Function(JSContext*, uintN, js::Value*)’:
../jsfun.cpp:2623: error: ‘strAnchor.JS::Anchor<JSString*>::hold’ may be used uninitialized in this function

That'll need to be fixed.  Looks like a (strange) false positive;  the calls to Anchor::get() are dominated by calls to Anchor::set().  In fact, there's already this comment:

        strAnchor.set(cx->runtime->emptyString);  /* set to avoid warning */

Weird.


(In reply to comment #9)
> Oops, ignore the change to manifest.py, that was a temp hack so jstests.py
> works.

I too have that hack.  Any idea what the real problem is?

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #10)
> Function() still returns a JSBool, so this should be JS_FALSE.

This is normal SM style when the return type itself can't be changed to 'bool' b/c of signature restrictions.  The conversion is well-defined (so no correctness concern) and there is no perf lost.

Comment 13

[Luke Wagner [:luke]]

(In reply to comment #11)
> Oh, I see this:
> 
> cc1plus: warnings being treated as errors
> ../jsfun.cpp: In function ‘JSBool Function(JSContext*, uintN, js::Value*)’:
> ../jsfun.cpp:2623: error: ‘strAnchor.JS::Anchor<JSString*>::hold’ may be used
> uninitialized in this function

Thanks, I forgot to compile in opt.

> I too have that hack.  Any idea what the real problem is?

No... I was under the impression someone was on it... maybe we are all under that impression...

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #4)
> I was looking at uses of TSF_ERROR and there only seems to be one:
> 
>       after_args:
>         if (state == BAD_FORMAL && !ts.isError()) {
> 
> in jsfun.cpp in the Function constructor.  I don't really understand this
> stuff, but seems like maybe we can kill TSF_ERROR too?

No, this is trying to avoid a double report. Second one would be spurious.

There are similar cases elsewhere that test for TOK_ERROR to avoid re-reporting.

/be

/be

Comment 15

[Luke Wagner [:luke]]

Attachment: njn's comments addressed, thinko fix

(In reply to comment #14)
> > in jsfun.cpp in the Function constructor.  I don't really understand this
> > stuff, but seems like maybe we can kill TSF_ERROR too?
> 
> No, this is trying to avoid a double report. Second one would be spurious.
> 
> There are similar cases elsewhere that test for TOK_ERROR to avoid
> re-reporting.

Indeed, the patch uses the return token to avoid said double-reporting, thereby allowing TSF_ERROR to be removed.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 521861 [details] [diff] [review]
njn's comments addressed, thinko fix

>+OnBadFormal(JSContext *cx, TokenKind token)

Nit: canonical name is tt not token. Dates from TokenType daze, tk not really an improvement. Consistency with tradition trumps consistency in forming acronyms and hacker contractions?

>+{
>+    if (token != TOK_EOF)
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_FORMAL);

As noted (the above code is from the "kill kill" patch I was commenting on), this will double-report if token is TOK_ERROR. Your latest patch, which I'm reviewing here, does fix the bug, but without a test. Should be able to write one, e.g.

  Function("a, `", "");

or something like that.

Looks great otherwise. I'm not your manifest.py reviewer, though. r=me with the above covered by a test, and jorendorff's or dmandelin's Python-fu.

/be

Comment 17

[Luke Wagner [:luke]]

(In reply to comment #16)
> Looks great otherwise. I'm not your manifest.py reviewer, though. r=me with the
> above covered by a test, and jorendorff's or dmandelin's Python-fu.

Oh, hah, that's my temp workaround for bug 644697; won't be landing that :)

Comment 18

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/4629abbde2f7

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/4629abbde2f7