[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > baloghadamsoftware wrote:
> > > > > NoQ wrote:
> > > > > > There should be only one way to express the parameter region. Let's call this one simply `ParamRegion` or something like that, and `assert(!isa<ParmVarDecl>(D))` in the constructor of `VarRegion`.
> > > > > Do you mean that we should use `ParamRegion` in every case, thus also when we have the definitioan for the function? I wonder whether it breaks too many things.
> > > > This will surely not work. The common handling of `ParamVarDecl` and `VarDecl` is soo deeply rooted in the whole analyzer that separating them means creation of a totally new analyzer engine from scratch.
> > > More specifically: whenever a function is inlined, its parameters are used as variables via `DeclRefExpr`s. A `DeclRefExpr` refers to a `Decl` which is a `ParamVarDecl` but that has reference neither for the `CallExpr` (since it is not related to the call, but to the `FunctionDecl` or `ObjCMethodDecl`) nore for its `Index` in the call. The former is the real problem that cannot be solved even on theoretical level: a function which is inlined cannot depend on the different `CallExpr`s where it is called. Even worse, if the function is analyzed top-level it has not `CallExpr` at all so using `ParamRegion` for its parameters is completely impossible.
> > > A `DeclRefExpr` refers to a `Decl` which is a `ParamVarDecl` but that has reference neither for the `CallExpr` (since it is not related to the call, but to the `FunctionDecl` or `ObjCMethodDecl`)
> > 
> > The call site is available as part of the current location context.
> > 
> > > nore for its Index in the call
> > 
> > It's available as part of `ParmVarDecl`.
> > 
> > > The former is the real problem that cannot be solved even on theoretical level: a function which is inlined cannot depend on the different `CallExpr`s where it is called
> > 
> > It already depends on the `CallExpr`. `ParmVarDecl`-based `VarRegion`s are different for different call sites even if `ParmVarDecl` is the same; moreover, they reside in non-overlapping memory spaces.
> > 
> > > Even worse, if the function is analyzed top-level it has not `CallExpr` at all so using `ParamRegion` for its parameters is completely impossible.
> > 
> > Ok, let's make an exception for this case and either use the old `VarRegion` (given that there's no redecl confusion in this case) or allow the `CallExpr` to be null (it's still trivially easy to extract all the necessary information).
> > 
> > > The common handling of `ParamVarDecl` and `VarDecl` is soo deeply rooted in the whole analyzer that separating them means creation of a totally new analyzer engine from scratch.
> > 
> > I don't see any indication of that yet.
> > or allow the `CallExpr` to be `null`
> 
> How do we calculate the type then? Or should we store it explicitly?
> I don't see any indication of that yet.

But I do. Even if I fix most of the crashes almost all the tests fail for different reasons.

But I also have crashes: what to do with functions called through pointers? These calls do not have direct callee, thus determining the type of the parameter based on the call is difficult. If the function has no prototype, then it is impossible. Maybe we should store the type explicitely indeed?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77229/new/

https://reviews.llvm.org/D77229