This revision was automatically updated to reflect the committed changes.
Closed by commit rL314975: [analyzer] Fix leak false positives on stuff put in
C++/ObjC initializer lists. (authored by dergachev).
Changed prior to commit:
https://reviews.llvm.org/D35216?vs=117699=117785#toc
dcoughlin added a comment.
In https://reviews.llvm.org/D35216#888506, @NoQ wrote:
> Do you think this patch should be blocked in favor of complete modeling?
Please, let's get this landed. We can add more precise modeling when the need
arises.
https://reviews.llvm.org/D35216
NoQ added a comment.
In https://reviews.llvm.org/D35216#886212, @rsmith wrote:
> This is precisely how the rest of the compiler handles
> `CXXStdInitializerListExpr`
Wow. Cool. I'd see what I can do. Yeah, it seems that this is a great case for
us to pattern-match the implementations as well
NoQ added inline comments.
Comment at: test/Analysis/objc-for.m:328
for (id key in array)
clang_analyzer_eval(0); // expected-warning{{FALSE}}
}
I guess these old tests should be replaced with `warnIfReached()`.
https://reviews.llvm.org/D35216
NoQ updated this revision to Diff 117699.
NoQ added a comment.
Herald added a subscriber: szepet.
Escape into array and dictionary literals, add relevant tests. Fix the null
statement check.
https://reviews.llvm.org/D35216
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
NoQ marked 3 inline comments as done.
NoQ added a comment.
In https://reviews.llvm.org/D35216#886212, @rsmith wrote:
> This is precisely how the rest of the compiler handles
> `CXXStdInitializerListExpr`
Wow. Cool. I'd see what I can do. Yeah, it seems that this is a great case for
us to
rsmith added a comment.
In https://reviews.llvm.org/D35216#856415, @NoQ wrote:
> We already have the object's fields in the AST, with its AST record layout,
> however field names and layouts are implementation-dependent, and it is
> unsafe to try to understand how the specific standard library
dcoughlin accepted this revision.
dcoughlin added inline comments.
This revision is now accepted and ready to land.
Comment at: test/Analysis/objc-boxing.m:66
+ BoxableStruct bs;
+ bs.str = strdup("dynamic string"); // The duped string shall be owned by val.
+ NSValue *val =
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1127
+// only consist of ObjC objects, and escapes of ObjC objects
+// aren't so important (eg., retain count checker ignores them).
+if (isa(Ex) ||
dcoughlin
dcoughlin added a comment.
Artem: Sorry it too me so long to review this! For CXXStdInitializerListExpr
this looks great. However, I don't think we want ObjCBoxedExprs to escape their
arguments. It looks to me like these expressions copy their argument values and
don't hold on to them.
NoQ updated this revision to Diff 116512.
NoQ added a comment.
Fix some comments in tests.
Devin: ok to commit? I hope i didn't miss anything in my reasoning about boxed
expressions.
https://reviews.llvm.org/D35216
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
NoQ added a comment.
In https://reviews.llvm.org/D35216#856093, @rsmith wrote:
> The `CXXStdInitializerListExpr` node has pretty simple evaluation semantics:
> it takes a glvalue array expression, and constructs a
> `std::initializer_list` from it as if by filling in the two members with a
>
rsmith added a comment.
The `CXXStdInitializerListExpr` node has pretty simple evaluation semantics: it
takes a glvalue array expression, and constructs a `std::initializer_list`
from it as if by filling in the two members with a pointer to the array and
either the size of the array or a
NoQ updated this revision to Diff 113101.
NoQ added a comment.
Fix a bit of ObjCBoxedExpr behavior.
https://reviews.llvm.org/D35216
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/initializer.cpp
test/Analysis/objc-boxing.m
Index: test/Analysis/objc-boxing.m
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1104
+// expression classes separately.
+if (!isa(Ex))
+ for (auto Child : Ex->children()) {
dcoughlin wrote:
> What is special about ObjCBoxedExpr here?
dcoughlin added a comment.
Other than my question above, this looks good to me.
https://reviews.llvm.org/D35216
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1104
+// expression classes separately.
+if (!isa(Ex))
+ for (auto Child : Ex->children()) {
What is special about ObjCBoxedExpr here? Naively I would
NoQ updated this revision to Diff 56.
NoQ added a comment.
Actually update the diff.
https://reviews.llvm.org/D35216
Files:
lib/StaticAnalyzer/Core/ExprEngine.cpp
test/Analysis/initializer.cpp
Index: test/Analysis/initializer.cpp
NoQ marked an inline comment as done.
NoQ added a comment.
We've discussed it in person with Devin, and he provided more points to think
about:
- If the initializer list consists of non-POD data, constructors of list's
objects need to take the sub-region of the list's region as `this`-region
NoQ added a comment.
In https://reviews.llvm.org/D35216#814124, @dcoughlin wrote:
> In this case, I would be fine with some sort of AbstractStorageMemoryRegion
> that meant "here is a memory region and somewhere reachable from here exists
> another region of type T". Or even multiple regions
dcoughlin added a comment.
In this case, I would be fine with some sort of AbstractStorageMemoryRegion
that meant "here is a memory region and somewhere reachable from here exists
another region of type T". Or even multiple regions with different identifiers.
This wouldn't specify how the
NoQ added a comment.
> Approach (2): We could teach the Store to scan itself for bindings to
> metadata-symbolic-based regions during scanReachableSymbols() whenever a
> region turns out to be reachable. This requires no work on checker side, but
> it sounds performance-heavy.
Nope, this
NoQ added a comment.
These are some great questions, i guess it'd be better to discuss them more
openly. As a quick dump of my current mood:
- To me it seems obvious that we need to aim for a checker API that is both
simple //and// powerful. This can probably by keeping the API as powerful as
xazax.hun added a comment.
At this point, I am a bit wondering about two questions.
- When should something belong to a checker and when should something belong to
the engine? Sometimes we model library aspects in the engine and model language
constructs in checkers.
- What is the checker
NoQ added a comment.
Or maybe it wasn't a good idea to make two diffs in one place. Dunno.
Comment at: test/Analysis/temporaries-callback-order.cpp:28
// CHECK: Bind
-// CHECK-NEXT: RegionChanges
Apparently, this was caused by the check if the states are
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:810
+public:
+ CollectReachableSymbolsCallback(ProgramStateRef State) {}
+ const InvalidatedSymbols () const { return Symbols; }
alexshap wrote:
> explicit ?
This code was moved from
NoQ updated this revision to Diff 105897.
NoQ edited the summary of this revision.
NoQ added a comment.
Herald added a subscriber: mgorny.
This other diff implements approach (1) through a draft of a checker (that
doesn't model much yet). I had to additionally make sure we already have a
region
alexshap added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:810
+public:
+ CollectReachableSymbolsCallback(ProgramStateRef State) {}
+ const InvalidatedSymbols () const { return Symbols; }
explicit ?
https://reviews.llvm.org/D35216
NoQ created this revision.
I've seen a few false positives that appear because we construct C++11
`std::initializer_list` objects with brace initializers, and such construction
is not properly modeled. For instance, if a new object is constructed on the
heap only to be put into a
29 matches
Mail list logo
