[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware abandoned this revision. baloghadamsoftware added a comment. Superseded by D80522 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 ___ cfe-commits

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware added a comment. Alternative approach is D80522 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 265928. baloghadamsoftware marked an inline comment as done. baloghadamsoftware added a comment. `FIXME` and assertions added. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: clang/includ

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware added a comment. The big question to decide: Either we keep `ParamRegion` as a separate region in the class hierarchy and at the few places where `DeclRegion` or `VarRegion` is used and parameters are possible we duplicate the few lines. (Current status.) The other way is to

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

martong added a comment. Thanks for addressing my comments! Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:309 + /// Get the lvalue for a parameter. + Loc getLValue(const Expr *Call, unsigned Index, +const LocationContext *LC

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked 7 inline comments as done. baloghadamsoftware added a comment. In D79704#2042130 , @martong wrote: > Can we find the FunctionDecl if the call happens through a function pointer? > Or in that case do we actually find the VarDecl o

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

martong added a comment. Seems like many changes could be simplified or absolutely not needed in this patch if ParamRegion (or with a better name ParamVarRegion) was derived from VarRegion. Is there any difficulties in that derivation? Comment at: clang/include/clang/StaticA

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 264979. baloghadamsoftware added a comment. SVal explanation extended and tests added for it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: clang/include/clang/StaticAnalyzer/Checkers/S

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

martong added a comment. Wow, this is some nice work and huge effort from all the participants, it was pretty informative to read through. Thanks @baloghadamsoftware and @NoQ for working on this! > This means that we always found a Decl. However, this Decl is not stored but > retrieved always

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware added a comment. In D79704#2040798 , @NoQ wrote: > > It turns out that parameter regions are never created for functions without > > `Decl` because of the first lines in > > `CallEvent::getCalleeAnalysisDeclContext()`. > > Removing the

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 264598. baloghadamsoftware added a comment. Unit test updated to cover all cases. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: clang/include/clang/StaticAnalyzer/Checkers/SValExplainer

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added a comment. In D79704#2040798 , @NoQ wrote: > > It turns out that parameter regions are never created for functions without > > `Decl` because of the first lines in > > `CallEve

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added a comment. > It turns out that parameter regions are never created for functions without > `Decl` because of the first lines in > `CallEvent::getCalleeAnalysisDeclContext()`. Removing these lines is more or less the whole point of your work. CHANGES SINCE LAST ACTION https://revie

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191 +const ParmVarDecl *ParamRegion::getDecl() const { + const Decl *D = getStackFrame()->getDecl(); + baloghadamsoftware wrote: > NoQ wrote: > > baloghadamsoftware wrote: > > > N

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added a comment. In D79704#2038280 , @NoQ wrote: > In D79704#2038257 , @Szelethus wrote: > > > In D79704#2037100

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added a comment. In D79704#2038257 , @Szelethus wrote: > In D79704#2037100 , @NoQ wrote: > > > > The code changes make me feel like we're doing a lot of chore (and make > > > it super easy to forget checking fo

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. Alright, I'm up to speed I think, cheers! In D79704#2037100 , @NoQ wrote: > > The code changes make me feel like we're doing a lot of chore (and make it > > super easy to forget checking for parameters explicitly). > > I wouldn

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added a comment. In D79704#2036581 , @Szelethus wrote: > If something tricky like this came up, the `getDecl` function should just > return with a nullptr, shouldn't it? How far are you willing to push it? For instance, are you ok with, say, `Typed

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. In D79704#2034571 , @NoQ wrote: > In D79704#2032947 , @Szelethus wrote: > > > In D79704#2032271 , @NoQ wrote: > > > > > Blanket reply! `ParamRegion

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191 +const ParmVarDecl *ParamRegion::getDecl() const { + const Decl *D = getStackFrame()->getDecl(); + NoQ wrot

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191 +const ParmVarDecl *ParamRegion::getDecl() const { + const Decl *D = getStackFrame()->getDecl(); + baloghadamsoftware wrote: > NoQ wrote: > > This doesn't work when the callee

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added a comment. Good news: I executed an analysis with all open-source checks enabled for several open-source projects: //BitCoint//, //CURL//, //OpenSSL//, //PostGreS//, //TMux//, //Xerces// using the patched and the non-p

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051 + + const Expr *OriginExpr; + unsigned Index; baloghadamsoftware wrote: > NoQ wrote: > > baloghadamsoftware wrote: > > > We do not use this at all. Ho

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added a comment. In D79704#2032947 , @Szelethus wrote: > In D79704#2032271 , @NoQ wrote: > > > Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not > > necessarily have a corresponding `Decl`.

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 263748. baloghadamsoftware added a comment. Herald added a subscriber: mgorny. Unit test added. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: clang/include/clang/StaticAnalyzer/Checkers

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 263720. baloghadamsoftware added a comment. Updated according to the comments from @NoQ. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: clang/include/clang/StaticAnalyzer/Checkers/SValEx

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736 + const Stmt *CallSite = SFC->getCallSite(); + if (!CallSite) +return std::make_pair(nullptr, UINT_MAX); + --

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked 9 inline comments as done. baloghadamsoftware added a comment. Than you for your reviews, @NoQ and @Szelethus. In D79704#2032271 , @NoQ wrote: > Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not > necessaril

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware added a comment. In D79704#2033669 , @Szelethus wrote: > Yeah, this patch should definitely have unit tests. All similar patches > should. I wonder how I could make unit tests for this. I already looked up the unit tests and found no

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442 + const VarDecl *VD; + if (const auto *VR = + dyn_cast(cast(Sym)->getRegion())) { +VD = cast(VR->getDecl()); + } else if (const auto *

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Yeah, this patch should definitely have unit tests. All similar patches should. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D7970

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. In D79704#2032271 , @NoQ wrote: > Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not > necessarily have a corresponding `Decl`. For instance, when calling a > function through an unknown function pointer, yo

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

NoQ added a comment. Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not necessarily have a corresponding `Decl`. For instance, when calling a function through an unknown function pointer, you don't have the declaration of a function, let alone its parameters. But for paramet

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. In D79704#2029305 , @baloghadamsoftware wrote: > Thank you for quickly looking into this. > > In D79704#2029230 , @Szelethus wrote: > > > - What identifies a `MemRegion`, `TypedValueRegio

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180 if (const auto *DeclReg = Reg->getAs()) { if (isa(DeclReg->getDecl())) Reg = C.getState()->getSVal(SV.castAs()).getAsRegion(); + } else if (const auto *ParamReg

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked 5 inline comments as done. baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180 if (const auto *DeclReg = Reg->getAs()) { if (isa(DeclReg->getDecl())) Reg = C.getState()->getSVal

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 263204. baloghadamsoftware added a comment. Thank you for your comments, @balazske. You are completely right, I updated the patch now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: cla

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

balazske added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:403 /// frame. May fail; returns null on failure. - const VarRegion *getParameterLocation(unsigned Index, -unsigned BlockCoun

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware updated this revision to Diff 263154. baloghadamsoftware added a comment. Minor fix to get rid of a warning, some comments added. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79704/new/ https://reviews.llvm.org/D79704 Files: clang/include/clang/StaticAnalyzer/Chec

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051 + + const Expr *OriginExpr; + unsigned Index; We do not use this at all. However,

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware marked 2 inline comments as done. baloghadamsoftware added a comment. Thank you for quickly looking into this. In D79704#2029230 , @Szelethus wrote: > - What identifies a `MemRegion`, `TypedValueRegion`s in particular? Why are > parame

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

Szelethus added a comment. > Currently, parameters of functions without their definition present cannot be > represented as regions because it would be difficult to ensure that the same > declaration is used in every case. To overcome this, we introduce a new kind > of region called ParamRegion

[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

baloghadamsoftware created this revision. baloghadamsoftware added reviewers: NoQ, Szelethus. baloghadamsoftware added a project: clang. Herald added subscribers: ASDenysPetrov, martong, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, wh