[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hmm, ok, it seems that i've just changed the API in https://reviews.llvm.org/D46368, and i should have thought about this use case. Well, at least i have some background understanding of these problems now. Sorry for not keeping eye on this problem.

In https://reviews.llvm.org/D44934#1051002, @NoQ wrote:

> Why do you need separate code for null and non-null character? The function's semantics doesn't seem to care.


I guess i can answer myself here:

  int32_t x;
  memset(&x, 1, sizeof(int32_t));
  clang_analyzer_eval(x == 0x1010101); // should be TRUE

I really doubt that we support this case.

So, yeah, zero character is indeed special.

And since zero character is special, i guess we could use the new `bindDefaultZero()` API, and perform a simple invalidation in the non-zero character case. @MTC, would this be enough for your use case? Or is it really important for you to support the non-zero character case? Cause my example is fairly artificial, and probably i'm worrying too much about it. If nobody really codes that way, i'm fine with supporting the non-zero character case via a simple default binding. In this case we should merge `bindDefaultZero()` with your `overwriteRegion()` (i'd prefer your function name, but please keep the empty class check).



================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:148
+  ProgramStateRef NewState = makeWithStore(NewStore);
+  return Mgr.getOwningEngine()
+             ? Mgr.getOwningEngine()->processRegionChange(NewState, R, LCtx)
----------------
MTC wrote:
> a.sidorin wrote:
> > Do clients of `overwriteRegion` really need to call checkers?
> It is possible that my understanding of the engine is not enough, I think we need to call `processRangeChange()` for memory change. `memset()` will overwrite the memory region, so I think there is a need to call this API.
> 
> What do you think?
Well, we need to make sure we don't notify checkers recursively. In `bindLoc()` we have the `notifyChanges` parameter, i guess we need to have something similar here as well, if we really need to notify checkers.

Technically, yeah, we need to notify checkers. Like, if you memset() a string to `'\0'`, string checker needs to know that its length has also become 0. Wait, we already are in the string checker, and we're already doing that. I guess it's not that important at the moment, so i'd rather delay it until we need it, and see if we have shared checker states available earlier and if they help.

Also `getOwningEngine()` is never null.


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:118
                                       const LocationContext *LCtx,
                                       bool notifyChanges) const {
   ProgramStateManager &Mgr = getStateManager();
----------------
Note: `notifyChanges` declared here :)


Repository:
  rC Clang

https://reviews.llvm.org/D44934