Closed Bug 863505 Opened 7 years ago Closed 7 years ago

Self-hosted code should never emit unbound name ops

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: shu, Assigned: shu)

Details

Attachments

(1 file, 1 obsolete file)

Currently self-hosted code converts GNAMEs to INTRINSICs, but there are corner cases where some NAME ops slip through, such as when using function statements:

  function f() { if (x) { function g() { } } }

In the above code, the "x" lookup is emitted as NAME "x", even in self-hosted code.

We should disallow this.
Attached patch fix (obsolete) — Splinter Review
Assignee: general → shu
Attachment #739344 - Flags: review?(tschneidereit)
Comment on attachment 739344 [details] [diff] [review]
fix

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

Oh wow, good catch. Surprising that this wasn't uncovered earlier.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1413,5 @@
>  
>  /*
> + * Attempts to bind the name, then checks that no dynamic scope lookup ops are
> + * emitted in self-hosting mode. NAME ops do lookups off current scope chain,
> + * and we do not want to allow self-hosted to use the dynamic scope.

s/self-hosted to/self-hosted code to/

@@ +1422,5 @@
> +    if (!BindNameToSlotHelper(cx, bce, pn))
> +        return false;
> +
> +    if (bce->selfHostingMode && !pn->isBound()) {
> +        JS_NOT_REACHED("Unbound name operation in self-hosted code");

Can we print an error message here that contains information on where the error occurred? Without that, hunting for the underlying issue might potentially be quite annoying.
Attachment #739344 - Flags: review?(tschneidereit) → review+
Attached patch fix v2Splinter Review
New version that reports an error instead of asserting.
Attachment #739344 - Attachment is obsolete: true
Attachment #739829 - Flags: review+
Comment on attachment 739829 [details] [diff] [review]
fix v2

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

Nice!
https://hg.mozilla.org/mozilla-central/rev/fad77e38f79e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.