[cfe-dev] CFRefCount Problem #4: Hybrid GC

[Jordy Rose]

(comments inline and at the end)

On Aug 30, 2011, at 20:16, Ted Kremenek wrote:

> 
> On Aug 29, 2011, at 11:49 PM, Jordy Rose wrote:
> 
>> Ah. I was thinking something like this:
>> 
>> 1. GC Disabled
>> a) create CheckerManager and register checkers
>> b) run non-path-sensitive analyses
>> c) run path-sensitive analyses
>> 2. GC Enabled
>> a) create CheckerManager and register checkers
>> b) run non-path-sensitive analyses
>> c) run path-sensitive analyses
>> 
> 
> Let's call this plan A.
> 
>> It sounds like you're thinking:
>> 
>> 1. Create CheckerManager and register checkers
>> 2. Mgr->setGCEnabled(true)
>> a) run non-path-sensitive analyses
>> b) run path-sensitive analyses
>> 3. GC Enabled
>> a) run non-path-sensitive analyses
>> b) run path-sensitive analyses
> 
> Let's call this plan B.
> 
>> 
>> ...and then checkers check if GC is enabled every time it's interesting, instead of once at registration time.
> 
> That's clearer.  I think your plan sounds cleaner (plan A).  Couple concerns:
> 
> (1) I still want checkers to be written so that if they want GC-specific logic, they don't need to be written as a separate checker.  For the Retain/Release checker, we can still keep conditional behavior just by passing the "GC enabled" flag to the checker at construction time.

The only thing I'm a little concerned about (again) is that if the flag lives on the CheckerManager it's not easily accessible (you have to go through the AnalysisManager). Is a shortcut in CheckerContext good enough? Not all callbacks include CheckerContexts, but many do.


> (2) While I'm fine with checkers being run multiple times, we're still going to want to merge duplicate diagnostics.
> 
> For (2), I'm thinking we need a step 0.  e.g.:
> 
> 0. Create a single PathDiagnosticClient that batches all results and uniques them.  Use this PathDiagnosticClient for everything.  We can put this logic into the PathDiagnosticClient base class.
> 1. GC Disabled
> a) create CheckerManager and register checkers
> b) run non-path-sensitive analyses
> c) run path-sensitive analyses
> 2. GC Enabled
> a) create CheckerManager and register checkers
> b) run non-path-sensitive analyses
> c) run path-sensitive analyses
> 
> Currently BugReporter does some uniquing of reports, and that's fine to keep (it serves a different purpose), but I think we'll want an easy way to unique all the diagnostics from all checkers.  That also can be used as a mechanism to solve the problem of printing all diagnostics in SourceLocation order to the terminal.
> 
> Of course, we can add step 0 later, but we need to account for it in the design.
> 
> What do you think?

It seems easy enough to move the PathDiagnosticClient from the AnalysisManager to the AnalysisConsumer. Currently we do something like this:

AnalysisConsumer -> [Translation Unit] -> AnalysisManager/CheckerManager/PDC -> [Decl] -> [GC/non-GC] -> ExprEngine

...but if the same analysis consumer was used for multiple TUs, it'll break anyway, because it only ever creates one PathDiagnosticClient. Except that's not how ASTConsumers work.

What I'm working with looks something like this:

AnalysisConsumer -> PDC -> [TU] -> [GC/non-GC] -> AnalysisManager/CheckerManager -> [Decl] -> ExprEngine

...and that should make it easy to have the PathDiagnosticClient manage multiple analysis runs. Right? (Really the PDC should still be below the TU, but it's easier just to change ownership to the AnalysisConsumer and not change how it's created.)

I didn't think about duplicate bugs before because none of our tests check for non-retain-count errors under hybrid GC. But I can't actually reproduce it, either before or after my changes.

int duplicate_bugs (int x) {
   if (x) return 0;
   return 5 / x;
}

This generates one warning whether I have -fobjc-gc, -fobjc-gc-only, or neither. Which is what we want, but I wonder where the uniquing's happening.

What is different is if I have, say, a "use-after-release" for a CF object, which has a different description in GC vs. non-GC but the same bug type. I'm not sure that's wrong though -- in the case of anything that does discriminate on GC, even just to provide different descriptions, there's an implication that one error or the other could appear on its own. (Which it could, e.g. if the release was performed with CFMakeCollectable.)

Anyway, here's a possible implementation?

Jordy

-------------- next part --------------
A non-text attachment was scrubbed...
Name: HybridGC-AnalysisConsumerPlan.patch
Type: application/octet-stream
Size: 10391 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110831/3b90e2d5/attachment.obj>