This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] pr38668: RegionStore: Do not attempt to cast loaded values of non-scalar types.
ClosedPublic

Authored by NoQ on Dec 18 2018, 6:54 PM.

Details

Summary

It is expected to have the same object (memory region) treated as if it has different types in different program points. Like, the object may be a union or contain a union, or a pointer to it can be reinterpreted as a pointer to a different type. The correct behavior for RegionStore when an object is stored as an object of type T1 but loaded as an object of type T2 is to store the object as if it has type T1 but cast it to T2 during load. This is what CastRetrievedVal is responsible for.

Note that the cast here is some sort of a "reinterpret_cast" (even in C). For instance, if you store a float and load an integer, you won't have your float rounded to an integer; instead, you will have garbage.

Therefore i propose to admit that we cannot perform the cast as long as types we're dealing with are non-trivial (neither integers, nor pointers). Our modeling would still be weird in this case when dealing with integer casts, but so is support for integer casts in general.

Of course, if the cast is not necessary (eg, T1 == T2), we can still load the value just fine. This trivial observation in fact improves our behavior on tests and addresses some old FIXMEs: previously we would have tried to perform the cast and fail. We should probably also teach SValBuilder to perform casts in this case.

Fixes https://bugs.llvm.org/show_bug.cgi?id=38668

Diff Detail

Event Timeline

NoQ created this revision.Dec 18 2018, 6:54 PM

These seems reasonable, although it does also seem like there could be quite a few unintended consequences that we haven't discovered yet.

I'm also a bit worried about the change in the analyzer's behavior on copy(). Do you have a sense of how much of an effect this will have and how easy potential false positives from this will be to suppress?

test/Analysis/bstring.cpp
47

This seems like it will be a big analysis policy change from the user's perspective and is likely to generate a bunch of new reports.

Can the user add an assertion that v.size() > 0 to tell the analyzer that the path on which the vector is empty is not feasible?

What are the diagnostic notes look like? Can the user tell that the the analyzer is assuming that begin() == end() on that path?

NoQ marked an inline comment as done.Dec 19 2018, 12:21 PM
NoQ added inline comments.
test/Analysis/bstring.cpp
47

Ugh! This change is actually due to D55873, i just didn't run the tests properly. Will re-think :o

NoQ updated this revision to Diff 178963.Dec 19 2018, 2:00 PM

Remove the test change that wasn't caused by this patch.

NoQ marked an inline comment as done.Dec 19 2018, 3:39 PM
dcoughlin accepted this revision.Dec 19 2018, 3:43 PM

Looks good to me.

This revision is now accepted and ready to land.Dec 19 2018, 3:43 PM
This revision was automatically updated to reflect the committed changes.
NoQ reopened this revision.Dec 20 2018, 11:39 AM

Revert!

lib/StaticAnalyzer/Core/Store.cpp
410

This is entirely incorrect. The whole point of this function is to handle the case when R->getValueType() has nothing to do with the original type of V.

Unfortunately, "type of an SVal" is not a thing, so it's going to be a bit more verbose.

This revision is now accepted and ready to land.Dec 20 2018, 11:39 AM
NoQ marked an inline comment as done.Dec 20 2018, 11:41 AM
NoQ added inline comments.
lib/StaticAnalyzer/Core/Store.cpp
410

Relevant test case:

double no_crash_reinterpret_double_as_int(double a) {
  *(int *)&a = 1;
  return a * a;
}
NoQ marked an inline comment as done.Dec 20 2018, 11:43 AM
NoQ added inline comments.
lib/StaticAnalyzer/Core/Store.cpp
410

...which crashes after this patch while trying to multiply 1 by 1 and return result as double.

(sry for the noise)

NoQ updated this revision to Diff 179218.Dec 20 2018, 5:58 PM

Attempt to define the notion of "type of SVal".

NoQ updated this revision to Diff 179228.Dec 20 2018, 6:28 PM

Add a few more tests, just in case.

NoQ updated this revision to Diff 179230.Dec 20 2018, 6:29 PM

I'll test this more thoroughly. In case it's still wrong, here's a safer fix.

NoQ updated this revision to Diff 179352.Dec 21 2018, 2:10 PM

Add test case bool_to_nullptr in casts.cpp on which my second attempt crashes but the current code does not.

NoQ updated this revision to Diff 179355.Dec 21 2018, 2:23 PM

// no-crash and a slightly cleaner test.

george.karpenkov accepted this revision.Dec 21 2018, 3:39 PM

Seems good, but a comment explaining why this is necessary and why the crash follows otherwise would be great.

NoQ updated this revision to Diff 179379.Dec 21 2018, 4:04 PM

Add a comment.

I guess it doesn't really matter why does this lead to a crash. The symbol itself is well-formed, but we probably don't support it yet in some place, and hopefully (but not necessarily) CastRetrievedVal is the only place we produce it. The point here is that the old behavior is clearly incorrect (i.e., the code doesn't do the right thing, and due to that the well-formed symbol is in fact not the symbol that we're looking for) and the new behavior is clearly conservative (i.e., returning UnknownVal should be pretty safe).

What we really need is a more direct test. Probably unit tests for SValBuilder, or some of those "denote - express" tests.

NoQ updated this revision to Diff 179389.Dec 21 2018, 4:33 PM

Add a denote - express test.

This revision was automatically updated to reflect the committed changes.