Fixed multiple ordering bugs involving unsafe a-temporal abstract values #2413

sb98052 · 2018-08-10T06:46:47Z

Release notes: Fixed multiple ordering bugs

Fixes #2327 and #2406. The discussion around this set of issues can be found in #2327. To summarize, sometimes the serializer's CSE algorithm would relocate simplified values out of the path along which they were simplified. This relocation is unsafe for values that can cause side effects and ones that can throw. Otherwise, it may be wasteful but safe.

This PR helps improves the situation by making all potentially unsafe abstract values temporal. The cases I found came out of a code audit. Calls to createFromTemplate should have been covered comprehensively. Most unsafe operations still don't accept abstract values and throw introspection errors, so they are not a problem yet.

sebmarkbage · 2018-08-10T07:16:02Z

I don’t understand the criteria decided upon. How are not everything temporal according to this definition?

I believe that almost every expression in JavaScript can throw if given invalid input. So when should I not mark something as temporal?

(If we count TDZ, I believe literally everything can throw.)

sb98052 · 2018-08-10T18:33:24Z

I think a more complete statement of the criterion would be "the value produced by the template can throw under a condition that is not precluded by the assumptions under which the template is used." This is the root of the specific bug that this PR tries to knock off, since such dynamic conditions are sensitive to simplifications by the simplifier.

The general problem of formalizing the constraints under which something can be atemporal and balancing those constraints with efficiency is still open, and this PR doesn't help with that.

sebmarkbage

"the value produced by the template can throw under a condition that is not precluded by the assumptions under which the template is used"

This seems like circular reasoning to me. This can only be used under the conditions we use it.

I think the most useful definition is that something should be temporal if it can throw at runtime, read mutable state or have side-effects, even when the args to the abstract value have the types that are assumed/inferred by the interpreter.

The issue here is that we end up passing an incorrect type arg. That means that any expression has to be able to accept any type or at least always undefined. This is not useful.

I don't think it's acceptable that everything becomes temporal which is the natural consequence of this.

My concrete concern is that figuring out whether something needs to be temporal or not is hard work. We have already converted things to temporal that shouldn't be. As a result, we now have to go through all of the temporal values and reevaluate if they really should be which is hard work. As we convert more of these, that list grows and it becomes even harder. We have to stop the leak.

I would be much more happy if we just turned on a flag in AbstractValue to turn everything temporal automatically if we just want to work around a bug temporarily. That way we can just turn the flag off when we've fixed the issue that doesn't let us distinguish.

sebmarkbage · 2018-08-10T18:59:38Z

src/methods/get.js

