Closed Bug 515233 Opened 15 years ago Closed 15 years ago

Time to widen JSTreeContext::flags

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

At the moment, the 'flags' member of JSTreeContext is a uint16.  As a result:

1) Adding a flag for ES5 strict mode (which clearly belongs in tree context, as strict mode boundaries correspond exactly to where we create JSTreeContext structures) required first turning TCF_HAS_FUNCTION_STMT into a JSStmtInfo flag.  This is bug 514584.

2) But then there's also this:

grep -nH -e TSF_DESTRUCTURING *.cpp *.h
jsparse.cpp:3865:    ts->flags |= TSF_DESTRUCTURING;
jsparse.cpp:3867:    ts->flags &= ~TSF_DESTRUCTURING;
jsparse.cpp:5557:            ts->flags |= TSF_DESTRUCTURING;
jsparse.cpp:5559:            ts->flags &= ~TSF_DESTRUCTURING;
jsparse.cpp:6495:            ts->flags |= TSF_DESTRUCTURING;
jsparse.cpp:6497:            ts->flags &= ~TSF_DESTRUCTURING;
jsparse.cpp:8092:            if (!afterDot && !(ts->flags & TSF_DESTRUCTURING) && !tc->inStatement(STMT_WITH)) {
jsparse.cpp:8100:                   ) && !(ts->flags & TSF_DESTRUCTURING)) {
jsscan.h:333:#define TSF_DESTRUCTURING   0x8000

That is, we've put a flag about whether we're parsing a destructuring form, used only by the parser, in the token stream structure.  (This change was introduced as part of the "upvar part deux" patch.)

If the width of the field is making work and leading to, um, inspired workarounds, it's time just go ahead and spend the extra four bytes per function at compile time.
Blocks: es5strict
Pre-patch, the initial static level at which a script should be
compiled to run is passed in the upper sixteen bits of the tcflags
argument to JSCompiler::compileScript; because JSTreeContext::tcflags
is a uint16, while the tcflags argument is a uint32, we know the
argument's upper bits are free.

However, we would like to enlarge JSTreeContext::tcflags to 32 bits,
as we already have a handful of new flags that belong there.  This
patch moves the static level to its own argument, which has a default
argument.
Attachment #399304 - Flags: review?(igor)
All the bits in this uint16 field are currently in use.  Adding bits
for projects like strict mode entails relocating existing flags, which
is additional work.  Furthermore, it seems that this has already
inspired people to put flags in places they don't belong:
TSF_DESTRUCTURING is a JSTokenStream flag, but is only used by the
parser.

This patch widens the field to 32 bits, and adjusts JSFunctionBox and
a few other places to match.
Attachment #399305 - Flags: review?(igor)
The flag TSF_DESTRUCTURING is used by the parser to modify the way
variables in the target pattern get represented in the parse tree.  It
is never set or used by the tokenizer.  This patch moves it to
JSTreeContext::flags, where it belongs, now that that field is wide
enough to hold it.
Attachment #399306 - Flags: review?(igor)
I admit to chickening out, or dodging a bullet.

BTW, aren't bitfields more winning than flags, from within gdb and as good or better for the compiler? We should move to them if so.

/be
Comment on attachment 399304 [details] [diff] [review]
Make the static level its own argument to JSCompiler::compileScript.

Drive-by nit: staticLevel matches the name used elsewhere (or just level).

Not-quite-nit: why not unsigned instead of int for its type?

/be
(In reply to comment #4)
> I admit to chickening out, or dodging a bullet.

Sgt. Zim has latitude for meritorious service.

> BTW, aren't bitfields more winning than flags, from within gdb and as good or
> better for the compiler? We should move to them if so.

I was thinking about that.  As far as code generation is concerned, they're equivalent.  In the debugger, bitfields are obviously better (although using Python GDB makes this much less of a problem, because you can define helpful printers).  The sticking point for me is that bitfields are a pain to manipulate en masse, as we do with TCF_FUN_FLAGS.  In the end it seemed fine to me to leave them as bits.
Truly TCF_FUN_FLAGS wants bits not fields but we could use fields for the rest. Not a big deal. I'll probably start switching jsscope.h bits to fields as time allows soon, though.

/be
Fixed nits found by Brendan.
Attachment #399304 - Attachment is obsolete: true
Attachment #400539 - Flags: review?(igor)
Attachment #399304 - Flags: review?(igor)
Comment on attachment 399305 [details] [diff] [review]
Widen JSTreeContext::flags to 32 bits.

I assume that a grep for uint16 was thorough ;)
Attachment #399305 - Flags: review?(igor) → review+
Comment on attachment 399306 [details] [diff] [review]
Move the TSF_DESTRUCTURING flag to the tree context.

In GeneratorExpr, http://hg.mozilla.org/tracemonkey/file/467758d5e9f1/js/src/jsparse.cpp#l6647 , there are some bit manipulations to propagate sharps to/from implicit generator function. I am not shure that TSF_DESTRUCTURING in the tree context does not require extra work. If that was checked, then r+ is in due.
Attachment #399306 - Flags: review?(igor)
Comment on attachment 400539 [details] [diff] [review]
Make the static level its own argument to JSCompiler::compileScript.

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -1236,7 +1236,7 @@ obj_eval(JSContext *cx, JSObject *obj, u
> {
>     JSStackFrame *fp, *caller;
>     JSBool indirectCall;
>-    uint32 tcflags;
>+    unsigned int staticLevel;

Nit: use here and elsewhere just unsigned, not unsigned int, for less noise.
Attachment #400539 - Flags: review?(igor) → review+
(In reply to comment #9)
> (From update of attachment 399305 [details] [diff] [review])
> I assume that a grep for uint16 was thorough ;)

Yes --- I grepped {jsparse,jsemit}.{cpp,h} for uint16 and checked everything.
(In reply to comment #10)
> (From update of attachment 399306 [details] [diff] [review])
> In GeneratorExpr,
> http://hg.mozilla.org/tracemonkey/file/467758d5e9f1/js/src/jsparse.cpp#l6647 ,
> there are some bit manipulations to propagate sharps to/from implicit generator
> function. I am not shure that TSF_DESTRUCTURING in the tree context does not
> require extra work. If that was checked, then r+ is in due.

TSF_DESTRUCTURING is only set while we parse destructuring targets, which cannot legitimately contain generator expressions; they'd be caught by CheckDestructuring.  So I don't think moving TSF_DESTRUCTURING to tc->flags can affect GeneratorExpr in a significant way.
(In reply to comment #13)
> TSF_DESTRUCTURING is only set while we parse destructuring targets, which
> cannot legitimately contain generator expressions; they'd be caught by
> CheckDestructuring.

This, of course, is not true:

js> x={}
[object Object]
js> [[a for each (a in [x])][0].p] = [4]
4
js> uneval(x)
({p:4})

Let me think about this a bit...
That should be:
js> [(a for each (a in [x])).next().p] = [4]
(In reply to comment #14)
> (In reply to comment #13)
> > TSF_DESTRUCTURING is only set while we parse destructuring targets, which
> > cannot legitimately contain generator expressions; they'd be caught by
> > CheckDestructuring.
> 
> This, of course, is not true:
> 
> js> x={}
> [object Object]
> js> [[a for each (a in [x])][0].p] = [4]

No genexp there (just mid-aired) but either way, the comprehension is the left operand of [] with right operand 0, which is left operand of . so this is just a destructuring pattern.

But none of the ts->flags |= TSF_DESTRUCTURING; statements in current tip jsparse.cpp are reached.

If you put a var in front of [[a for each (a in [x])][0].p] = [4] then you will hit the ts->flags |= TSF_DESTRUCTURING statement in Variables, and as hoped, CheckDestructuring will save the day:

typein:2: SyntaxError: missing variable name:
typein:2: var [[a for each (a in [x])][0].p] = [4]
typein:2: .....^

/be
Which all goes to show that TCF_DESTRUCTURING in JSTreeContext::flags does not need to be a member of TCF_FUN_FLAGS or to be propagated to generator expression function-box tcflags.

/be
Attachment #399306 - Attachment is obsolete: true
Comment on attachment 401124 [details] [diff] [review]
Move the TSF_DESTRUCTURING flag to the tree context.

This version of the patch adds a bunch of comments.  If folks could check them for accuracy and clarity, that'd be great.
Attachment #401124 - Flags: review?(igor)
Comment on attachment 401124 [details] [diff] [review]
Move the TSF_DESTRUCTURING flag to the tree context.

Review ping?
Attachment #401124 - Flags: review?(igor) → review+
http://hg.mozilla.org/tracemonkey/rev/49553b2af70b
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: