[PATCH] D38358: [analyzer] Fix autodetection of getSVal()'s type argument.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1404
+          // When trying to dereference a void pointer, read the first byte.
+          T = Ctx.CharTy;
+        }
----------------
dcoughlin wrote:
> NoQ wrote:
> > dcoughlin wrote:
> > > Nit: It seems a bit odd to read the first byte here since (unless I'm misunderstanding) this would never be triggered by actual C semantics, only by a checker. Did you consider just returning UnknownVal() in this case?
> > `UnknownVal` seems to be quite similar to assertion failure: in both cases we'd have to force the checker to specify `CharTy` explicitly. But assertions are better because the author of the checker would instantly know that he needs to specify the type, instead of never noticing the problem, or spending a lot of time in the debugger trying to understand why he gets an `UnknownVal`.
>  I think having an assertion with a helpful message indicating how the checker author could fix the problem would be great! But you're not triggering an assertion failure here since you're changing T to be CharTy. Instead, you're papering over the problem by making up a fake value that doesn't correspond to the program semantics. If you're going to paper over the the problem and return a value, I think it is far preferable to use UnknownVal. This would signal "we don't know what is going here" rather than just making up a value that is likely wrong.
> If you're going to paper over the the problem and return a value, I think it is far preferable to use UnknownVal.

If we return an UnknownVal, the user would never figure out what's wrong by looking at the value, i.e. what limitation of the analyzer he's stepping on (in fact he isn't, he's just using the APIs incorrectly, while we know very well what's going on). Additionally, returning the first byte makes API simpler in cases the type actually doesn't matter (which is why it's not provided), unlike returning UnknownVal. This makes me think that returning UnknownVal is worse than both returning first byte and stepping on assertion. However, yeah, if we try to model an API that manipulates objects of well-defined types through void pointers, returning bytes might cause issues when the user doesn't realize he needs to specify his types, especially when RegionStore would often ignore the type and only take the offset, unless it has no bindings and needs to return region value or derived symbol (so the user may easily fail to test this case).

I guess i'd follow up with a patch to remove the defensive behavior and bring back the assertion and modify checkers to provide types when necessary, and later see if it's worth it to remove auto-detection completely.


Repository:
  rL LLVM

https://reviews.llvm.org/D38358