Only havoc abstract value args when really necessary #2184

sebmarkbage · 2018-06-29T07:55:35Z

In #2179 I describe why we might need to havoc some args to abstract values sometimes. This adds an invariant to ensure that we always have args in the cases we need to havoc.

If this abstract value cannot be an object, then we don't need to havoc since there's nothing that can be mutated.

If this has a known values domain, then we know the possible objects. We only need to havoc those, not the args.

NTillmann

I left a comment in #2179. I don't fully understand the general reasoning here.

I feel that a one-size-fits-all treatment of abstract values doesn't generally work (unless this is just meant as an incremental refinement).

NTillmann · 2018-06-29T15:58:24Z

test/serializer/pure-functions/AbstractObject.js

+  let knownObj = {x:1};
+  let knownObj2 = {y:3};
+  let conditionalObj = c ? knownObj : knownObj2;
+  fn(conditionalObj); // This must havoc both known objects


I guess this is the case where we have concrete objects in the value domain, and the second half of visitAbstractValue kicks in.

NTillmann · 2018-06-29T15:59:36Z

test/serializer/pure-functions/AbstractObject2.js

+function test(fn) {
+  let knownObj = {x:1};
+  let conditionalUnknownObj = c ? knownObj : unknownObj;
+  fn(conditionalUnknownObj); // This must havoc the known obj


I guess here the first half of visitAbstractValue, since we don't have an overapproximation of a concrete values domain. As far as I can read, this would also potentially havoc objects referenced in c. So I think another refinement and test case is needed to avoid and test that.

I could add a special case for conditionals like this but there is really a large number of similar operations. I can’t special case them all. I just thought of this one as a test case.

sebmarkbage · 2018-06-29T16:35:14Z

This PR is just slightly better than what came before it. The additions are the proper way to model it I think.

Havocing the args (which was already there) is a bit sketchy. The goal is to be as conservative as possible for things we haven’t yet modeled so that we don’t yield incorrect code.

For loops I want to add a template object form which doesn’t guarantee which concrete instance it is but guarantees that it is not one of the known ones. I believe that can help us model abstract objects without going to topVal in more cases so that we don’t end up needing to be conservative.

NTillmann · 2018-06-29T18:25:05Z

src/utils/havoc.js

+        "Havoced unknown object requires havocable arguments"
+      );
+
+      for (let i = 0, n = val.args.length; i < n; i++) {


Maybe leave a TODO here that havcoing all args is still overly conservative in general (e.g. for "conditional").

In facebookarchive#2179 I describe why we might need to havoc some args to abstract values sometimes. This adds an invariant to ensure that we always have args in the cases we need to havoc. If this abstract value cannot be an object, then we don't need to havoc since there's nothing that can be mutated. If this has a known values domain, then we know the possible objects. We only need to havoc those, not the args.

hermanventer · 2018-06-29T18:24:10Z

src/utils/havoc.js

-    for (let i = 0, n = val.args.length; i < n; i++) {
-      this.visitValue(val.args[i]);
+    if (!val.mightBeObject()) {
+      // Only objects need to be havoced.


I can see that we should not waste time on non objects nor havoc objects that appear in the val.args, but cannot be seen by the callee. This is all good.

hermanventer · 2018-06-29T18:29:43Z

src/utils/havoc.js

+    if (val.values.isTop()) {
+      // If we don't know which object instances it might be,
+      // then it might be one of the arguments that created
+      // this value. See #2179.


Here I get concerned. By definition ALL objects known to prepack might be denoted by val. Just havocing the arguments does not seem sufficient. As things are today, we refuse to compile any program that accesses a property of an abstract object with a values domain that is Top.

We don’t refuse to compile programs in pure scope mode. It havocs the object in case it had a getter on it.

It would be way too restrictive to completely not allow for these.

In practice this almost always works out though. Most operations either havoc eagerly as the operation produces the abstract, or it leaves the possible values on the args.

I think we need to come up with a less conservative way of modeling the cases that go to topVal.

hermanventer · 2018-06-29T18:30:59Z

src/utils/havoc.js

+      // that can be havoced, we require at least one argument.
+      let whitelistedKind =
+        val.kind &&
+        (val.kind === "widened numeric property" || // TODO: Widened properties needs to be havocable.


It seems wrong to for a value that calls out "numeric" in its kind to not have a types domain of NumberValue. Not the fault of this PR, but now that we've noticed....

It is the property name that is numeric. Not the value of it.

Ah, very misleading.

I think the widened slots will need to change to be modeled on the property binding/environment binding instead of as a value.

hermanventer · 2018-06-29T18:45:59Z

src/utils/havoc.js

+      let whitelistedKind =
+        val.kind &&
+        (val.kind === "widened numeric property" || // TODO: Widened properties needs to be havocable.
+          val.kind.startsWith("abstractCounted"));


It took me a while to convince myself that this is OK. Essentially, anything created this way is "pre-havoced", i.e. we don't know the value of a property of such an object and we get a new one every time we access the property. At the very least we should provide and efficient and much more obviously correct way of checking that an abstract value is of this kind.

On second thoughts: We can only access properties of abstract objects with templates and such object will have a values domain that is not top.

Optimized functions get these arguments passed into them. https://github.com/facebook/prepack/blob/3deb5e2885a689e7b8be5bb8ad88e24fe8e4fecf/src/serializer/functions.js#L150

These two are the only cases of empty args that occurs in test coverage.

It is ok not to havoc them since they're arguments to pure functions, they can't be mutated.

hermanventer · 2018-06-29T19:03:47Z

It seems important to make sure that we have 100% code coverage for the new code.

hermanventer · 2018-06-29T19:06:33Z

Except for the last new test case, it is not obvious to me that the new test cases will fail on master.

sebmarkbage · 2018-06-29T19:37:04Z

Note that master is more conservative than this. The other new test cases doesn't fail on master but they do test that the havocing that was already in master still works. If I removed the havocing of args, then there wouldn't be other tests that failed. So this ensures that all branches are covered.

facebook-github-bot

@sebmarkbage is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

sebmarkbage requested review from sb98052, hermanventer and NTillmann June 29, 2018 07:55

facebook-github-bot added the CLA Signed label Jun 29, 2018

NTillmann reviewed Jun 29, 2018

View reviewed changes

NTillmann approved these changes Jun 29, 2018

View reviewed changes

sebmarkbage added 2 commits June 29, 2018 11:50

Don't havoc the condition in a conditional

60d79e3

sebmarkbage force-pushed the argshavoc branch from 9e768cb to 60d79e3 Compare June 29, 2018 18:50

hermanventer reviewed Jun 29, 2018

View reviewed changes

facebook-github-bot reviewed Jun 29, 2018

View reviewed changes

facebook-github-bot closed this in 3240eae Jun 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only havoc abstract value args when really necessary #2184

Only havoc abstract value args when really necessary #2184

sebmarkbage commented Jun 29, 2018

NTillmann left a comment

NTillmann Jun 29, 2018

NTillmann Jun 29, 2018

sebmarkbage Jun 29, 2018

sebmarkbage commented Jun 29, 2018 •

edited

Loading

NTillmann Jun 29, 2018

hermanventer Jun 29, 2018

hermanventer Jun 29, 2018

sebmarkbage Jun 29, 2018

hermanventer Jun 29, 2018

sebmarkbage Jun 29, 2018

hermanventer Jun 29, 2018

sebmarkbage Jun 29, 2018

hermanventer Jun 29, 2018

hermanventer Jun 29, 2018

sebmarkbage Jun 29, 2018

hermanventer commented Jun 29, 2018

hermanventer commented Jun 29, 2018

sebmarkbage commented Jun 29, 2018

facebook-github-bot left a comment

Only havoc abstract value args when really necessary #2184

Only havoc abstract value args when really necessary #2184

Conversation

sebmarkbage commented Jun 29, 2018

NTillmann 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

sebmarkbage commented Jun 29, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hermanventer commented Jun 29, 2018

hermanventer commented Jun 29, 2018

sebmarkbage commented Jun 29, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

sebmarkbage commented Jun 29, 2018 •

edited

Loading