231518 - Decompiler must quote keywords and non-identifier property names

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Adrian Klein and Martin Honnen have reported several cases where the decompiler
does not quote reserved identifiers used to name properties.  The first case was

    function do_something() {
      var obj = new Object();

      obj['name']      = 'Just a name';
      obj['if']        = false;
      obj['some text'] = 'correct';
    }

which decompiles with obj['if'] rendered as the illegal obj.if.  Martin Honnen's
followup pointed out a case with object literals:

    function f() { return {'if': "then"}; }

I already fixed the first case, but Martin's post turned over a virtual rock,
under which crawled a number of bugs, including footprint bloat in Decompile
dating from the introduction of ECMA Edition 3 Unicode identifiers.

Patch soon.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

- Save footprint by eliminating old (pre-ECMA-Edition-3) IsASCIIIDentifier
  static helper and its calls.	This was used to decompile o.p\uBC19rop as
  o['p\uBC19rop'] before the Edition 3 support for Unicode identifiers went in.

  But when Unicode id support was done, IsASCIIIdentifier was not removed.

  Now, we need only test ATOM_KEYWORD in order to decide whether bytecode such
  as JSOP_GETPROP<obj, 'if'>, which comes from an input expression of the form
  obj["if"], should be decompiled as obj['if'].  If the property identifier is
  not a reserved id, we can decompile using dot notation -- provided we call
  QuoteString(..., 0) to escape any non-ASCII characters in the id.

  The for..in Decompile cases had redundant QuoteString(..., 0) calls before
  goto do_for*in* jumps, which could be commoned at the jump target(s).  So I
  consolidated all such calls.	See the code that goes to do_fornameinloop, and

  code below that label that depends on lval, atom, xval, and rval being set
  correctly for the current JSOP_FOR* bytecode.

- Share a bunch of similar Sprint calls using a |const char *fmt| local.  Also
  fused the JSOP_SETPROP += vs. = (SRC_ASSIGNOP vs. no such note) cases via %s=

  and a conditionally empty assignment-operator-string Sprint argument.

- Fix JSOP_ENUMELEM (again), it was not conspiring with JSOP_FORELEM correctly
  at all.  It ends with a goto do_forinbody, which code depends on the |tail|
  local variable, but |tail| was not set by JSOP_ENUMELEM -- instead, it was
  hacked in JSOP_FORELEM, with a random 1 subtracted from its value reckoned
  from the ifeq at the top of the (for example) |for (o[p] in q);| loop.  But
  the bytecodes needed to execute o[p] come between the forelem;ifeq and the
  enumelem ops, and these indexing expression bytecodes have arbitrary length
  in general, so there's no way JSOP_FORELEM can compute |tail|.

  The fix is to propagate the tail (loop-closing goto) bytecode pointer via a
  forelem_tail local, analogous to forelem_done, from JSOP_FORELEM's case in
  Decompile to JSOP_ENUMELM's case.  The latter case can then subtract the
  current pc to compute the prper |tail| value, before the goto do_forinbody.

- Clean up some unnecessarily nested assignments in conditions, etc.

/be

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 139454 [details] [diff] [review]
proposed fix

Passes the JS testsuite.

/be

Comment 3

[Brendan Eich [:brendan]]

One tiny change, here's the diff-diff (old patch on left, new on right):

7c7
< +++ jsopcode.c        20 Jan 2004 00:06:46 -0000
---
> +++ jsopcode.c        22 Jan 2004 23:48:02 -0000
167c167
< +                    js_printf(jp, "%s%s", *lval ? "." : "", xval);
---
> +                    js_printf(jp, *lval ? ".%s" : "%s", xval);

To illuminate this further, the code here is part of the do_forinloop jazz that
wants to handle for (x in o), for (x.y in o), and for (x[z] in o), where y may
be a keyword (in which case we want for (x['y'] in o) as output).

So lval, atom, xval, and rval are all set, and if (atom), we have for (x in o)
or for (x.y in o) where y is known not to be a keyword, and lval == "" tells
whethe we have for (x in o) or for (x.y in o); else if (xval), we have the
indexed case: for (x[z] in o).

The above tweak just reduces by one the somewhat carefree %s format mania in the
decompiler.

Hope this is all clear.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 139454 [details] [diff] [review]
proposed fix

We clearly need some of these bits to be added to the test suite.  And for my
birthday, I would like a copy of O'Reilly's "Mastering the Spidermonkey
Decompiler", thanks.

The duplication of the 

ATOM_KEYWORD ? (quote, o[p]) : (0, o.p);
QuoteString(...);

logic makes me wonder if we should have a macro to unify, but it's not a
showstopper.

r=shaver.

Comment 5

[Brendan Eich [:brendan]]

Attachment: like this?

I'll check this in after a handshake with shaver, here or in irc.

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 139734 [details] [diff] [review]
like this?

money. r=shaver

Comment 7

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: followup fix to problem reported by timeless

I'd like to get this in straight away.

/be

Comment 9

[Brendan Eich [:brendan]]

Generalizing bug as I should have done originally.

/be

Comment 10

[Brendan Eich [:brendan]]

Followup fix is in.

/be

Comment 11

[timeless]

*** Bug 232736 has been marked as a duplicate of this bug. ***

Comment 12

[Georg Maaß]

Does your fix also catch this variation?


js> a = new RegExp;
//
js> b=eval(uneval(a));
js> b.toSource();
3: TypeError: b has no properties
js> 

Comment 13

[timeless]

In short, "no":
js> build()
built on Jan 25 2004 at 09:14:22
js> a = new RegExp;
/(?:)/
js>  b=eval(uneval(a));
/(?:)/
js>  b.toSource();
/(?:)/

That was fixed by bug 225550 a while ago

Comment 14

[Brendan Eich [:brendan]]

Re: comment 12: Please don't spam closed bugs with clearly unrelated issues.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-231518.js

I'll write to Adrian and Martin.

Comment 16

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-231518.js checked in.

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed.