NoQ added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:438-442
+ Optional<SVal> getReturnValueUnderConstruction(unsigned BlockCount) const;
+
+ Optional<SVal> getArgObject(unsigned Index, unsigned BlockCount) const;
+
+ Optional<SVal> getReturnObject(unsigned BlockCount) const;
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > I believe these functions don't need to be optional. There's always a
> > parameter location and a location to return to (even if the latter is
> > incorrect due to unimplemented construction context, it'll still be some
> > dummy temporary region that you can use).
> Do you mean we should return the original `SVal` if parameter or return value
> location fails? (Thus `LazyCompoundVal` in worst case?
It simply should not fail.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
+class ParamWithoutVarRegion : public TypedValueRegion {
+ friend class MemRegionManager;
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77229/new/
https://reviews.llvm.org/D77229
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