@@ -344,7 +344,10 @@ export function OrdinaryGetPartial(
  Receiver: Value
 ): Value {
  if (Receiver instanceof AbstractValue && Receiver.getType() === StringValue && P === "length") {
-    return AbstractValue.createFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], lengthTemplateSrc);
+    return AbstractValue.createTemporalFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], {


This is a simple example. Given that the type of the Receiver is a string, this can never throw, have side-effects nor read mutable state (since it's immutable). It can only error if this is fed with the wrong type of input arg. That needs to be fixed.

However, once we fix that, how will I know that this can be turned atemporal again? Not everything has this property. So I have to go through them all one-by-one and think hard if this can really throw or not.

Simple: if Receiver is temporal, the get call must be temporal. Otherwise it is a-temporal.

sebmarkbage · 2018-08-10T19:05:18Z

Another approach could be to require all abstract values to be derived using the generator but add a flag like isTemporal to distinguish things that could be reordered vs things that can't. As long as that information is preserved in the source code.

sb98052 · 2018-08-10T20:39:24Z

"the value produced by the template can throw under a condition that is not precluded by the assumptions under which the template is used"

This seems like circular reasoning to me. This can only be used under the conditions we use it.

More like "This can only throw under the conditions in which it is expected to throw" which is not circular. It's the throwing under certain conditions that is precluded, not the conditions themselves. But my phrasing didn't help in making this clear.

That said, the distinction is irrelevant if the precondition for a value to be atemporal is that it never throw at runtime.

I do see your concern - my approach makes some values atemporal in a conditional setting, but at the cost of making many others temporal, even in an unconditional setting. As a side effect of my change, calls to String.length and such would never get lifted, and that's bad.

I'll try to come up with an approach that strikes a better balance, given these parameters.

hermanventer

This seems to be a work in progress and not ready for merging just yet.

hermanventer · 2018-08-10T20:34:35Z

src/evaluators/BinaryExpression.js

+        // "in" and "instanceof" can throw and therefore must result in temporals
+        if (result instanceof AbstractValue && (op === "in" || op === "instanceof")) {
+          invariant(realm.generator !== undefined);
+          result = realm.generator.deriveAbstract(result.types, result.values, result.args, result.operationDescriptor);


Only AbstractValue methods should ever call deriveAbstract directly.

hermanventer · 2018-08-10T20:37:13Z

src/evaluators/BinaryExpression.js

-        return AbstractValue.createFromBinaryOp(realm, op, lval, rval, loc);
+        let result = AbstractValue.createFromBinaryOp(realm, op, lval, rval, loc);
+
+        // "in" and "instanceof" can throw and therefore must result in temporals


If getPureBinaryOperationResultType does not throw, it should not be the case that the binary operation may throw or have a side effect.

hermanventer · 2018-08-10T20:41:49Z

src/intrinsics/ecma262/JSON.js

@@ -317,6 +317,8 @@ const JSONParse = buildExpressionTemplate(JSONParseStr);
 function InternalJSONClone(realm: Realm, val: Value): Value {
  if (val instanceof AbstractValue) {
    if (val instanceof AbstractObjectValue) {
+      // These may need to use the temporal variants of createFromTemplate


In particular, if val is temporal, then the derived values must be temporal as well. The reason being that val is known to be an object (and thus not null or undefined) at this temporal point, but may be null or undefined at a different temporal point. Since stringify and parse are not total when their arguments are null or undefined, they cannot become a-temporal when their arguments are not also a-temporal.

hermanventer · 2018-08-10T20:44:09Z

src/intrinsics/ecma262/JSON.js

@@ -317,6 +317,8 @@ const JSONParse = buildExpressionTemplate(JSONParseStr);
 function InternalJSONClone(realm: Realm, val: Value): Value {
  if (val instanceof AbstractValue) {
    if (val instanceof AbstractObjectValue) {
+      // These may need to use the temporal variants of createFromTemplate
+      // See #2327
      let strVal = AbstractValue.createFromTemplate(realm, JSONStringify, StringValue, [val], JSONStringifyStr);


Are you fixing things or just adding a todo comment? If the former, this is not complete, if the latter then please add TODO to the comment along with an issue number.

hermanventer · 2018-08-10T20:45:03Z

src/intrinsics/ecma262/StringPrototype.js

@@ -577,7 +577,10 @@ export default function(realm: Realm, obj: ObjectValue): ObjectValue {
    let O = RequireObjectCoercible(realm, context);

    if (O instanceof AbstractValue && O.getType() === StringValue) {
-      return AbstractValue.createFromTemplate(realm, sliceTemplate, StringValue, [O, start, end], sliceTemplateSrc);
+      return AbstractValue.createTemporalFromTemplate(realm, sliceTemplate, StringValue, [O, start, end], {


You only need to to do this if O is temporal.

hermanventer · 2018-08-10T20:45:26Z

src/intrinsics/ecma262/StringPrototype.js

-        [O, separator, limit],
-        splitTemplateSrc
-      );
+      return AbstractValue.createTemporalFromTemplate(realm, splitTemplate, ObjectValue, [O, separator, limit], {


Check if O is temporal.

hermanventer · 2018-08-10T20:46:32Z

src/methods/get.js

@@ -344,7 +344,10 @@ export function OrdinaryGetPartial(
  Receiver: Value
 ): Value {
  if (Receiver instanceof AbstractValue && Receiver.getType() === StringValue && P === "length") {
-    return AbstractValue.createFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], lengthTemplateSrc);
+    return AbstractValue.createTemporalFromTemplate(realm, lengthTemplate, NumberValue, [Receiver], {


Simple: if Receiver is temporal, the get call must be temporal. Otherwise it is a-temporal.

sb98052 · 2018-08-13T15:51:53Z

Very nice. Herman's insight strikes the balance we want, and also refines the criterion. In general, a value must be temporal if it is dynamic (can throw, side effects). The assumption that something is not dynamic may itself rely on dynamic parameters - and those should be temporal. Conversely, if a parameter is temporal, then a value that relies on such an assumption becomes temporal too.

I've updated my implementation to do this. Also, I have added inverted tests to make sure that the operations I have touched get lifted in the unconditional case.

This is still work in progress because turning on the currently commented out invariant in createAbstractConcreteValue breaks a few things. Also, some tests are breaking and have to be fixed.

sebmarkbage · 2018-08-14T20:23:36Z

I'm still not satisfied with that definition. If an abstract value is atemporal but depends on a temporal value, and is depended upon by a temporal (everything is eventually depended upon by a generator entry). That only means that it has to be computed between those two temporal points. We can still move it relative to other temporal points. That information is already there in the graph and can be computed by the visitor.

If we go with the definition for abstract values needing to be temporal if their dependencies are temporal, then we need to start modeling the dependency graph in the generators instead so that different entries in the generator can be reordered depending on their dependency graph.

sebmarkbage

I think this should probably just be modeled directly in AbstractValue. If any arg is temporal, the whole thing should be temporal for any abstract value.

Doing this on a one-by-one basis seems error prone to me. In backends like LLVM, the things that throw are not always the same as when generating JS neither.

Being specific in the interpreter about guessing whether the serialized form can throw or not seems problematic.

hermanventer · 2018-08-24T20:29:44Z

src/evaluators/BinaryExpression.js

+        // may rely on a dynamic condition. We express this condition as an implication,
+        // rval is temporal => this operation is temporal
+        // See #2327
+        if ((op === "in" || op === "instanceof") && (rvalIsAbstract && rval.isTemporal()))


Rather put this condition into a helper function named something like "operationMustBeTemporal".

I've deferred this to the refactoring in #2489.

hermanventer

There is a logic inversion that needs fixing and testing. There is also a commented out invariant that needs to be uncommented or deleted.

hermanventer · 2018-08-24T20:37:31Z

src/evaluators/BinaryExpression.js

+            createOperationDescriptor("BINARY_EXPRESSION", { op }),
+            { isPure: true, skipInvariant: true }
+          );
+        else return AbstractValue.createFromBinaryOp(realm, op, lval, rval, loc);


I'm still a little bit disturbed by this. If one of the arguments is temporal, then moving the operation to point where the current path conditions do not apply may be safe, but it seems hardly desirable.

In essence, any operation over temporal values should result in a temporal value, as a rule, except for a few specific cases that are well documented.

I'm going to enforce this protocol in a follow up PR that addresses #2489. Doing so will involve auditing uses of all of the abstract value factory methods.

hermanventer · 2018-08-24T20:39:27Z

src/intrinsics/ecma262/JSON.js

+      let obVal;
+
+      // If val is temporal, then this operation is also temporal. See #2327.
+      if (val.isTemporal()) {


This logic seems the inverse of what you intend. This seems to call for an additional test case.

It was inverted. On the other hand, it is not obvious to me that InternalJSONClone can create the particular conditions that this PR is trying to fix. So I have dropped this check for now, and will audit in a follow up request.

hermanventer · 2018-08-24T20:51:01Z

src/values/AbstractValue.js

+
+    // Abstract values in an abstract concrete union are temporal, as they are predicated
+    // on the conditions that preclude the concrete values in the union.
+    // invariant(abstractValue.isTemporal());


Why is this commented out?

Thanks, I should have emphasized that the implementation is work in progress. I am yet to audit all of the paths that lead to this invariant.

trueadm · 2018-08-31T09:11:47Z

Is this PR still active? Seems you addressed most of these issues in other PRs?

sb98052 requested review from sebmarkbage, trueadm, cblappert, hermanventer and NTillmann August 10, 2018 06:46

facebook-github-bot added the CLA Signed label Aug 10, 2018

sb98052 force-pushed the safe-templates branch 2 times, most recently from 740fdaf to ad27aca Compare August 10, 2018 17:01

sb98052 force-pushed the safe-templates branch from ad27aca to 0b95dfb Compare August 10, 2018 18:33

sebmarkbage suggested changes Aug 10, 2018

View reviewed changes

sebmarkbage reviewed Aug 10, 2018

View reviewed changes

hermanventer suggested changes Aug 10, 2018

View reviewed changes

sb98052 force-pushed the safe-templates branch from 0b95dfb to b713418 Compare August 13, 2018 15:27

sb98052 added the WIP This pull request is a work in progress and not ready for merging. label Aug 13, 2018

sebmarkbage reviewed Aug 17, 2018

View reviewed changes

sebmarkbage approved these changes Aug 17, 2018

View reviewed changes

hermanventer reviewed Aug 24, 2018

View reviewed changes

hermanventer suggested changes Aug 24, 2018

View reviewed changes

sb98052 changed the title ~~Fixed multiple ordering bugs involving unsafe a-temporal abstract values~~ Make non-exempt operations temporal if args are temporal Aug 27, 2018

sb98052 changed the title ~~Make non-exempt operations temporal if args are temporal~~ Fixed multiple ordering bugs involving unsafe a-temporal abstract values Aug 27, 2018

Sapan Bhatia added 2 commits August 27, 2018 11:20

Fixed multiple ordering bugs involving unsafe a-temporal abstract values

61e7ebf

Comments

6c4331e

sb98052 force-pushed the safe-templates branch from b713418 to 6c4331e Compare August 27, 2018 17:46

sb98052 closed this Sep 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed multiple ordering bugs involving unsafe a-temporal abstract values #2413

Fixed multiple ordering bugs involving unsafe a-temporal abstract values #2413

sb98052 commented Aug 10, 2018 •

edited

sebmarkbage commented Aug 10, 2018 •

edited

sb98052 commented Aug 10, 2018

sebmarkbage left a comment •

edited

sebmarkbage Aug 10, 2018 •

edited

hermanventer Aug 10, 2018

sebmarkbage commented Aug 10, 2018 •

edited

sb98052 commented Aug 10, 2018 •

edited

hermanventer left a comment

hermanventer Aug 10, 2018

hermanventer Aug 10, 2018

hermanventer Aug 10, 2018

hermanventer Aug 10, 2018

hermanventer Aug 10, 2018

hermanventer Aug 10, 2018

hermanventer Aug 10, 2018

sb98052 commented Aug 13, 2018 •

edited

sebmarkbage commented Aug 14, 2018

sebmarkbage left a comment

hermanventer Aug 24, 2018

sb98052 Aug 27, 2018

hermanventer left a comment

hermanventer Aug 24, 2018

sb98052 Aug 27, 2018

hermanventer Aug 24, 2018

sb98052 Aug 27, 2018

hermanventer Aug 24, 2018

sb98052 Aug 27, 2018

trueadm commented Aug 31, 2018

Fixed multiple ordering bugs involving unsafe a-temporal abstract values #2413

Fixed multiple ordering bugs involving unsafe a-temporal abstract values #2413

Conversation

sb98052 commented Aug 10, 2018 • edited

sebmarkbage commented Aug 10, 2018 • edited

sb98052 commented Aug 10, 2018

sebmarkbage left a comment • edited

Choose a reason for hiding this comment

sebmarkbage Aug 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Aug 10, 2018 • edited

sb98052 commented Aug 10, 2018 • edited

hermanventer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb98052 commented Aug 13, 2018 • edited

sebmarkbage commented Aug 14, 2018

sebmarkbage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hermanventer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm commented Aug 31, 2018

sb98052 commented Aug 10, 2018 •

edited

sebmarkbage commented Aug 10, 2018 •

edited

sebmarkbage left a comment •

edited

sebmarkbage Aug 10, 2018 •

edited

sebmarkbage commented Aug 10, 2018 •

edited

sb98052 commented Aug 10, 2018 •

edited

sb98052 commented Aug 13, 2018 •

edited