Closed Bug 460865 Opened 11 years ago Closed 11 years ago

Read barrier for cx->fp

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 4 obsolete files)

This is the first step toward calling arbitrary native code on trace.

The tracer needs a chance to reconstruct the JS stack (making a note to bail off any currently executing traces) when a native tries to access cx->fp.
Can we assume that if a native needs cx->fp in any given call, it will need cx->fp in all calls?

If so, is the idea to reconstruct frames but resume the trace? That would leave the trace's loop exit finding frames at cx->fp above the entry frame. How could we detect that and avoid re-reconstruction?

/be
(In reply to comment #1)
> Can we assume that if a native needs cx->fp in any given call, it will need
> cx->fp in all calls?

That's not the case if the native is simply using cx->fp in its error reporting, which I think is a common pattern.
(In reply to comment #2)
> (In reply to comment #1)
> > Can we assume that if a native needs cx->fp in any given call, it will need
> > cx->fp in all calls?
> 
> That's not the case if the native is simply using cx->fp in its error
> reporting, which I think is a common pattern.

We could arrange for any error-as-exception to abort the trace, and therefore always reconstruct frames from js_ErrorToException/InitExnPrivate -- of course provided that js_ExecuteTree can detect that the non-entry frames have already been reconstructed, and avoid re-reconstructing 'em disastrously.

So the two tricks seem to be ensuring cx->fp-dependency trace recording/execution invariance, and detecting early reconstruction and coping on trace exit.

/be
(In reply to comment #1)
> Can we assume that if a native needs cx->fp in any given call, it will need
> cx->fp in all calls?

Heh - it's not true even for the hand-picked functions we've been whitelisting (like Array.p.join, bug 460336), much less arbitrary native functions.

(In reply to comment #3)
> So the two tricks seem to be ensuring cx->fp-dependency trace
> recording/execution invariance, and detecting early reconstruction and coping
> on trace exit.

I think we have to do the latter, with an extra guard when we call an unknown native.  The former seems fundamentally untrustworthy.  (Natives can already declare themselves INFALLIBLE to avoid the extra guard; we'll keep that.)
Right now, the code to regenerate the stack can fail with OOM.

It looks like I have to choose between (a) making that stuff completely infallible; and (b) breaking APIs like JS_IsConstructing, JS_IsAssigning, JS_FrameIterator, etc. that access cx->fp but are supposed to be infallible.
(c) pre-allocate stackPool space before entering a trace for the worst-case needs of any native traced from the tree being triggered plus trees it calls.

/be
We already have a similar mechanism for doubles so its easy to extend and should cost perf.
"should not cost perf", of course.

All we need to do is ensure enough stackPool space is pre-allocated. That reminds me: we need stackPool to change from a JSArenaPool to a jsval growable array that doubles and halves as needed. I'll file a bug if one isn't already on file.

/be
Blocks: deepbail
Depends on: 462084
Depends on: 461485
Attached patch v1 (obsolete) — Splinter Review
There are a few kinds of places where cx->fp is used.

1.  Inside the interpreter (functions marked JS_REQUIRES_STACK for static analysis), the stack is always complete: the fact that we're in the interpreter means we're not on trace.  Direct access to the JS stack is fine.

2.  Outside the interpreter but in js/src, we'll call js_GetTopStackFrame.  This is the new function where the JS stack reconstitution will ultimately go.

3.  Outside js/src, I'm calling JS_FrameIterator and JS_GetScriptedCaller, both of which now call js_GetTopStackFrame.  Some places (e.g. LiveConnect) still assign to cx->fp, but that's OK.

The static analysis makes sure there are no paths from 2 or 3 to 1 that don't go through js_GetTopStackFrame at some point.
Assignee: general → jorendorff
Attachment #345342 - Flags: review?(mrbkap)
Comment on attachment 345342 [details] [diff] [review]
v1

I'm withdrawing this one for now. It needs a few tweaks.
Attachment #345342 - Flags: review?(mrbkap)
Attached patch v2 (obsolete) — Splinter Review
This version puts the assertion in js_GetTopStackFrame under #ifdef DEBUG_jason.

The only test that currently triggers that assertion for me is doing
  new String("foo").match("bar")
which ends up in js_ValueToString and ultimately js_Invoke.  But this is because our existing tests don't do everything in a loop.  :)  The test in bug 460336 comment 0 triggers it too.  (The latter, at least, is a good thing.  In bug 462027 we'll replace the assertion with code that allows life to go on from there.)
Attachment #345342 - Attachment is obsolete: true
Attachment #345605 - Flags: review?(mrbkap)
Comment on attachment 345605 [details] [diff] [review]
v2

>+    *scriptp = JS_GetScriptedCaller(cx, NULL)->script;

For JS_GetScriptedCaller and JS_IsConstructing, I wondered aloud on IRC whether it's worth adding a js_* API to avoid relocatable function call penalties.

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>@@ -4562,20 +4563,20 @@ js_generic_native_method_dispatcher(JSCo
>-    JS_ASSERT(cx->fp->argv == argv);
>+    JS_ASSERT(js_GetTopStackFrame(cx)->argv == argv);
>     if (!js_ComputeThis(cx, JS_TRUE, argv))
>         return JS_FALSE;
>-    cx->fp->thisp = JSVAL_TO_OBJECT(argv[-1]);
>+    js_GetTopStackFrame(cx)->thisp = JSVAL_TO_OBJECT(argv[-1]);

Calling js_GetTopStackFrame inside JS_ASSERT seems like it's asking for trouble. You could, instead, move the assertion under the call to js_ComputeThis, after you've called it to set thisp for real.

>diff --git a/js/tests/jsDriver.pl b/js/tests/jsDriver.pl

The changes to this file deserve their own bug.

>diff --git a/xpcom/analysis/jsstack.js b/xpcom/analysis/jsstack.js

dmandelin should really review this file...

>+    warning("(Remove the workaround in jsinterp.js and recompile to get a JS stack trace.)",
>+            location_of(isn));

...but that jsinterp.js should be jsstack.js anyway.

r=mrbkap with my comments addressed.
Attachment #345605 - Flags: review?(mrbkap) → review+
Attached patch changes from v2 to v3 (obsolete) — Splinter Review
Attached patch v3 (obsolete) — Splinter Review
Addresses mrbkap's comments.  Carrying forward the r+.

dmandelin, please review the analysis.
Attachment #345605 - Attachment is obsolete: true
Attachment #347403 - Flags: review+
Attachment #347403 - Flags: review?(dmandelin)
This patch has no effect on the SunSpider numbers.
The explanation of the RED and GREEN properties is very clear, but I think you want to add a note saying which annotations are trusted and which are untrusted. Partly because I'm not entirely sure after reading the code.

+// Why is this necessary?  We don't know.
+MapFactory.use_injective = true;

The answer is that it makes things run faster because each hash bucket can only ever contain one thing.

+function attrs(tree) {
+  let a = DECL_P(tree) ? DECL_ATTRIBUTES(tree) : TYPE_ATTRIBUTES(TREE_TYPE(tree));
+  return translate_attributes(a);
+}
+
+function hasUserAttribute(tree, attrname) {
+  let attributes = attrs(tree);
+  if (attributes) {
+    for (let i = 0; i < attributes.length; i++) {
+      let attr = attributes[i];
+      if (attr.name == 'user' && attr.value.length == 1 && attr.value[0] == attrname)
+        return true;
+    }
+  }
+  return false;
+}

Please put the above 2 functions into Treehydra in gcc_util.js.

+  // Ordinarily a user of ESP runs the analysis, then generates output based
+  // on the results.  But in our case (a) we need sub-basic-block resolution,
+  // which ESP doesn't keep; (b) it so happens that even though ESP can
+  // iterate over blocks multiple times, in our case that won't cause
+  // spurious output.  (It could cause us to the same error message each time
+  // through--but that's easily avoided.)  Therefore we generate the output
+  // while the ESP analysis is running.

This is entirely reasonable.

+  // Tell ESP that fndecl is a "variable".  It'll be 1 in RED regions and
+  // "don't know" in GREEN regions.  We are lying to ESP because ESP tracks the
+  // state of variables only.
+  this._state_var_decl = fndecl;
+  let state_var = new ESP.PropVarSpec(this._state_var_decl,
+                                      false,
+                                      isRed(fndecl) || isTurnRed(fndecl) ? 1 : undefined);

The comment is not quite true. ESP tracks the variables that your flow function sets. The special significance of property variables is that ESP analyzes them in a path-sensitive way.

If you set param 2 (keep) to 'true', you will not need to add these to keepVars later on. If that doesn't work, it's a bug in ESP.

Param 3 seems wrong. If the function is a TurnsRed, doesn't that mean the state is not necessarily red at the start? Or are they more magical than I think? 

+  // Lame - hack private data structures to make sure ESP doesn't throw away
+  // the value of our state variable.
+  for (let bb in cfg_bb_iterator(cfg))
+    bb.keepVars.add(this._state_var_decl);
(In reply to comment #16)
> The explanation of the RED and GREEN properties is very clear, but I think you
> want to add a note saying which annotations are trusted and which are
> untrusted. Partly because I'm not entirely sure after reading the code.

What do "trusted" and "untrusted" mean here?

> +function attrs(tree) { ...
> +function hasUserAttribute(tree, attrname) { ...
> 
> Please put the above 2 functions into Treehydra in gcc_util.js.

Per IRC discussion I'm not doing this yet.  I'm not comfortable enough that my use of attributes here makes any sense.  Waiting for more experience with GCC attributes.
(In reply to comment #17)
> (In reply to comment #16)
> > The explanation of the RED and GREEN properties is very clear, but I think you
> > want to add a note saying which annotations are trusted and which are
> > untrusted. Partly because I'm not entirely sure after reading the code.
> 
> What do "trusted" and "untrusted" mean here?

By "untrusted" I meant checked by the analysis. By "trusted" I meant not checked by the analysis (i.o.w., the analysis assumes the annotation is correct).
Ah, right, OK.  If I explained that, it would also answer this comment:

> Param 3 seems wrong. If the function is a TurnsRed, doesn't that mean the state
> is not necessarily red at the start? Or are they more magical than I think?

because the TurnsRed annotation is trusted.

It would be pretty easy to check it, given a single trusted TurnsRed function that the others would call.
Attached patch v4Splinter Review
Rebased for checkin at last.  This also addresses dmandelin's comments.  Carrying forward mrbkap's review again.
Attachment #347403 - Attachment is obsolete: true
Attachment #351403 - Flags: review+
Attachment #347403 - Flags: review?(dmandelin)
Attachment #347402 - Attachment is obsolete: true
Attachment #351404 - Flags: review?(dmandelin)
Attachment #351404 - Flags: review?(dmandelin) → review+
Blocks: 460336
No longer blocks: 460336
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 468937
Depends on: 469191
Depends on: 469872
Comment on attachment 351403 [details] [diff] [review]
v4

need this on 1.9.1?
Attachment #351403 - Flags: approval1.9.1?
Attachment #351403 - Flags: approval1.9.1? → approval1.9.1+
Flags: blocking1.9.1+
Depends on: 471197
Flags: in-testsuite-
Flags: in-litmus-
Flags: in-testsuite- → in-testsuite+
benjamin: the test is part of treehydra?
Yes, we have automated static analysis to verify that code doesn't use cx->fp incorrectly. It's currently running on mozilla-central, but I plan to set something up for tracemonkey as well to catch bugs early.
verified 1.9.1, 1.9.2 based on green static analysis for mozilla-central and tracemonkey.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.