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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:441
+  const SymbolMetadata *getMetadataSymbol(const MemRegion *R, QualType T,
                                           const void *SymbolTag = nullptr);
 
----------------
steakhal wrote:
> Why do we even need the tag?
> Why is it defaulted to `nullptr` if it asserts later that the `tag` must not be `null`.
> I'm confused, somebody help me out :D
The tag is necessary so that different checkers could attach different metadata to the same region (possibly even of the same type). A null value indeed makes no sense here.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:516-522
   /// Unconditionally marks a symbol as live.
   ///
   /// This should never be
   /// used by checkers, only by the state infrastructure such as the store and
   /// environment. Checkers should instead use metadata symbols and markInUse.
+  /// TODO: update this comment!!!
   void markLive(SymbolRef sym);
----------------
steakhal wrote:
> Is it true for modeling checkers too?
This comment is much older than the concept of a "modeling checker" and it has never been true. Checkers need to mark things live. It's part of life! (no pun intended) Like, i agree that non-modeling checkers probably don't need to use this method, but that wasn't what the author was trying to say :) And also there's nothing special about `SymbolMetadata`; any data that the checker helps keep track of may need to be marked live by the checker, so `markInUse()` is insufficient.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2417
 
 void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
     CheckerContext &C) const {
----------------
martong wrote:
> 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.)
Nope, there can't be a generic implementation. It depends a lot on the data structure tracked by the checker (the checker's technically allowed to track "a map from regions to maps from lists of expressions to sets of SVals", good luck coming up with a generic `checkLiveSymbols()` for that) and it most likely isn't uniquely determined by the data structure either.

But what we can do is provide half-baked implementations for the common situations such as "a map from regions to symbols" (eg. this checker, smart pointer checker) or "a map from symbols to an enum of states the object associated with the symbol is in" (malloc checker, stream checker). And even better, we could provide half-baked implementations of entire state traits, not just `checkDeadSymbols`
/`checkLiveSymbols()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2429
     if (SymbolRef Sym = Len.getAsSymbol()) {
       if (SR.isDead(Sym))
+        Entries = F.remove(Entries, MR);
----------------
Ok, this doesn't look correct (looks like it never was). It's liveness of the region that's important to us, not liveness of the symbol. Because it's the liveness of the region that causes the program to be able to access the map entry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86445