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&id=117785#toc
Reposit
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
test/Analysis/initial
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 patt
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 wr
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
test/Analysis/initializer.cp
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
> p
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 pointe
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? Naivel
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 hav
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 In
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 wi
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 memor
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 appro
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 progr
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 equ
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:810
+public:
+ CollectReachableSymbolsCallback(ProgramStateRef State) {}
+ const InvalidatedSymbols &getSymbols() const { return Symbols; }
alexshap wrote:
> explicit ?
This code was
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 &getSymbols() const { return Symbols; }
explicit ?
https://reviews.llvm.org/
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 brace-initiali
29 matches
Mail list logo
