[cfe-dev] [analyzer] Zombie symbols.

[Anna Zaks via cfe-dev]

> On Mar 24, 2016, at 4:29 AM, Yury Gribov <y.gribov at samsung.com> wrote:
> 
> Anna, Devin,
> 
> Could you comment on this? It sounds like an important design issue.

This is an important issue to address; unfortunately, I do not have a solution at hand.

> 
> On 03/14/2016 03:55 PM, Artem Dergachev via cfe-dev wrote:
>> A certain peculiar property of the removeDeadBindings() mechanism seems
>> to have been overlooked since quite a long time.
>> 
>> My colleague Ilya Palachev has recently shown me a specific leak
>> false-negative. It could be simplified down to the following fairly
>> short SimpleStreamChecker test that currently fails:
>> 
>> 
>>   void test() {
>>     FILE *fp = fopen("myfile.txt", "w");
>>     fp = 0; // Hmm, we should have overwritten the symbol.
>>   } // expected-warning{{Opened file is never closed; potential
>> resource leak}}
>> 
>> 
>> So what's going on? In fact, it is not quite correct to say that
>> "checkDeadSymbols() is  called for symbols that are no longer available
>> in the program state". It is just the opposite: it is only called for
>> symbols that *are* available in the program state, otherwise how does it
>> know that they even exist!
>> 
>> In order to die, the symbol needs to be marked as dead on one of the
>> removeDeadBindings() runs, and also never marked as live during such run
>> (otherwise the "live" mark takes a higher priority). Such marks can be
>> added by Environment (which marks values of active expressions as live
>> and values of inactive expressions as dead), Store (which runs a
>> worklist to find live keys and mark their values live, and then find
>> more live keys, etc.), Constraint Manager (which simply marks all of its
>> symbols as dead, as he is not the one to be interested in keeping them
>> alive), or checkers (through checkLiveSymbols() callback).
>> 
>> So in the program state before the assignment, the file descriptor is
>> live, because the store marks it as live, because it is a value stored
>> in a region of a live variable.
>> 
>> And in the state after the assignment, the file descriptor is not
>> present at all, because the checker fails to inform SymbolReaper of this
>> symbol's existence in its checkLiveSymbols() callback, and there are no
>> other sources of such information for the SymbolReaper.

If I understand correctly, the problem is that by the time we run removeDeadBindings, the symbol is not in the state; therefore, we do not qualify it as dead or alive. We just do not know it ever existed. At which point is the symbol removed from the state? It that a byproduct of the store? 

>> 
>> And the assignment itself doesn't instantly trigger the dead symbol
>> removal. It is only triggered once in a while automatically "between"
>> the statements, and the kind of the statement is of little concern.
>> 
>> 

>> SimpleStreamChecker could be fixed with the following callback:
>> 
>> 
>>   void SimpleStreamChecker::checkLiveSymbols(ProgramStateRef State,
>>                                              SymbolReaper &SymReaper)
>> const {
>>     StreamMapTy TrackedStreams = State->get<StreamMap>();
>>     for (StreamMapTy::iterator I = TrackedStreams.begin(),
>>                                E = TrackedStreams.end(); I != E; ++I) {
>>       SymbolRef Sym = I->first;
>>       SymReaper.maybeDead(Sym);
>>     }
>>   }
>> 
>> 
>> However, even if i propose this fix, somebody has to, kind of, update
>> the presentation of writing the checker in 24 hours, and explain all the
>> stuff from this mail in the process, and i guess the checker would no
>> more be as simple as it used to be.
>> 
>> A few other checkers are not suffering from this issue because another
>> entity performs the maybeDead() call for them: for example,
>> MallocChecker adds default bindings on their symbol's symregions, and
>> leaves it to the Store to mark the base symbols as dead. However, i
>> guess they shouldn't rely on the Store for doing this job.
>> 
>> So i've got a few ideas:
>> 
>> 0. Fix all checkers as discussed above.

I do agree with Artem that changing all of the checkers is not a good option. We should try to move away from the model that asks checker writers to write more boilerplate.

>> 1. Scan all symbols in the SymbolManager to find zombie symbols,
>> deprecate the maybeDead() mechanism.
>> 2. Wait until some kind of smart data map takes care of this extra work.

Are you talking about a data structure that keeps track of the data stored by the checkers. Presumably, that could be scanned to find out which symbols are tracked by the checkers? This direction looks the most appealing at this point.

>> 3. Call checkDeadSymbols on exceptional occasions, like bindings and
>> invalidations, explicitly marking the relevant symbols as maybe dead.
>> 
>> 1. and 2. sound most reasonable to me (3. is hacky and we cannot expect
>> to cover all the cases and probably code duplication). Not sure about
>> performance of 1., maybe i'd have a look, but it'd most likely simplify
>> some code. Otherwise i guess we'd have to just 2., unless anybody has
>> anything else in mind or i'm missing something(?)
>> 
>> Best regards,
>> Artem.
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>