Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL247532: [Static Analyzer] Merge the Objective-C Generics
Checker into Dynamic Type… (authored by xazax).
Changed prior to commit:
http://reviews.llvm.org/D12381?vs=34606=34651#toc
Repository:
rL LLVM
http://reviews.llvm.org/D12381
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
cfe/trunk/test/Analysis/generics.m
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -53,7 +53,6 @@
ObjCAtSyncChecker.cpp
ObjCContainersASTChecker.cpp
ObjCContainersChecker.cpp
- ObjCGenericsChecker.cpp
ObjCMissingSuperCallChecker.cpp
ObjCSelfInitChecker.cpp
ObjCUnusedIVarsChecker.cpp
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -7,11 +7,23 @@
//
//===--===//
//
+// This file contains two checkers. One helps the static analyzer core to track
+// types, the other does type inference on Obj-C generics and report type
+// errors.
+//
+// Dynamic Type Propagation:
// This checker defines the rules for dynamic type gathering and propagation.
//
+// Generics Checker for Objective-C:
+// This checker tries to find type errors that the compiler is not able to catch
+// due to the implicit conversions that were introduced for backward
+// compatibility.
+//
//===--===//
#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/Builtins.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
@@ -24,25 +36,83 @@
using namespace clang;
using namespace ento;
+// ProgramState trait - The type inflation is tracked by DynamicTypeMap. This is
+// an auxiliary map that tracks more information about generic types, because in
+// some cases the most derived type is not the most informative one about the
+// type parameters. This types that are stored for each symbol in this map must
+// be specialized.
+REGISTER_MAP_WITH_PROGRAMSTATE(TypeParamMap, SymbolRef,
+ const ObjCObjectPointerType *)
+
namespace {
class DynamicTypePropagation:
public Checker< check::PreCall,
check::PostCall,
check::DeadSymbols,
-check::PostStmt,
-check::PostStmt > {
+check::PostStmt,
+check::PostStmt,
+check::PreObjCMessage,
+check::PostObjCMessage > {
const ObjCObjectType *getObjectTypeForAllocAndNew(const ObjCMessageExpr *MsgE,
CheckerContext ) const;
/// \brief Return a better dynamic type if one can be derived from the cast.
const ObjCObjectPointerType *getBetterObjCType(const Expr *CastE,
CheckerContext ) const;
+
+ ExplodedNode *dynamicTypePropagationOnCasts(const CastExpr *CE,
+ ProgramStateRef ,
+ CheckerContext ) const;
+
+ mutable std::unique_ptr ObjCGenericsBugType;
+ void initBugType() const {
+if (!ObjCGenericsBugType)
+ ObjCGenericsBugType.reset(
+ new BugType(this, "Generics", categories::CoreFoundationObjectiveC));
+ }
+
+ class GenericsBugVisitor : public BugReporterVisitorImpl {
+ public:
+GenericsBugVisitor(SymbolRef S) : Sym(S) {}
+
+void Profile(llvm::FoldingSetNodeID ) const override {
+ static int X = 0;
+ ID.AddPointer();
+ ID.AddPointer(Sym);
+}
+
+PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext ,
+ BugReport ) override;
+
+ private:
+// The tracked symbol.
+SymbolRef Sym;
+ };
+
+ void reportGenericsBug(const ObjCObjectPointerType *From,
+ const ObjCObjectPointerType *To, ExplodedNode *N,
+ SymbolRef Sym, CheckerContext ,
+ const Stmt *ReportedNode = nullptr) const;
+
+ void checkReturnType(const ObjCMessageExpr *MessageExpr,
+ const ObjCObjectPointerType *TrackedType, SymbolRef Sym,
+
Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
xazax.hun added a comment. In http://reviews.llvm.org/D12381#241709, @xazax.hun wrote: > There are several fallouts from this review, so I decided to split this patch > up the following way: > > 1. I created a patch to incorporate the result of this review into > ObjCGenericsChecker: http://reviews.llvm.org/D12701 > 2. I will created a separate patch to purge the dynamic type information from > the GDM for dead symbols. The second patch is available here: http://reviews.llvm.org/D12767 > 3. Once the former two patch is accepted I will rebase this patch on the top > of those, so this will only contain minimal changes required for the merge. http://reviews.llvm.org/D12381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
xazax.hun marked 2 inline comments as done. xazax.hun added a comment. There are several fallouts from this review, so I decided to split this patch up the following way: 1. I created a patch to incorporate the result of this review into ObjCGenericsChecker: http://reviews.llvm.org/D12701 2. I will created a separate patch to purge the dynamic type information from the GDM for dead symbols. 3. Once the former two patch is accepted I will rebase this patch on the top of those, so this will only contain minimal changes required for the merge. http://reviews.llvm.org/D12381 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
xazax.hun marked an inline comment as not done.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:81
@@ +80,3 @@
+initBugType();
+SmallString<64> Buf;
+llvm::raw_svector_ostream OS(Buf);
zaks.anna wrote:
> How do we know that the string is big enough?
When the string is not big enough, there will be an allocation. So this is not
a correctness issue. However I checked that, and the error messages tend to be
very long. I could either increase the size of this smallstring to something
like 150 which should be enough for the common of the cases, or I could just
make it a string. Which one do you prefer?
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:179
@@ -178,3 @@
- // We only track dynamic type info for regions.
- const MemRegion *ToR = C.getSVal(CastE).getAsRegion();
- if (!ToR)
zaks.anna wrote:
> This line used to be unconditional and now, it's only executed if we are
> casting between ObjC Types.
It should not be a problem. The code bellow only executes for bitcasts, and
getBetterObjCType only returns a valid value, when the cast was between Obj-C
types.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:407
@@ +406,3 @@
+// Clean up the states stored by the generics checker.
+void DynamicTypePropagation::checkDeadSymbols(SymbolReaper ,
+ CheckerContext ) const {
zaks.anna wrote:
> Do you know if the info tracked by the DynamicTypeInfo checker gets cleaned
> up for dead symbols?
That information is stored in DynamicTypeMap which is populated in
lib/StaticAnalyzer/Core/ProgramState.cpp
I could not find any cleanup code. What do you think, what would be the best
way to do the cleanup. Exposing a removeDynamicTypeInfo method from the
ProgramState does not seem to be elegant, but it would work.
http://reviews.llvm.org/D12381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
zaks.anna added a comment.
Partial review in-line.
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:456
@@ -455,2 +455,3 @@
def ObjCGenericsChecker : CheckerObjCGenerics,
HelpTextCheck for incorrect usages of parameterized types.,
+ DescFileDynamicTypePropagation.cpp;
We need a better description here this one is too vague. Maybe something like
this one:
Check for type errors when using Objective-C generics.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:17
@@ -11,1 +16,3 @@
//
+// Generics Checker:
+// This checker tries to find type errors that the compiler is not able to
catch
Mention ObjC here.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:45
@@ +44,3 @@
+class DynamicTypePropagation
+: public Checkercheck::PreCall, check::PostCall, check::PreObjCMessage,
+ check::PostObjCMessage, check::DeadSymbols,
nit: Let's go back to the formatting with a single line per callback.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:48
@@ +47,3 @@
+ check::PostStmtCastExpr, check::PostStmtCXXNewExpr {
+ mutable std::unique_ptrBugType BT;
+
Better name please, we might add more bug types.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:76
@@ +75,3 @@
+
+ void reportGenericsBug(const ObjCObjectPointerType *From,
+ const ObjCObjectPointerType *To, ExplodedNode *N,
Please, move the definitions outside to reduce clutter.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:81
@@ +80,3 @@
+initBugType();
+SmallString64 Buf;
+llvm::raw_svector_ostream OS(Buf);
How do we know that the string is big enough?
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:83
@@ +82,3 @@
+llvm::raw_svector_ostream OS(Buf);
+OS Incompatible pointer types assigning to ';
+QualType::print(To, Qualifiers(), OS, C.getLangOpts(), llvm::Twine());
The error message does not sounds like a proper sentence..
Assigning from 'A' to 'B' sounds more natural.
Are we only warning on assignments? Maybe converting would be more applicable
in some contexts..
Something like this might be better: Conversion from value of type 'Integer'
to incompatible type 'String'
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:105
@@ -43,1 +104,3 @@
+
+ DefaultBool CheckGenerics;
};
Doxygen comment please.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:179
@@ -178,3 @@
- // We only track dynamic type info for regions.
- const MemRegion *ToR = C.getSVal(CastE).getAsRegion();
- if (!ToR)
This line used to be unconditional and now, it's only executed if we are
casting between ObjC Types.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:407
@@ +406,3 @@
+// Clean up the states stored by the generics checker.
+void DynamicTypePropagation::checkDeadSymbols(SymbolReaper SR,
+ CheckerContext C) const {
Do you know if the info tracked by the DynamicTypeInfo checker gets cleaned up
for dead symbols?
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:448
@@ -251,9 +447,3 @@
- // Get the old and new types.
- const ObjCObjectPointerType *NewTy =
- CastE-getType()-getAsObjCObjectPointerType();
- if (!NewTy)
-return nullptr;
- QualType OldDTy = C.getState()-getDynamicTypeInfo(ToR).getType();
- if (OldDTy.isNull()) {
-return NewTy;
+/// Get the most derived class if From that do not loose information about type
+/// parameters. To has to be a subclass of From. From has to be specialized.
Please, use doxygen.
Also, the comment is not clear.. Ex: most derived class if From
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:456
@@ +455,3 @@
+
+static bool storeWhenMoreInformative(ProgramStateRef State, SymbolRef Sym,
+ const ObjCObjectPointerType *const *Old,
There s nothing about picking more informative type here.. especially with
respect to informative term used in the previous function..
http://reviews.llvm.org/D12381
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.
xazax.hun created this revision. xazax.hun added reviewers: krememek, zaks.anna, jordan_rose, dcoughlin. xazax.hun added a subscriber: cfe-commits. This patch merged the functionality from ObjCGenericsChecker into DynamicTypePropagation checker. Note that the Generics Checker can still be turned on or off individually but this only affects whether diagnostics are generated or not. There is no intended functional change in this patch. Rationale: This is a refactoring that makes some collaboration between the ObjCGenericsChecker and DynamicTypePropagation checker possible. The final goal (which will be achieved by some followup patches) is to use the reasoning of ObjCGenericsChecker about generics to infer dynamic type for objects. Lets consider the following scenario: id object = arrayOfStrings.firstObject; Here the DynamicTypePropagation checker can not infer any dynamic type information because the static type of the arrayOfStrings.firstObject expression is id when arrayOfStrings is not annotated. However the generics checker might be able to infer an upper bound (NSString *) for the same expression from the usage of the symbol. In a follow up patch when the DynamicTypePropagation checker fails to infer the dynamic type for an object, we would fall back to the inference of the generics checker, because it may have additional information. Impact: When an exact type is inferred as a dynamic type (this happens when the allocation was visible by the analyzer), method calls on that object will be devirtualized (inlined). When the allocation is not visible but an upper bound can be inferred, there will be a path split on method calls. On one path the method will be inlined (when a body is available) on the other, there will be no inlining. There are some heuristic cases, where an upper bound is treated as an exact type. The expected effect of the follow up patch is that, upper bound can be inferred more frequently. Due to cross translation unit limits, this might not change the inlining behavior significantly. However there are other advantages. Utilizing this dynamic type information, a generic type checker could be implemented trivially which could catch errors like: id object = myNSNumber; NSString *myString = object; Why not keep the Generics checker and the dynamic type propagation as completely separate checks? One of the problem is that, right now both checkers infer type information from casts. In order to be able to fallback to the type inference of Generics checker when the type inference of dynamic type propagation failed, the Generics checker should assume that the dynamic type propagation is done already by the time its callback is invoked. Since there is no way to specify ordering between the checkers right now, the only way to do it correctly is to merge two checks into one checker. What do you think? http://reviews.llvm.org/D12381 Files: lib/StaticAnalyzer/Checkers/CMakeLists.txt lib/StaticAnalyzer/Checkers/Checkers.td lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp test/Analysis/generics.m Index: test/Analysis/generics.m === --- test/Analysis/generics.m +++ test/Analysis/generics.m @@ -418,7 +418,7 @@ // CHECK:keydescription/keystringIncompatible pointer types assigning to apos;NSArraylt;NSNumber *gt; *apos; from apos;NSArraylt;NSString *gt; *apos;/string // CHECK:keycategory/keystringCore Foundation/Objective-C/string // CHECK:keytype/keystringGenerics/string -// CHECK:keycheck_name/keystringalpha.osx.cocoa.ObjCGenerics/string +// CHECK:keycheck_name/keystringcore.DynamicTypePropagation/string // CHECK: keyissue_context_kind/keystringfunction/string // CHECK: keyissue_context/keystringincompatibleTypesErased/string // CHECK: keyissue_hash/keystring2/string @@ -528,7 +528,7 @@ // CHECK:keydescription/keystringIncompatible pointer types assigning to apos;NSString *apos; from apos;NSNumber *apos;/string // CHECK:keycategory/keystringCore Foundation/Objective-C/string // CHECK:keytype/keystringGenerics/string -// CHECK:keycheck_name/keystringalpha.osx.cocoa.ObjCGenerics/string +// CHECK:keycheck_name/keystringcore.DynamicTypePropagation/string // CHECK: keyissue_context_kind/keystringfunction/string // CHECK: keyissue_context/keystringincompatibleTypesErased/string // CHECK: keyissue_hash/keystring3/string @@ -672,7 +672,7 @@ // CHECK:keydescription/keystringIncompatible pointer types assigning to apos;NSArraylt;NSNumber *gt; *apos; from apos;NSArraylt;NSString *gt; *apos;/string // CHECK:keycategory/keystringCore Foundation/Objective-C/string // CHECK:keytype/keystringGenerics/string -// CHECK:keycheck_name/keystringalpha.osx.cocoa.ObjCGenerics/string +// CHECK:keycheck_name/keystringcore.DynamicTypePropagation/string // CHECK:
