NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2429
if (SymbolRef Sym = Len.getAsSymbol()) {
if (SR.isDead(Sym))
+ Entries = F.remove(Entries, MR);
----------------
NoQ wrote:
> martong wrote:
> > steakhal wrote:
> > > NoQ wrote:
> > > > 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.
> > > Let's say we have this:
> > > ```lang=C++
> > > void foo() {
> > > char *p = malloc(12);
> > > // strlen(p); // no-leak if strlen called, leak warning otherwise...
> > > } // expected-warning {{leak}}
> > > ```
> > > If we were marking the region live here, we would miss the `leak`
> > > warning. That warning is only triggered if the region of the `p` becomes
> > > dead. Which will never become dead if we have a cstring length symbol
> > > associated to that region.
> > > I came to this conclusion after implementing your suggested edit above
> > > (checking regions instead of symbols).
> > Is it possible to have two string length symbols that are associated to the
> > same region? Could that cause any problem?
> > E.g. what will happen below? Will we remove `MR` twice?
> > ```
> > char *p = "asdf"
> > char *q = p + 1;
> > strlen(p); strlen(q);
> > ```
> > Which will never become dead if we have a cstring length symbol associated
> > to that region.
>
> That's not how it's supposed to work. Symbols don't keep their associated
> regions alive, only regions keep symbols associated with them alive.
>
> Don't manipulate region liveness manually at all here. Regardless of
> `strlen()`, the heap region dies with `p` which is the last variable to refer
> to it. Rely on it to garbage-collect the symbol for `strlen(p)` but don't
> actively mutate it.
> E.g. what will happen below?
These are different regions. Your `p` is `Element{0, "asdf", char}`, whereas
your `q` is `Element{1, "asdf", char}`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86445/new/
https://reviews.llvm.org/D86445
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
