[PATCH] D14277: [Analyzer] Make referenced SymbolMetadata live even if its region is dead

[Aleksei Sidorin via cfe-commits]

a.sidorin added a comment.

> - What happens when you take a string's length, test it against something, throw it away, then remeasure the string? In this case we'd be okay because CStringChecker still hangs on to the symbol in its map until the region is invalidated.


We cannot throw out anything related to live region we reference in the checker. While region lives, checker will keep its length alive, so this case seem to be OK.

> - Will we keep more symbols alive than we did previously, increasing the analyzer's memory use? I think the answer is "yes", but not by very much: the live/dead checking is two-pass, so we'll mark a metadata symbol live in `checkLiveSymbols` but then find out we could have dropped it in `checkDeadSymbols` when the region is found to be dead. It'll get cleaned up the next time around, but I think that sort of conditional liveness is what the old mechanism was trying to accomplish.


Yes, you are right. The lengths of dead regions will be garbage-collected only in the next collection so they will consume some memory.

> Do you have any ideas for how we could make that better? I can think of a complicated case where you use `markInUse` if the metadata symbol exactly matches the key, but `markLive` otherwise…but at some point I'm not sure it's worth the complexity cost.


As I understand, to make a precise GC, we need to do it in a number of iterations but now we make only one iteration. This does not influence correctness because we are conservative and we don't remove information that is alive (but may keep some dead). However, iterative GC may dramatically decrease analyzer perfomance.

> (In particular, after this change, I'm not sure metadata symbols behave any differently from regular conjured symbols. This is a good thing.)


Yes. But we extensively use SymbolMetadata's information in our summary implementation so don't hurry to throw it out :)

> (We've also talked about having better REGISTER_MAP_WITH_PROGRAMSTATE that auto-remove any keys for dead symbols or dead regions. I'm sure Anna and Devin would be happy to see someone pick that up.)


I don't think it is a good idea because some checkers handle dead symbols in customizable ways to detect leaks. Some helper for remove all items with dead keys from state working with common collections (in CheckerHelpers, for example) may be useful to to avoid code duplication, but it is an item for another patch.

BTW, what about my comment for line 457? Should we consider a metadata symbol always dead after this patch?


Repository:
  rL LLVM

http://reviews.llvm.org/D14277