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

Fri Mar 30 04:32:20 PDT 2018

MTC added a comment.

Thanks for your review, NoQ!

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

Thank you for your advice, I will remove the duplicate code！

> I'd rather consider the case of non-concrete character separately. Because wiping a region with a symbol is not something we currently support; a symbolic default binding over a region means a different thing and it'd be equivalent to invalidation, so for non-concrete character we don't have a better choice than to invalidate. For concrete non-zero character, on the contrary, a default binding would work just fine.

Thank you for your reminding, I overlooked this point. However for non-concrete character, the symbol value, if we just invalidate the region, the constraint information of the non-concrete character will be lost. Do we need to consider this?

> Could you explain why didn't a straightforward `bindLoc` over a base region wasn't doing the thing you wanted? I.e., why is new Store API function necessary?

I am also very resistant to adding new APIs. One is that I am not very familiar with RegionStore. One is that adding a new API is always the last option.

- The semantics of `memset()` need default binding, and only `bindDefault()` can guarantee that. However `bindDefault()` is just used to initialize the region and can only be called once on the same region.
- Only when the region is `TypedValueRegion` and the value type is `ArrayType` or `StructType`, `bindLoc()` can add a default binding. So if we want to continue using `default binding`, `bindLoc()` is not the correct choice.

And if I use `bindLoc()` instead of `overwriteRegion()`, there will be some crashes occured because `bindLoc()` broke some assumptions, e.g. `bindArray()` believes that the value for binding should be `CompoundVal`, `LazyCompoundVal` or `UnknownVal`.

Repository:
  rC Clang

https://reviews.llvm.org/D44934