martong marked 3 inline comments as done.
martong added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:116
+ const bool Foreign = false; // From CTU.
+
----------------
xazax.hun wrote:
> martong wrote:
> > martong wrote:
> > > xazax.hun wrote:
> > > > I feel that we use different terms for the imported declarations.
> > > > Sometimes we call them `new`, sometimes `imported`, sometimes
> > > > `foreign`. In case all of these means the same thing, it would be nice
> > > > to standardize on a single way of naming. If there is a subtle
> > > > difference between them, let's document that in a comment. It would be
> > > > nice if we did not need the comment after the declaration but it would
> > > > be obvious from the variable name.
> > > Yes, I agree that this should deserver some more explanation. Maybe right
> > > above this declaration?
> > >
> > > So, `new` means that a declaration is **created** newly by the
> > > ASTImporter.
> > > `imported` means it has been imported, but not necessarily `new`. Think
> > > about this case, we import `foo`'s definition.
> > > ```
> > > // to.cpp
> > > void bar() {} // from a.h
> > > // from.cpp
> > > void bar() {} // from a.h
> > > void foo() {
> > > bar();
> > > }
> > > ```
> > > Then `foo` will be `new` and `imported`, `bar` will be `imported` and not
> > > `new`.
> > > `foreign` basically means `imported` and `new`.
> > I've just added an explanatory comment for this field.
> Foreign means new and imported. But is there a way for a declaration to be
> new and not to be imported? If no, in that case it feels like new and foreign
> are actually the same and we should standardize on a single name.
Yeah, you are right, `new` and `foreign` are the same in this sense. However, I
think the term `foreign` is more expressive in the sense that is suggests that
the definition is coming from another translation unit.
================
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:
> 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.
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
