dcoughlin added a comment.
Thanks for the patch. It is really great to see these documented!
Who is the target of this documentation? Is it developers of the analyzer or is
it end users of the analyzer? If it is end users, it is unfortunate that we've
been just grabbing examples from the regres
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks!
Do you have commit access, or do you need someone to commit it for you?
https://reviews.llvm.org/D33645
___
cfe-commits mail
dcoughlin added a comment.
Thanks for the patch, and apologies for the delay reviewing!
Here are some initial comments -- there are more coming.
Comment at:
include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:159
+llvm::APFloat To(S, llvm::APFloat::uniniti
dcoughlin added a comment.
This looks like a very solid start!
One area that I'm very worried about is that in various places the analyzer
makes assumptions about algebraic simplifications that don't hold for floating
point because of NaN and other floating point oddities. I think it really
im
dcoughlin added a comment.
Looks good to me, other than a nit on indentation and a request for a little
bit more info in a comment!
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:3394
+ // See if the function has 'rc_ownership_trusted_implementation'
+ // ann
This revision was automatically updated to reflect the committed changes.
Closed by commit rL308242: [analyzer] Add missing documentation for static
analyzer checkers (authored by dcoughlin).
Changed prior to commit:
https://reviews.llvm.org/D33645?vs=106184&id=106989#toc
Repository:
rL LLVM
dcoughlin added a comment.
Committed in r308242. Thanks Dominik!
Repository:
rL LLVM
https://reviews.llvm.org/D33645
___
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/Checkers/RetainCountChecker.cpp:3412
+ if (FD->getDefinition()) {
+canEval = hasRCAnnotation(FD->getDefinition(),
+ "rc_ownership_trusted_implementation");
--
dcoughlin added a comment.
> So, should I submit another patch on the same revision after modifying the
> title, summary, etc. or should I create another revision for that?
You should create another revision. You can mark it as dependent on this one in
Phabricator.
Repository:
rL LLVM
http
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM! One thing I should have noted in a prior review is that the helper
functions should be declared 'static' so that we don't have a public symbol for
them. Making them static will pre
dcoughlin added a comment.
Committed in r308416.
Repository:
rL LLVM
https://reviews.llvm.org/D34937
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL308416: [analyzer] Add annotation attribute to trust retain
count implementation (authored by dcoughlin).
Changed prior to commit:
https://reviews.llvm.org/D34937?vs=107020&id=107243#toc
Repository:
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
dcoughlin added a comment.
This looks pretty good. There are some minor comments in line.
One thing that is missing: tests for the new diagnostic text. Can you add new
tests that specifically test for the new diagnostic text with //
expected-warning {{ ... }}}. These should probably go in retai
dcoughlin added a comment.
I will commit.
Repository:
rL LLVM
https://reviews.llvm.org/D35613
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Sorry for the delay! This looks good to me.
We have a really embarrassing FIXMELATER from 2012 (!!!) that disabled the
plist tests for diagnostics. This means we're not getting testing f
dcoughlin added a comment.
Do we have a radar for this? It sounds familiar.
Comment at: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp:3313
+static bool isNoReturnBlock(const CFGBlock *Blk) {
+ if (Blk->hasNoReturnElement())
Maybe a better name for this w
This revision was automatically updated to reflect the committed changes.
Closed by commit rL308990: [analyzer] Add diagnostic text for generalized
refcount annotations. (authored by dcoughlin).
Changed prior to commit:
https://reviews.llvm.org/D35613?vs=107471&id=108115#toc
Repository:
rL L
dcoughlin added inline comments.
Comment at: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp:3313
+static bool isDominatedByNoReturnBlocks(const ExplodedNode *N) {
+ const CFG &Cfg = N->getCFG();
Do you really mean "is dominated by"? That is, "every path fro
dcoughlin added a comment.
I have some concerns about soundness when the rearrangement may overflow.
Here is an example:
void clang_analyzer_eval(int);
void foo(signed char A, signed char B) {
if (A + 0 >= B + 0) {
clang_analyzer_eval(A - 126 == B + 3); // This yields FALSE with
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D41935
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM with the TODO and the test case I requested inline.
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:487
+if (const MemRegion *MR = I.second.getAsRegion())
+
dcoughlin accepted this revision.
dcoughlin added a comment.
This looks good to me, as well.
https://reviews.llvm.org/D41250
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dcoughlin accepted this revision.
dcoughlin added a comment.
LGTM as well.
https://reviews.llvm.org/D41266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Looks great. It is nice to have this fixed and cleaned up!
Comment at: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp:95
+ llvm::errs() << "PreCall";
+
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM!
Repository:
rC Clang
https://reviews.llvm.org/D41797
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Thanks for adding these! This looks good to me. Do you have commit access, or
do you need someone to commit this?
Repository:
rC Clang
https://reviews.llvm.org/D41881
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks for fixing this.
Repository:
rC Clang
https://reviews.llvm.org/D42015
___
cfe-commits mailing list
cfe-commits@lists.llvm.o
dcoughlin added a comment.
The diagnostic text looks to me, but I do have a comment about the nested 'if'
inline.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:150
+SB.getKnownValue(state, C.getSVal(B->getRHS()));
+if ((unsigned) RHS->getZE
dcoughlin added a comment.
This seems reasonable.
Would it make sense to use the last element of the block edge's source for the
diagnostic location when the destination block is empty?
Repository:
rC Clang
https://reviews.llvm.org/D42266
___
c
dcoughlin accepted this revision.
dcoughlin added a comment.
Looks good to me, thanks!
https://reviews.llvm.org/D41816
___
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:
cfe/trunk/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp:517
+if (const auto *Array = dyn_cast(DeclRef->getType())) {
+ uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8;
+ if (const auto *String = dyn
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
This looks good to me. However, I do think you should take George's suggestion
to have makeZeroElementRegion() have a boolean out parameter rather than a
EvalCallOptions out parameter. T
dcoughlin added a comment.
I think it will be great to have a more explicit representation of construction
in the CFG! Comments in line.
Comment at: include/clang/Analysis/CFG.h:143
+ CXXConstructExpr *Constructor;
+ Stmt *Trigger;
+
I think it would be good
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
This looks good to me, although as I noted inline I think both the name and the
comment for VisitForConstructionContext() are confusing. If you can be more
precise I think it would help
dcoughlin added a comment.
Thanks for updating the tests to be able to run both the z3 and
range-constraint managers! I think this will make it much easier to ensure the
z3-backed manager continues to work as as expected moving forward. I suggested
an alternative approach inline to running the
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Dominic, this (https://reviews.llvm.org/D28952) and
https://reviews.llvm.org/D26061 look get to me! Let's get these two committed!
We'd like to get to a place where in-tree incremental
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM! Thanks!
https://reviews.llvm.org/D30373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
dcoughlin added a comment.
Saving the relevant portion of the build bot log output before it disappears
from the bot:
0.000 [0/1/0] Running all regression tests
-- Testing: 31894 tests, 80 threads --
Traceback (most recent call last):
File "", line 1, in
File "C:\Python27\lib\mult
dcoughlin added a comment.
I'm not sure what is going on.
One thing to try is defining the class in clang/test/lit.cfg -- which seems to
work for Swift. (See
https://github.com/apple/swift/blob/master/test/lit.cfg#L137) We'd still want
to only use that class in Analysis/lit.local.cfg.
Another
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Seems like a safe quick fix for the crash. Looks good to me!
https://reviews.llvm.org/D30499
___
cfe-commits mailing list
cfe-commits@lists
This revision was automatically updated to reflect the committed changes.
Closed by commit rL296646: [analyzer] pr32088: Don't destroy the temporary if
its initializer causes… (authored by dcoughlin).
Changed prior to commit:
https://reviews.llvm.org/D30499?vs=90178&id=90201#toc
Repository:
dcoughlin added a comment.
In https://reviews.llvm.org/D30373#689613, @ddcc wrote:
> With the first method, I'm not sure how to refer to the `AnalyzerTest` class
> defined in `lit.cfg` from `lit.local.cfg`. It doesn't seem to be in scope, so
> unless I store an instantiation in the `config` obj
dcoughlin added a comment.
Thanks for sticking with this!!
This looks good to me, although I would prefer if the definition of the
AnalyzerTest class were moved to the bottom of the lit.cfg file. The other
configuration and substitutions are far more important to the rest of clang --
people sh
dcoughlin added a comment.
Looks great. Thanks!!
https://reviews.llvm.org/D30373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Thanks for fixing this! It looks like this is tracked by PR31835.
https://bugs.llvm.org//show_bug.cgi?id=31835
https://reviews.llvm.org/D30565
___
dcoughlin added a comment.
@zaks.anna: What do you think? Should we try to get this into clang 4.0?
Repository:
rL LLVM
https://reviews.llvm.org/D30565
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/
dcoughlin added reviewers: zaks.anna, NoQ.
dcoughlin added a comment.
The analyzer has two different kinds of diagnostics: AST-based and
path-sensitive. AST-based diagnostics are similar to the diagnostics that clang
performs in Sema in that they can usually be localized to a single program
poi
dcoughlin added a comment.
We add the suppression for libcxx issues in
LikelyFalsePositiveSuppressionBRVisitor in BugReporterVisitors.cpp. It is
ad-hoc pattern recognition of idioms we know cause false positives rather than
an explicit database.
Repository:
rL LLVM
https://reviews.llvm.org
dcoughlin added a comment.
Kevin, would you be willing to file a PR on https://bugs.llvm.org with the 9
false positives you are seeing? This will help us suppress them for users who
don't use 'suppress-c++-stdlib'.
Repository:
rL LLVM
https://reviews.llvm.org/D30593
_
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
LGTM,
https://reviews.llvm.org/D30798
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinf
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
This is great!
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:205
- // We need to be careful about treating a derived type's value as
- // bindings for a base t
dcoughlin added a comment.
PointerToMemberData looks like it is on the right track! One thing that is
still missing is using the base specifier list to figure out the correct
subobject field to read and write from. This is important when there is
non-virtual multiple inheritance, as there will
dcoughlin accepted this revision.
dcoughlin added a comment.
Overall, this looks good to me. Thanks for tackling this!
One request, though. Could you move the tests into already existing test files?
We're trying to avoid test files that only test a single issue. This makes it
easier for people
dcoughlin added a comment.
In https://reviews.llvm.org/D26691#610292, @zaks.anna wrote:
> I am not a big fan of loosing svn blame only to fix formatting, but since you
> are modifying this code anyway, it's fine by me.
>
> Artem and Devin, what is your opinion on this?
I agree that in general
dcoughlin updated this revision to Diff 79981.
dcoughlin added a comment.
- Update the diagnostic to be explicit about whether the issue occurs during
construction or destruction
- Add test for pure, intraprocedural case (Sema also catches this).
https://reviews.llvm.org/D26768
Files:
includ
dcoughlin updated this revision to Diff 79992.
dcoughlin added a comment.
- Add a PureOnly analyzer-config option that, when set, will limit diagnostics
to calls to only pure virtual functions.
This should have gone in with the prevision updated diff; my apologies for the
noise.
https://revie
dcoughlin marked 2 inline comments as done.
dcoughlin added a comment.
In https://reviews.llvm.org/D26768#607157, @zaks.anna wrote:
> Not sure if we should make pure vs not an option so that users could turn the
> checking off. Is there a way to suppress the warning?
There is not a way to supp
dcoughlin added inline comments.
Comment at: include/clang/StaticAnalyzer/Checkers/ObjCRetainCount.h:179
- static RetEffect MakeOwned(ObjKind o, bool isAllocated = false) {
-return RetEffect(isAllocated ? OwnedAllocatedSymbol : OwnedSymbol, o);
+ static RetEffect MakeOw
dcoughlin added a comment.
Looks good to me!
To ease the future maintenance burden I would suggest moving the test into
'retain-release-arc.m' unless it really needs to be in its own separate file.
Comment at: test/Analysis/dispatch-data-leak.m:59
+#endif
+}
dcoughlin added a comment.
In https://reviews.llvm.org/D27409#614601, @NoQ wrote:
> Not sure we need to stay merged with `retain-release-arc.m`, as we're not
> really reusing many declarations across these files.
I find it more helpful to organize tests around functionality ('retain count
che
dcoughlin added a comment.
The analyzer currently doesn't do any checking for dispatch retain/release APIs
in C. It similarly doesn't do any checking in Objective-C when
OS_OBJECT_USE_OBJC is 0 (and thus the dispatch types are defined to their
C-struct versions). This happens when the user expl
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
Ship it! :-)
https://reviews.llvm.org/D27409
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/
dcoughlin added a comment.
This looks great to me other than the idiom I mentioned inline (which is
common, as I have found to my chagrin).
Comment at: lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp:12
+// Currently finds only one kind of issue:
+// - Find autosynthesiz
dcoughlin commandeered this revision.
dcoughlin edited reviewers, added: skomski; removed: dcoughlin.
dcoughlin added a comment.
Commandeering and closing this revision as it has been more than a year.
Repository:
rL LLVM
https://reviews.llvm.org/D13134
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp:44
+ BugReporter &BR) const {
+ if (D->isReadOnly() || D->getSetterKind() != ObjCPropertyDecl::Copy)
+return;
It is pro
dcoughlin added a comment.
I think in the 'null' case it might be better to keep it as "Null passed" or
even "Null value passed". This is different than the 'nil' case because the
diagnostic is not referring to a literal.
https://reviews.llvm.org/D27600
_
dcoughlin added a comment.
I evaluated this checker on three internal codebases that make large use of
virtual functions.
Project 1: ~190,000 lines of C++. 16 alarms. I triaged all of them. There were
2 definite false positives (FPs) and 14 likely FPs.
Project 2: ~320,000 lines of C++. 116 alar
dcoughlin retitled this revision from "[analyzer] Improve VirtualCallChecker
diagnostics and move out of alpha" to "[analyzer] Improve VirtualCallChecker
diagnostics".
dcoughlin updated this revision to Diff 80953.
dcoughlin added a comment.
Keep the diagnostic improvements but remove the change
dcoughlin retitled this revision from "[analyzer] Improve VirtualCallChecker
diagnostics" to "[analyzer] Improve VirtualCallChecker diagnostics and move to
optin package.".
dcoughlin updated the summary for this revision.
dcoughlin updated this revision to Diff 80979.
dcoughlin added a comment.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL289309: [analyzer] Improve VirtualCallChecker diagnostics
and move into optin package. (authored by dcoughlin).
Changed prior to commit:
https://reviews.llvm.org/D26768?vs=80979&id=80984#toc
Repository
dcoughlin added a comment.
In https://reviews.llvm.org/D26768#619222, @malcolm.parsons wrote:
> In https://reviews.llvm.org/D26768#618651, @dcoughlin wrote:
>
> > The definite false positives were cases where the programmer seemed aware
> > of the semantics of virtual calls during construction/d
dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.
The diagnostics look great to me and the macro logic seems reasonable.
Comment at: test/Analysis/diagnostics/macros.cpp:49
+}
\ No newline at end of file
-
dcoughlin added inline comments.
Comment at: include/clang/AST/Decl.h:53
class VarTemplateDecl;
+class CompilerInstance;
Is this needed? It seems like a layering violation.
Comment at: include/clang/AST/Mangle.h:59
+ // the static analyzer.
201 - 274 of 274 matches
Mail list logo