martong marked an inline comment as done.
martong added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:446
+ }
+ const bool BState = State->contains<CTUDispatchBifurcationSet>(D);
+ if (!BState) { // This is the first time we see this foreign function.
----------------
xazax.hun wrote:
> martong wrote:
> > xazax.hun wrote:
> > > xazax.hun wrote:
> > > > martong wrote:
> > > > > xazax.hun wrote:
> > > > > > So if we see the same foreign function called in multiple contexts,
> > > > > > we will only queue one of the contexts for the CTU. Is this the
> > > > > > intended design?
> > > > > >
> > > > > > So if I see:
> > > > > > ```
> > > > > > foreign(true);
> > > > > > foreign(false);
> > > > > > ```
> > > > > >
> > > > > > The new CTU will only evaluate `foreign(true)` but not
> > > > > > `foreign(false)`.
> > > > > This is intentional.
> > > > > ```
> > > > > foreign(true);
> > > > > foreign(false);
> > > > > ```
> > > > > The new CTU will evaluate the following paths in the exploded graph:
> > > > > ```
> > > > > foreign(true); // conservative evaluated
> > > > > foreign(false); // conservative evaluated
> > > > > foreign(true); // inlined
> > > > > foreign(false); // inlined
> > > > > ```
> > > > > The point is to keep bifurcation to a minimum and avoid the
> > > > > exponential blow up.
> > > > > So, we will never have a path like this:
> > > > > ```
> > > > > foreign(true); // conservative evaluated
> > > > > foreign(false); // inlined
> > > > > ```
> > > > >
> > > > > Actually, this is the same strategy that we use during the dynamic
> > > > > dispatch of C++ virtual calls. See `DynamicDispatchBifurcationMap`.
> > > > >
> > > > > The conservative evaluation happens in the first phase, the inlining
> > > > > in the second phase (assuming the phase1 inlining option is set to
> > > > > none).
> > > > > The new CTU will evaluate the following paths in the exploded graph:
> > > > > ```
> > > > > foreign(true); // conservative evaluated
> > > > > foreign(false); // conservative evaluated
> > > > > foreign(true); // inlined
> > > > > foreign(false); // inlined
> > > > > ```
> > > >
> > > > When we encounter `foreign(true)`, we would add the decl to
> > > > `CTUDispatchBifurcationSet`. So the second time we see a call to the
> > > > function `foreign(false);`, we will just do the conservative evaluation
> > > > and will not add the call to the CTU worklist. So how will
> > > > `foreign(false);` be inlined in the second pass? Do I miss something?
> > > >
> > > Oh, I think I now understand. Do you expect `foreign(false);` to be
> > > inlined after we return from `foreign(true);` in the second pass? Sorry
> > > for the confusion, in that case it looks good to me.
> > > Oh, I think I now understand. Do you expect `foreign(false);` to be
> > > inlined after we return from `foreign(true);` in the second pass?
> >
> > Yes, that's right.
> Actually, in this case I wonder whether we really need a set of declarations.
> Wouldn't a boolean be sufficient for each path?
>
> So in case of:
> ```
> foreign1(true);
> foreign2(false);
> ```
>
> Why would we want to add `foreign2` to the CTU worklist? More specifically,
> why is it important whether a foreign call is actually the same declaration
> that we saw earlier on the path? Isn't being foreign the only important
> information in this case?
Good point.
By using the set of declarations we might have a path where `foreign1` is
conservatively evaluated and then `foreign2` is inlined. I was having a hard
time to find any use-cases where this would be useful, but could not find one.
On the other hand, by using a `bool`, as you suggest, no such superfluous path
would exist. Plus, the dependent child patch (D123784) is becoming unnecessary
because the CTUWorklist will have at most one element during the first phase.
Besides, I've made measurements comparing the `bool` and the `set`
implementation. The results are quite similar. Both have lost the same amount
of bugreports compared to the single-tu analysis and the average length of the
bugpaths are also similar. {F23090628}
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123773/new/
https://reviews.llvm.org/D123773
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
