473492 - Teach Treehydra's Zero-Nonzero ESP analysis to handle EQ_EXPR nodes

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Graydon Hoare :graydon]

Attachment: Handle the node.

Turns out that unlike for bool b, for pointer p, !p gimplifies to p == 0. Need to partially evaluate.

Will revisit this eventually and do up a proper recursive evaluator, as the logic currently only goes 1 level deep and is smeared around the assignment handler, which is wrong. But this works for the shallow case I care about today.

Comment 1

[David Mandelin [:dmandelin]]

Comment on attachment 356871 [details] [diff] [review]
Handle the node.

One design issue, one bug, and I think we need tests. See below. By the way, I realized I r+'d the same bug and lack of test in 473430. Do you want me to fix that or do you want to do it?

>+// FIXME: generalize these two functions to a recursive partial evaluator.
>+function isEvaluable(isn) {
>+  return DECL_P(isn) || TREE_CODE(isn) == INTEGER_CST;
>+}
>+
>+function evaluate(isn, ss) {
>+  if (DECL_P(isn))
>+    return ss.get(isn);
>+  else if (TREE_CODE(isn) == INTEGER_CST)
>+    return TREE_INT_CST_LOW(isn);  
>+  throw new Error("Non-evaluable node");    
>+}

I think it would be better because less redundant if there were only the 'evaluate' function and it had a way to say "I don't know how to evaluate this", say by throwing an exception or returning a unique object. Is there a particular justification for the split? If not, I say just have the one.

>+    case EQ_EXPR:
>+      let [op1,op2] = rhs.operands();
>+      if (isEvaluable(op1) && isEvaluable(op2)) {
>+        state.assignMapped(lhs, function(ss) {
>+          let v1 = evaluate(op1, ss);
>+          let v2 = evaluate(op2, ss);
>+          return (v1 == v2 ? 1 : 0);
>+        }, isn);

This is wrong--we need to evaluate operators over abstract values. Since this is the znz library, it's fair to only understand its abstract values, which are the integers, NONZERO, and TOP. A specific case of wrongness is that if v1 is NONZERO and v2 is 2, then the result must be TOP.

Because of the trickiness of this, I guess we really need test cases, which I should have realized earlier. Too bad it's hard to do a true unit test in this environment, because it would be perfect.

Comment 2

[(dormant account)]

I'll second the request for testcases. Easiest way to do that would be to add another test for esp_lock.js that triggers your modifications to z/nz.

Comment 3

[Joshua Cranmer [:jcranmer]]

Dehydra and treehydra are no longer maintained by Mozilla.