485164 - emitter: improper checks for literals with sharp defs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

To signalize the presence of the sharp definition for the array literal the parser put TOK_SHARPDEF node as the first child on the children list for the array. Moreover, this sharp node has no children as the node with a child indicates the sharp definition for the child. But the emitter fails to check for this second condition as the following example demonstrates:

js> uneval([#1=([]), #1#])
#1=[#1#]

Here the expected result should be [#1=[], #1#].

I am surprised that fuzzing tools have not discovered this yet. Or have they?

The bug is discovered during checking the assert from the bug 484769.

Comment 1

[Jesse Ruderman]

> I am surprised that fuzzing tools have not discovered this yet.

jsfunfuzz testUnevalString ignores stuff with # because of bug 328745.

Comment 2

[Igor Bukanov]

There is another issue with sharp variables which is a regression from 1.9.0. Consider the following script:

print(uneval([0, #1=[], #1#]));

With js shell from 1.9.0 it prints: [0, #1=[], #1#]
With TM tip the result is: Error: invalid sharp variable use #1# 

The reason for that is that TM tip and 1.9.1 uses the newly introduced JSOP_NEWARRAY to create [0, #1=[], #1#]. But that does not increase the sharp depth for the frame. And without that the JSOP_ENDINIT that finishing #1=[] would release the sharp container for the frame so the later #1# cannot find it.

I think a simplest fix for this would be to disable using JSOP_NEWARRAY for a script if it ever uses sharp variables. Ideally the restriction should be per expression statement, but AFAICS there is no easy way to set such flag on the expression.

I nominate it for 1.9.1 as this is a regression from 1.9.0.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> I think a simplest fix for this would be to disable using JSOP_NEWARRAY for a
> script if it ever uses sharp variables.

It is interesting that it would make 1.9.1 to work exactly as 1.9.0 even in presence of eval calls that defines and use the sharps. Consider the following sessions from 1.9.0 shell:

js> uneval([ 0, #1=[], eval('#2=[]'), #1#, #2# ]); 
[0, #1=[], #2=[], #1#, #2#]
js> uneval([ 0, eval('#2=[]'), #1=[], #1#, #2# ]);  
typein:3: Error: invalid sharp variable use #2#

Note the error in the second case when the first sharp definition comes from the eval. This discrepancy comes from the fact that the main frame shares the sharp object with the eval frame if the object is already created but the object is not copied from the eval back into the main frame if the eval frame has to create it.

A consequence of this "feature" is that on 1.9.0 sharp variables can not come only from the eval like in:

js> uneval([ eval('#1=[]'), eval('#1#')]);
typein:5: Error: invalid sharp variable use #1#

And since this does not work in 1.9.0 so 1.9.1 does not need to care about eval-only sharps which would make it impossible to decide at the compile time to optimize them away.

It also suggests to stop propagating sharps across eval frames for simple and easy to understand behavior.

Comment 4

[Igor Bukanov]

Attachment: v1

The patch changes TOK_SHARPDEF parsing to always use a node with a child to represent this operation. That is, the parser no longer marks array and object literals with a special childless sharp def node. Instead the emitter checks that the kid of the sharp is array or object literal and alters literals code generation accordingly.

This way all those checks for the special sharp marker node in array can be removed and in future the code can trust that pn->pn_count represents the number of the elements in the array.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 370410 [details] [diff] [review]
v1

I find it terribly unfortunate that we have to support an interpreter that doesn't support sharp variables. I feel like the stacked #ifs here really interrupt the normal control flow, but I don't see any way around it.

Comment 6

[Brendan Eich [:brendan]]

Does the engine build if you turn off sharp vars (e.g., to build JSVERSION_ECMA_3 or JSVERSION_ECMA_3_TEST?

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Does the engine build if you turn off sharp vars (e.g., to build
> JSVERSION_ECMA_3 or JSVERSION_ECMA_3_TEST?

Yes, if one also patches jsinterp.cpp to include the missing L_JSOP_DEFSHARP and L_JSOP_USESHARP labels when !JS_HAS_SHARP_VARS. But this is from the bug 485535. I may include it with this patch.

Comment 8

[Igor Bukanov]

(In reply to comment #5)
> (From update of attachment 370410 [details] [diff] [review])
> I find it terribly unfortunate that we have to support an interpreter that
> doesn't support sharp variables.

IMO it is unfortunate that SM is stuck with a feature that nobody supports (and will not support unless I miss something) and that can be replaced either with let expressions or even with a pure ES3 code using function closures.

Comment 9

[Igor Bukanov]

In fact I would even suggest to warn about using the sharps as a deprecated feature.

Comment 10

[Igor Bukanov]

Attachment: v2

The new patch is v1 plus an extra fix to jsinterp.cpp to make sure that the threaded interpreter compiles when !JS_HAS_SHARP_VARS.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 370410 [details] [diff] [review] [details])
> > I find it terribly unfortunate that we have to support an interpreter that
> > doesn't support sharp variables.
> 
> IMO it is unfortunate that SM is stuck with a feature that nobody supports (and
> will not support unless I miss something)

You mean no other JS impls, because it won't get standardized, right?

We support our extensions till we kill 'em, if we kill them. Sharp vars' time has not yet come.

> and that can be replaced either with
> let expressions or even with a pure ES3 code using function closures.

That is another project of dubious priority given the sunk costs. Agree we are spending time on sharp vars. As much as reimplementing would?

And let expressions are not in good shape to be standardized as is (maybe we can rescue them by tweaking the body grammar to require parens if the expr is not super-primary). So more cost of trying to lead the standard and market.

Our extensions go back years, even a decade. Without a crystal ball, all I can do is channel Edith Piaf ("Non, je ne regrette rien"). Well, almost nothing. ;-)

(In reply to comment #9)
> In fact I would even suggest to warn about using the sharps as a deprecated
> feature.

Not until we have a replacement to steer people toward. Otherwise it's pointless nagging a la our old 'with' deprecation warning, which we removed. Sharp vars are used in Firefox front end code, and elsewhere.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> You mean no other JS impls, because it won't get standardized, right?

Right.

> We support our extensions till we kill 'em, if we kill them. Sharp vars' time
> has not yet come.
...

> Our extensions go back years, even a decade. Without a crystal ball, all I can
> do is channel Edith Piaf ("Non, je ne regrette rien").

I have not said that sharps were a bad feature! They are reasonable way to encode the cycles in the object graph. But since they have not inspired anybody to follow the suite, it is better to retire them to avoid spending efforts to support them for the years to come. I will file bug about changing the graph serializer for 1.9.2 to use a function block instead of sharps and to warn about the usage of sharps.

Comment 13

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/382c1065eab7

Comment 14

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> Sharp vars are used in Firefox front end code

Not true, the only places in Mozilla that use them are JS tests now:

http://mxr.mozilla.org/mozilla-central/search?string=\%23[0-9]\%23&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
http://mxr.mozilla.org/mozilla-central/search?string=\%23[0-9][0-9]\%23&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

The last time I saw sharp variables anywhere in Mozilla (before these searches) was back in 2006, and I'm pretty sure I replaced all such uses with standard code that didn't use undocumented extensions as a readability improvement.  Given their obscurity, I would be astounded if anyone ever adds another sharp variable to non-JS test code in Mozilla.

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/382c1065eab7

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8418dc789c7f