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

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Sorry for the long delay, I have just finished my holiday.

Thanks a lot for the review, I will fix the typo in next update.



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2100
+    std::tie(StateNullChar, StateNonNullChar) =
+        assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+
----------------
a.sidorin wrote:
> I think we can use the argument type here (which is int).
> One more question. Could we use `SVal::isZeroConstant()` here instead? Do we need the path-sensitivity for the value or we can just check if the value is a constant zero?
Yea, it's should be int.

`SVal::isZeroConstant()` is enough here, thanks!

One question that has nothing to do with the patch, I would like to know if there is a standard to determine whether there is a need for state splitting.



================
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127
+                             /*IsSourceBuffer*/ false, Size);
+  }
+
----------------
a.sidorin wrote:
> Could you please factor this chunk to a function? It makes the method too big.
will do.


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:148
+  ProgramStateRef NewState = makeWithStore(NewStore);
+  return Mgr.getOwningEngine()
+             ? Mgr.getOwningEngine()->processRegionChange(NewState, R, LCtx)
----------------
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?


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1718
 
-    llvm_unreachable("Unknown default value");
+    return val;
   }
----------------
a.sidorin wrote:
> Could you please explain what is the reason of this change? Do we get new value kinds?
`memset()` can bind anything to the region with `default binding`, if there is no such change, it will trigger `llvm_unreachable()`. 

I don't think much about it, just put `return val;` here. Any suggestions?


================
Comment at: lib/StaticAnalyzer/Core/Store.cpp:72
 
+StoreRef StoreManager::OverwriteRegion(Store store, const MemRegion *R, SVal V) {
+  return StoreRef(store, *this); 
----------------
a.sidorin wrote:
> Could we make this method pure virtual?
To be honest, I implemented this function according to `BindDefault()`. And I also think pure virtual is better, thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D44934