This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Track the right hand side of the last store regardless of its value
ClosedPublic

Authored by Szelethus on Jul 6 2019, 9:34 AM.

Details

Summary

The following code snippet taken from D64271#1572188 has an issue: namely, because flag's value isn't undef or a concrete int, it isn't being tracked.

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;
}

This, in my opinion, makes no sense, other values may be interesting too. Originally added by rC185608.

Diff Detail

Event Timeline

Szelethus created this revision.Jul 6 2019, 9:34 AM
Szelethus retitled this revision from [analyzer] Track the right hand side of the last store unconditionally to [analyzer] Track the right hand side of the last store regardless of it's value.Jul 6 2019, 9:35 AM
Szelethus added a reviewer: dcoughlin.
Szelethus retitled this revision from [analyzer] Track the right hand side of the last store regardless of it's value to [analyzer] Track the right hand side of the last store regardless of its value.
NoQ added a comment.Jul 9 2019, 12:48 PM

Whoa, the new notes are amazing. Looks like you've found a pretty bad omission.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1404–1405

You're removing this visitor, does it cause any removal of notes?

1947–1949

Fair enough. Did we already have a test case for this? I wonder if function pointers might also cause problems here.

Ugh, this code is so underdebugged.

Szelethus marked 2 inline comments as done.Jul 14 2019, 9:39 AM

Would you say it's good to go? :)

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1404–1405

Nope, trackExpressionValue will add it back, if appropriate.

1947–1949

Yup, test/Analysis/exercise-ps.c flopped. And I agree, this looks kinda nasty.

NoQ accepted this revision.Jul 14 2019, 9:57 PM

Yeah, i think this is good to go :)

This revision is now accepted and ready to land.Jul 14 2019, 9:57 PM
xazax.hun accepted this revision.Jul 17 2019, 11:34 AM

LGTM!

Since we allow new kinds of SVals to be tracked it would be great to test this first on a larger corpus of projects just to see if there is a crash (due to an unhandled SVal type).

LGTM!

Since we allow new kinds of SVals to be tracked it would be great to test this first on a larger corpus of projects just to see if there is a crash (due to an unhandled SVal type).

It doesn't! Though that analysis was run with plenty of other patches as well, but I saw no crashes regardless.

This revision was automatically updated to reflect the committed changes.
Szelethus marked 2 inline comments as done.