Page MenuHomePhabricator

[analyzer] Don't track the right hand side of the last store for conditions
AbandonedPublic

Authored by Szelethus on Jul 5 2019, 2:51 PM.

Details

Summary

Exactly what it says on the tin!

Diff Detail

Event Timeline

Szelethus created this revision.Jul 5 2019, 2:51 PM
NoQ added a comment.Jul 5 2019, 10:05 PM

Consider:

int flag;
bool coin();

void foo() {
  flag = coin();
}

void test() {
  int *x = 0;
  int local_flag;
  flag = 1;

  foo();
  local_flag = flag;
  if (local_flag)
    x = new int;

  foo();
  local_flag = flag;
  if (local_flag)
    *x = 5;
}

I'd rather track flag when i reach local_flag. I believe that we must track the RHS, but only as long as it's an overwrite on its own (or it's the value that participated in the collapse).

This is why the heuristic that i suggested was "track normally until the *last* out-of-frame overwrite point or until the collapse point".

Szelethus abandoned this revision.EditedJul 6 2019, 9:39 AM

You're right. If condition tracking only adds necessary information, this shouldn't hurt that much anyways.

NoQ added a comment.Jul 9 2019, 12:54 PM

I'd rather not abandon this patch, because it looks like a strict improvement over the lack of condition tracking, and it might as well still be an improvement over "zealous" condition tracking, as my counterexample is fairly artificial. It indicates that a slightly more sophisticated algorithm is necessary (i'm not sure if it's single-pass or even linear). But i'll be perfectly happy with simply adding it as a FIXME test.

In D64271#1576872, @NoQ wrote:

I'd rather not abandon this patch, because it looks like a strict improvement over the lack of condition tracking, and it might as well still be an improvement over "zealous" condition tracking, as my counterexample is fairly artificial. It indicates that a slightly more sophisticated algorithm is necessary (i'm not sure if it's single-pass or even linear). But i'll be perfectly happy with simply adding it as a FIXME test.

My other patches actually deal with this far more nicely. D64272 + D64232 combined almost achieve the same thing, but I'm running analyses to find gather some real world experience.
.