[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

Mon Aug 24 06:31:40 PDT 2020

martong added inline comments.

================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:198
 /// SymbolMetadata - Represents path-dependent metadata about a specific region.
 ///  Metadata symbols remain live as long as they are marked as in use before
 ///  dead-symbol sweeping AND their associated regions are still alive.
----------------
I think the comments should be updated as well: remove "in use" and refer to the aliveness of the memregion.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2417

 void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
     CheckerContext &C) const {
----------------
Seems like `checkDeadSymbols` could have a generic implementation. Perhaps in the following form:
```
class CStringChecker : public Checker<
                                         check::DeadSymbols<CStringLength>
```
But this should be done in a separate patch.

Maybe it would make sense to have a generic default check::LiveSymbols<GDM0, GDM1, ...> implementation as well. In that implementation we could iterate over the GDM entries and could mark the dependent (sub)symbols live.

(Note, this is a reminder from our verbal discussion with @steakhal.)

================
Comment at: clang/test/Analysis/string.c:1544
+
 void memset7_char_array_nonnull() {
   char str[5] = "abcd";
----------------
It would make sense to split this into two. Only one of them should be in the `FIXMEs` section with the {{UNKNOWN}}. TRUE expectations could be moved out from the FIXMEs section.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86445/new/

https://reviews.llvm.org/D86445