[cfe-commits] r170625 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/ include/clang/StaticAnalyzer/Core/PathSensitive/ lib/StaticAnalyzer/Checkers/ lib/StaticAnalyzer/Core/
Jordan Rose
jordan_rose at apple.com
Wed Dec 19 18:35:07 PST 2012
Yay!
On Dec 19, 2012, at 16:38 , Anna Zaks <ganna at apple.com> wrote:
> Author: zaks
> Date: Wed Dec 19 18:38:25 2012
> New Revision: 170625
>
> URL: http://llvm.org/viewvc/llvm-project?rev=170625&view=rev
> Log:
> [analyzer] Add the pointer escaped callback.
>
> Instead of using several callbacks to identify the pointer escape event,
> checkers now can register for the checkPointerEscape.
>
> Converted the Malloc checker to use the new callback.
> SimpleStreamChecker will be converted next.
>
> Modified:
> cfe/trunk/include/clang/StaticAnalyzer/Core/Checker.h
> cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
> cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h
> cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
> cfe/trunk/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
> cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
> cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/CheckerManager.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/ProgramState.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
>
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h?rev=170625&r1=170624&r2=170625&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/CheckerManager.h Wed Dec 19 18:38:25 2012
> @@ -310,11 +310,27 @@
> /// by a call.
> ProgramStateRef
> runCheckersForRegionChanges(ProgramStateRef state,
> - const StoreManager::InvalidatedSymbols *invalidated,
> + const InvalidatedSymbols *invalidated,
> ArrayRef<const MemRegion *> ExplicitRegions,
> ArrayRef<const MemRegion *> Regions,
> const CallEvent *Call);
>
> + /// \brief Run checkers when pointers escape.
> + ///
> + /// This notifies the checkers about pointer escape, which occurs whenever
> + /// the analzyer cannot track the symbol any more. For example, as a
Typo: analzyer.
> + /// result of assigning a pointer into a global or when it's passed to a
> + /// function call the analyzer cannot model.
> + ///
> + /// \param State The state at the point of escape.
> + /// \param Escaped The list of escaped symbols.
> + /// \param Call The corresponding CallEvent, if the symbols escape as
> + /// parameters to the given call.
Nitpicking: should this be indented to match either "Call" or "The"?
> + /// \returns Checkers can modify the state by returning a new one.
> + ProgramStateRef runCheckersForPointerEscape(ProgramStateRef State,
> + const InvalidatedSymbols &Escaped,
> + const CallEvent *Call);
Weird indentation. Can you cheat by putting the return type on the previous line?
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h?rev=170625&r1=170624&r2=170625&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h Wed Dec 19 18:38:25 2012
> @@ -213,13 +213,25 @@
>
> ProgramStateRef killBinding(Loc LV) const;
>
> - /// invalidateRegions - Returns the state with bindings for the given regions
> - /// cleared from the store. The regions are provided as a continuous array
> - /// from Begin to End. Optionally invalidates global regions as well.
> + /// \brief Returns the state with bindings for the given regions
> + /// cleared from the store.
> + ///
> + /// Optionally invalidates global regions as well.
> + ///
> + /// \param Regions the set of regions to be invalidated.
> + /// \param E the expression that caused the invalidation.
> + /// \param BlockCount the current basic block count.
This sounds like it refers to the number of basic blocks (as if we're generating more of them) rather than the number of times the current block has been visited. (I wish we weren't exposing this all over the place; something like "path uniquer" is what it's used for.)
> + /// \param ResultsInPointerEscape the flag is set to true when
> + /// the invalidation is due to escape of a symbol (representing a pointer).
> + /// For example, due to it being passed as an argument in a call.
This very much confused me, so I went to look at what passes 'true'. It turns out the comment is right and the name is wrong: this should be CausedByPointerEscape, not ResultsInPointerEscape.
Mild additional points:
- I'd prefer AddressEscape to PointerEscape, since it includes things passed through C++ references.
- The comment was also confusing because it looks like "symbol" is important, but it isn't. I think this can be true even if you pass the address of a VarRegion to a function.
> + /// \param IS the set of invalidated symbols.
> + /// \param If Call is non-null, the invalidated regions were directly
> + /// invalidated by the call - as parameters.
Cause and effect seem backwards here. (Also, missing the param name.) "\param Call if non-null, the invalidated regions represent parameters to the call and should be considered directly invalidated"
> ProgramStateRef invalidateRegions(ArrayRef<const MemRegion *> Regions,
> const Expr *E, unsigned BlockCount,
> const LocationContext *LCtx,
> - StoreManager::InvalidatedSymbols *IS = 0,
> + bool ResultsInPointerEscape,
> + InvalidatedSymbols *IS = 0,
> const CallEvent *Call = 0) const;
>
> /// enterStackFrame - Returns the state for entry to the given stack frame,
> @@ -395,7 +407,8 @@
> invalidateRegionsImpl(ArrayRef<const MemRegion *> Regions,
> const Expr *E, unsigned BlockCount,
> const LocationContext *LCtx,
> - StoreManager::InvalidatedSymbols &IS,
> + bool ResultsInSymbolEscape,
> + InvalidatedSymbols &IS,
> const CallEvent *Call) const;
> };
>
>
> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h?rev=170625&r1=170624&r2=170625&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h (original)
> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h Wed Dec 19 18:38:25 2012
> @@ -35,6 +35,8 @@
> class ProgramStateManager;
> class ScanReachableSymbols;
>
> +typedef llvm::DenseSet<SymbolRef> InvalidatedSymbols;
> +
This is a silly time to nitpick about this, but InvalidatedSymbolSet? I'm not a fan of plural class names...they sound like variables.
> + /// \brief Called when pointers escape.
> + ///
> + /// This notifies the checkers about pointer escape, which occurs whenever
> + /// the analzyer cannot track the symbol any more. For example, as a
Same typo.
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=170625&r1=170624&r2=170625&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Wed Dec 19 18:38:25 2012
> @@ -199,7 +199,7 @@
>
> ProgramStateRef
> ExprEngine::processRegionChanges(ProgramStateRef state,
> - const StoreManager::InvalidatedSymbols *invalidated,
> + const InvalidatedSymbols *invalidated,
> ArrayRef<const MemRegion *> Explicits,
> ArrayRef<const MemRegion *> Regions,
> const CallEvent *Call) {
> @@ -1588,6 +1588,111 @@
> }
> }
>
> +namespace {
> +class CollectReachableSymbolsCallback : public SymbolVisitor {
> + InvalidatedSymbols Symbols;
> +public:
> + CollectReachableSymbolsCallback(ProgramStateRef State) {}
> + const InvalidatedSymbols &getSymbols() const { return Symbols; }
> +
> + bool VisitSymbol(SymbolRef Sym) {
> + Symbols.insert(Sym);
> + return true;
> + }
> +};
> +} // end anonymous namespace
> +
> +/// Call PointerEscape callback when a value escapes as a result of bind.
> +/// A value escapes in three possible cases:
> +/// (1) we are binding to something that is not a memory region.
> +/// (2) we are binding to a memregion that does not have stack storage
> +/// (3) we are binding to a memregion with stack storage that the store
> +/// does not understand.
Doc comments belong in headers, and this one could certainly be cleaned up a bit. This also applies to all the comments within ExprEngine::processPointerEscapedOnBind.
> +
> +/// Call PointerEscape callback when a value escapes as a result of
> +/// region invalidation.
Ditto (for the header bit).
ALL STYLE COMMENTS. Nice! (and sorry)
More information about the cfe-commits
mailing list