[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added a comment. Sorry for the wait! Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:134 +else if (I->isUnsigned()) + OS << I->getZExtValue() << ", which is"; +else Please print single quotes around the value. Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:138 + +OS << " larger or equal with the width of type '" + << B->getLHS()->getType().getAsString() << "'."; "equal with the width" -> "equal to the width" Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36737: [analyzer] Store design discussions in docs/analyzer for future use.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thanks! https://reviews.llvm.org/D36737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna accepted this revision. zaks.anna added a comment. Once the comments by @paquette are addressed, LGTM. Thanks! Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:138 + +OS << " larger or equal to the width of type '" + << B->getLHS()->getType().getAsString() << "'."; paquette wrote: > Maybe "greater than or equal to" instead of "larger or equal to" just for > convention? I hear/read that more often, so seeing "larger" is a little weird. > > Minor point though, so if it makes the message too long it doesn't matter. I agree that "greater than or equal to" is better, so let's change to that. Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name
zaks.anna added a comment.
Looks like the need to have the checker name in BugType along with the checker
names not being initialized early enough, leads to worse checker-writer
experience. Is there a way to ensure that the checker names are set at
construction?
Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:131
<< "' inside of critical section";
- auto R = llvm::make_unique(*BlockInCritSectionBugType, os.str(),
ErrNode);
+ if (!BlockInCritSectionBugType)
+BlockInCritSectionBugType.reset(
nit: I prefer to use '{' here since the if body spans multiple lines.
Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:308
if (StOutBound && !StInBound) {
+if (!Filter.CheckCStringOutOfBounds)
+ return state;
This seems to be related to the change in the test, correct? In the future,
please, split changes like this one into their own separate commits.
https://reviews.llvm.org/D37437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+ } else if (StoreSite->getLocation().getAs()) {
+os << "Reach the max loop limit.";
+os << " Assigning a conjured symbol";
+if (R->canPrintPretty()) {
MTC wrote:
> NoQ wrote:
> > This is user-facing text, and users shouldn't know about conjured symbols,
> > and "max" shouldn't be shortened, and i'm not sure what else. I'd probably
> > suggest something along the lines of "Contents of <...> are wiped", but
> > this is still not good enough.
> >
> > Also could you add a test that displays this note? I.e. with
> > `-analyzer-output=text`.
> Thanks for your review.
>
> You are right, whether this information should be displayed to the user is a
> question worth discussing.
I am not convinced that we need to print this information to the user. The
problem here is that it leaks internal implementation details about the
analyzer. The users should not know about "loop limits" and "invalidation" and
most of the users would not even know what this means. I can see how this is
useful to the analyzer developers for debugging the analyzer, but not to the
end user.
https://reviews.llvm.org/D37187
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:651-652
+ } else if (StoreSite->getLocation().getAs()) {
+os << "Reach the max loop limit.";
+os << " Assigning a conjured symbol";
+if (R->canPrintPretty()) {
xazax.hun wrote:
> zaks.anna wrote:
> > MTC wrote:
> > > NoQ wrote:
> > > > This is user-facing text, and users shouldn't know about conjured
> > > > symbols, and "max" shouldn't be shortened, and i'm not sure what else.
> > > > I'd probably suggest something along the lines of "Contents of <...>
> > > > are wiped", but this is still not good enough.
> > > >
> > > > Also could you add a test that displays this note? I.e. with
> > > > `-analyzer-output=text`.
> > > Thanks for your review.
> > >
> > > You are right, whether this information should be displayed to the user
> > > is a question worth discussing.
> > I am not convinced that we need to print this information to the user. The
> > problem here is that it leaks internal implementation details about the
> > analyzer. The users should not know about "loop limits" and "invalidation"
> > and most of the users would not even know what this means. I can see how
> > this is useful to the analyzer developers for debugging the analyzer, but
> > not to the end user.
> >
> While we might not want to expose this to the user this can be really useful
> to understand what the analyzer is doing when we debugging a false positive
> finding. Maybe it would be worth to introduce a developer mode or verbose
> mode for those purposes. What do you think?
I'd be fine with that in theory, though the downside is that it would pollute
the code a bit. One trick that's often used to better understand a report when
debugging is to remove the path note pruning (by passing a flag). I am not sure
if that approach can be extended for this case. Ex: maybe we could have special
markers on the notes saying that they are for debug purposes only and have the
pruning remove them.
By the way, is this change related to the other change from this patch?
https://reviews.llvm.org/D37187
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33671: [analyzer] In plist alternate mode, don't add weird control flow pieces from ObjC property declarations to uses.
zaks.anna accepted this revision. zaks.anna added inline comments. This revision is now accepted and ready to land. Comment at: lib/StaticAnalyzer/Core/BugReporter.cpp:1674 const Decl *D = CalleeLC->getDecl(); -addEdgeToPath(PD.getActivePath(), PrevLoc, - PathDiagnosticLocation::createBegin(D, SM), - CalleeLC); +if (D->hasBody()) + addEdgeToPath(PD.getActivePath(), PrevLoc, Why does the edge to the end of the function is not drawn either? (I assume it is not.) https://reviews.llvm.org/D33671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added a comment.
These are great additions!
Going back to my comment about adding these to CheckerContext, I think we
should be adding helper functions as methods on CheckerContext as it is **the
primary place where checker writers look for helpers**. Two of the three
methods added take CheckerContext as an argument, so what is the reason for
adding them elsewhere?
As for the svalComparesTo method, if we want to make it available to the two
callbacks that do not have checker context, we can include a method on checker
context that calls into a helper in CheckerHelpers.h. However, given that even
this patch is not using the function, I would argue for leaving it as a helper
function internal to CheckerContext.cpp.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:119
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to negative value on the right "
+ "side of operand";
"right side of operand" does not sound right..
How about:
"The result of the '<<' expression is undefined due to negative value on the
right side of operand"
->
"The result of the left shift is undefined because the right operand is
negative"
or
"The result of the '<<' expression is undefined because the right operand is
negative"
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:126
+ << BinaryOperator::getOpcodeStr(B->getOpcode())
+ << "' expression is undefined due to shift count >= width of type";
+ } else {
It's best not to use ">=" in diagnostic messages.
Suggestions: "due to shift count >= width of type" ->
- "due to shifting by a value larger than the width of type"
- "due to shifting by 5, which is larger than the width of type 'int'" //
Providing the exact value and the type would be very useful and this
information is readily available to us. Note that the users might not see the
type or the value because of macros and such.
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:98
+
+bool clang::ento::svalComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
How about evalComparison as a name for this?
Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:121
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
Please, use doxygen comment style here and below.
Repository:
rL LLVM
https://reviews.llvm.org/D30295
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34102: [analyzer] Add portability package for the checkers.
zaks.anna added inline comments. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:454 +def UnixAPIPortabilityChecker : Checker<"API">, + HelpText<"Finds implementation-defined behavior in UNIX/Posix functions">, + DescFile<"UnixAPIChecker.cpp">; Does this need to be in unix? https://reviews.llvm.org/D34102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34102: [analyzer] Add portability package for the checkers.
zaks.anna added inline comments. Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:454 +def UnixAPIPortabilityChecker : Checker<"API">, + HelpText<"Finds implementation-defined behavior in UNIX/Posix functions">, + DescFile<"UnixAPIChecker.cpp">; NoQ wrote: > zaks.anna wrote: > > Does this need to be in unix? > Well, > 1. the description still says "UNIX/Posix" and > 2. even if we believe that `malloc` isn't a UNIX/Posix-specific thing, we'd > also need to move our `MallocChecker` out of the `unix` package. > > This is probably the right thing to do, but i didn't try to do it here. If we > want this, i'd move the portability checker out of `unix` now and deal with > `MallocChecker` in another patch. I can also move the portability checker's > code into `MallocChecker.cpp`, because that's what we always wanted to do, > according to a `UnixAPIChecker`'s FIXME. If someone is interested in checking if their code is portable, they'd just enable profitability package. I do not think "unix" adds much here. I suggest to just add the check to portability package directly, change the name and make no other changes. WDYT? https://reviews.llvm.org/D34102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27753: [analyzer] alpha.security.DirtyScalar Checker
zaks.anna added a comment. This generally sounds good. Definitely do submit these changes in small pieces! See http://llvm.org/docs/DeveloperPolicy.html#incremental-development for rationale. https://reviews.llvm.org/D27753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33645: [analyzer] Add missing documentation for static analyzer checkers
zaks.anna added a comment. Great cleanup! Some comments below. Comment at: www/analyzer/alpha_checks.html:91 +(C, C++) +Check for logical errors for function calls and Objective-C message +expressions (e.g., uninitialized arguments, null function pointers, for function calls -> in function calls? After briefly looking into this, the checker only reports the use of uninitialized arguments in calls, not the other issues (like null function pointers). Could you double check this? Comment at: www/analyzer/alpha_checks.html:152 +(C, C++, ObjC) +Loss of sign/precision in implicit conversions + sign/precision -> sign or precision Comment at: www/analyzer/available_checks.html:1423 +(C) +This checker isn't a independent checker, it is part of +unix.Malloc with configuration option I cannot find MallocWithAnnotations checker. Also if it exists, it should not be on by default, it should be alpha. (I do not know if anyone is using Optimistic=true and think we should just remove that mode..) Comment at: www/analyzer/available_checks.html:1556 +unix.StdCLibraryFunctions +(C, C++) I do not think we should list "modeling" checkers here. https://reviews.llvm.org/D33645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31029: [analyzer] Fix logical not for pointers with different bit width
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
Looks good with a nit!
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:325
+ Loc makeNull() { return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth()); }
+
Formatting changes here look inconsistent with the rest of the code around.
Repository:
rL LLVM
https://reviews.llvm.org/D31029
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31320: [analyzer] Teach CloneDetection about Qt Meta-Object Compiler
zaks.anna added a comment. Should this revision be "abandoned" or is the plan to iterate on it? Repository: rL LLVM https://reviews.llvm.org/D31320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28953: [analyzer] Eliminate analyzer limitations on symbolic constraint generation
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Core/SValBuilder.cpp:356
QualType ResultTy) {
- if (!State->isTainted(RHS) && !State->isTainted(LHS))
-return UnknownVal();
I am concerned that removing the guard will regress performance in the vanilla
case. (Note that Z3 support as well as taint are not on by default.)
I am curious how much of the regression you've measured could be gained back if
we make this conditional.
Comment at: lib/StaticAnalyzer/Core/SValBuilder.cpp:363
// instead of generating an Unknown value and propagate the taint info to it.
- const unsigned MaxComp = 1; // 10 28X
Reducing the MaxComp is going to regress taint analysis..
> I've updated this revision to account for the recent SVal simplification
> commit by @NoQ,
Which commit introduced the regression?
> but now there is an exponential blowup problem that prevents testcase
> PR24184.cpp from terminating,
What triggers the regression? Removing the if statement above? Does the
regression only effect the Z3 "mode" (I am guessing not since you said "due to
an interaction between Simplifier::VisitNonLocSymbolVal() and
SValBuilder::makeSymExprValNN()")?
https://reviews.llvm.org/D28953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis
zaks.anna added a comment. > -(Anna) Scan-build-py integration of the functionality is nearly finished > (see https://github.com/rizsotto/scan-build/issues/83) (--ctu switch performs > both analysis phases at once). This I think could go in a different patch, > but until we could keep the ctu-build.py and ctu-analyze.py scripts. Do you > agree? It's important to bring this patch into the LLVM repo so that it becomes part of the clang/llvm project and is used. The whole point of adding CTU integration to scan-build-py is to make sure that there is a single tool that all/most users could use; adding the patch to a fork does not accomplish that goal. Also, I am not a fan of developing on downstream branches and that is against the LLVM Developer policy due to all the reasons described here: http://www.llvm.org/docs/DeveloperPolicy.html#incremental-development. This development style leads to fragmentation of the community and the project. Unfortunately, we often see cases where large patches developed out of tree never make it in as a result of not following this policy and it would be great to avoid this in the future. > This I think could go in a different patch, but until we could keep the > ctu-build.py and ctu-analyze.py scripts. Do you agree? It would be best to just add the scan-build-py support to the tree, especially, since the new scrips are not tested. > -(Anna) Dumping the ASTs to the disk. We tried a version where, ASTs are not > dumped in the 1st phase, but is recreated each time a function definition is > needed from an external TU. It works fine, but the analysis time went up by > 20-30% on open source C projects. I am curious which optimization strategies you considered. An idea that @NoQ came up with was to serialize only the most used translation units. Another idea is to choose the TUs that a particular file has most dependencies on and only inline functions from those. What mode would you prefer? Would you pay the 20%-30% in speed but reduce the huge disk consumption? That might be a good option, especially, if you have not exhausted the ideas on how to optimize. > Is it OK to add this functionality in a next patch? Or should we it as an > optional feature right now? This depends on what the plan for going forward is. Specifically, if we do not need the serialization mode, you could remove that from this patch and add the new mode. If you think the serialization mode is essential going forward, we could have the other mode in a separate patch. (It would be useful to split out the serialization mode work into a separate patch so that we could revert it later on if we see that the other mode is better.) I see some changes to the compiler proper, such as ASTImporter, ASTContext, SourceManager. We should split those changes into a separate patch and ask for review from people working on those components. You can point back to this patch, which would contain the analyzer related changes, and state that the other patch is blocking this work. https://reviews.llvm.org/D30691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33645: [analyzer] Add missing documentation for static analyzer checkers
zaks.anna added inline comments. Comment at: www/analyzer/alpha_checks.html:91 +(C, C++) +Check for logical errors for function calls and Objective-C message +expressions (e.g., uninitialized arguments, null function pointers, szdominik wrote: > zaks.anna wrote: > > for function calls -> in function calls? > > After briefly looking into this, the checker only reports the use of > > uninitialized arguments in calls, not the other issues (like null function > > pointers). Could you double check this? > As I see here > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp#L341 > > there are checks for null pointers too. The file implements 2 checkers and only one of them is in alpha: REGISTER_CHECKER(CallAndMessageUnInitRefArg) REGISTER_CHECKER(CallAndMessageChecker) You can search for "CallAndMessageUnInitRefArg" to see that it effects uninitialized parameters warnings. https://reviews.llvm.org/D33645 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34277: [analyzer] Bump default performance thresholds?
zaks.anna added a comment. > Maybe we should introduce another UMK_* for deeper analysis instead? The difference here is not substantial enough to warrant a new level. The general motivation for bumping these numbers is that we've set the timeouts many years ago and the hardware improved quite a bit since then. https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34277: [analyzer] Bump default performance thresholds?
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Once Artem gives more details about the codebase he tested on, I think it would be fine to commit this. We can revert/address concerns later if @a.sidorin or anyone else raises concerns based on further testing on their codebases. @a.sidorin, would this work for you? https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34266: Static Analyzer - Localizability Checker: New Localizable APIs for macOS High Sierra & iOS 11
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thanks! Do you have commit access? Repository: rL LLVM https://reviews.llvm.org/D34266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34102: [analyzer] Add portability package for the checkers.
zaks.anna added a comment. > eg. checkers for portability across linux/bsd should be off on windows by > default, checkers for non-portable C++ APIs should be off in plain C code, etc Is the checker you are moving to portability off and not useful on Windows? https://reviews.llvm.org/D34102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30406: [Analyzer] Add support for displaying cross-file diagnostic paths in HTML output
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM. Thank you! (Not sure if @NoQ wants to do a final review.) Do you have commit access or should we commit on your behalf? https://reviews.llvm.org/D30406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28953: [analyzer] Eliminate analyzer limitations on symbolic constraint generation
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Core/SValBuilder.cpp:356
QualType ResultTy) {
- if (!State->isTainted(RHS) && !State->isTainted(LHS))
-return UnknownVal();
ddcc wrote:
> zaks.anna wrote:
> > I am concerned that removing the guard will regress performance in the
> > vanilla case. (Note that Z3 support as well as taint are not on by default.)
> >
> > I am curious how much of the regression you've measured could be gained
> > back if we make this conditional.
> To clarify, the changes made in this patch aren't specific to Z3 support,
> especially simplifying `SymbolCast` and `IntSymExpr`. With the exception of
> `PR24184.cpp` and `plist-macros.cpp`, all testcases pass with both the
> default and Z3 constraint managers. However, creating additional constraints
> does have performance overhead, and it may be useful to consider the
> parameters for gating this functionality.
>
> On a combined execution (Range + Z3) through the testcases, except the two
> mentioned above, the runtime is 327 sec with this patch applied, and 195 sec
> without this patch applied. On a separate execution through the testcases
> with only the Z3 constraint manager, I get runtimes 320 and 191, respectively.
>
> For testing purposes, I also tried the following code, which has combined
> runtime 311 sec, but loses the accuracy improvements with the Range
> constraint manager on `bitwise-ops.c`, `conditional-path-notes.c`,
> `explain-svals.cpp`, and `std-c-library-functions.c`.
>
> ```
> ConstraintManager &CM = getStateManager().getConstraintManager();
> if (!State->isTainted(RHS) && !State->isTainted(LHS) && !CM.isZ3())
> ```
Thanks for the explanation!
Regressing the current default behavior is my main concern. By looking at the
numbers you provided before (Pre-commit and Post-Commit for
RangeConstraintManager), it seems that this patch will introduce a performance
regression of about 20%, which is large. But you are pointing out that there
are improvements to the modeling as well and those are captured by the updated
tests.
Here are a couple of ideas that came into mind on gaining back performance for
the RangeConstraintManager case:
- Can we drop computing these for some expressions that we know the
RangeConstraintManager will not utilize?
- We could implement the TODO described below and possibly also lower the
MaxComp value. This means that instead of keeping a complex expression and
constraints on every symbol used in that expression, we would conjure a new
symbol and associate a new constrain derived from the expression with it.
(Would this strategy also work for the Z3 case?)
I have to point out that this code is currently only used by taint analysis,
which is experimental and the current MaxComp value is targeted at that. We
would probably need to refine the strategy here if we want to apply this logic
to a general case. It's possible that different MaxComp should be used for
different cases.
It would be valuable to run this on real code and measure how the number of
reports we get differs depending on these values.
https://reviews.llvm.org/D28953
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34277: [analyzer] Bump default performance thresholds?
zaks.anna added a comment. > In particular, there are patches to generate more symbolic expressions, that > could also degrade the performance (but fix some fixmes along the way). The patch you are talking about might be promising, but needs much more investigation and tuning for performance vs issues found. I do not think we should block this patch until the investigation is done. https://reviews.llvm.org/D34277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34266: Static Analyzer - Localizability Checker: New Localizable APIs for macOS High Sierra & iOS 11
zaks.anna added a comment. > Hmm, should there be new tests that demonstrate that we cover the new APIs? Unless we use a new registration mechanism for some of these APIs, I'd be fine without adding a test for every API that has localization constraints. Repository: rL LLVM https://reviews.llvm.org/D34266 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34102: [analyzer] Add portability package for the checkers.
zaks.anna added a comment. This just supports the statement that this particular check should not go under unix. I understand that it will be inconsistent with the name of the malloc checker, which we probably should not change as people might be relying on the package names. I think it's better to have inconsistency than having checks applicable to windows in a package named portability.unix. If there will be checks that need to be in portability and only used for unix, we could create that sub-package later on. https://reviews.llvm.org/D34102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35450: [analyzer] Support generating and reasoning over more symbolic constraint types
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Core/SValBuilder.cpp:364 if (symLHS && symRHS && - (symLHS->computeComplexity() + symRHS->computeComplexity()) < MaxComp) + (symLHS->computeComplexity() + symRHS->computeComplexity()) < MaxComp) return makeNonLoc(symLHS, Op, symRHS, ResultTy); As a follow up to the previous version of this patch, I do not think we should set the default complexity limit to 1. What is the relation between this limit and the limit in VisitNonLocSymbolVal? If they are related, would it be worthwhile turning these into an analyzer option? https://reviews.llvm.org/D35450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36737: [analyzer] Store design discussions in docs/analyzer for future use.
zaks.anna added a comment. I think we should have these is .rst format as this is what the rest of the documentation predominantly uses. (Note, the formatting can be very basic, it's the format that I care about.) Otherwise, great to see this addition! https://reviews.llvm.org/D36737 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37103: [StaticAnalyzer] LoopUnrolling fixes
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:190
+ /* Default = */ false);
+ return shouldUnrollLoops() || explicitlyIncludeLoopExit;
}
I would rather keep this method as is and add the extra logic elsewhere (ex:
the place where includeLoopExitInCFG is used). To me,
AnalyzerOptions::includeLoopExitInCFG() returns the value of the corresponding
parameter and I would not expect it to use anything else.
Comment at: test/Analysis/loop-unrolling.cpp:276
+
+int loopexit_while_empty_loopstack() {
+ if (getNum())
nit: "loop" "exit" and "loop" "stack" are separate words, so consider using "_"
to separate them.
https://reviews.llvm.org/D37103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35450: [analyzer] Support generating and reasoning over more symbolic constraint types
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Core/SValBuilder.cpp:364 if (symLHS && symRHS && - (symLHS->computeComplexity() + symRHS->computeComplexity()) < MaxComp) + (symLHS->computeComplexity() + symRHS->computeComplexity()) < MaxComp) return makeNonLoc(symLHS, Op, symRHS, ResultTy); ddcc wrote: > zaks.anna wrote: > > As a follow up to the previous version of this patch, I do not think we > > should set the default complexity limit to 1. > > > > What is the relation between this limit and the limit in > > VisitNonLocSymbolVal? If they are related, would it be worthwhile turning > > these into an analyzer option? > To clarify, the current version of this patch does not modify the `MaxComp` > parameter. > > My understanding is that `MaxComp` is the upper bound for building a new > `NonLoc` symbol from two children, based on the sum of the number of child > symbols (complexity) across both children. > > In contrast, the limit in `VisitNonLocSymbolVal` (@NoQ, correct me if I'm > wrong), is the upper bound for recursively evaluating and inlining a `NonLoc` > symbol, triggered from `simplifySVal` by `evalBinOpNN`. Note that these two > latter functions indirectly call each other recursively (through > `evalBinOp`), causing the previous runtime blowup. Furthermore, each call to > `computeComplexity`will re-iterate through all child symbols of the current > symbol, but only the first complexity check at the root symbol is actually > necessary, because by definition the complexity of a child symbol at each > recursive call is monotonically decreasing. > > I think it'd be useful to allow both to be configurable, but I don't see a > direct relationship between the two. > To clarify, the current version of this patch does not modify the MaxComp > parameter. I know. Also, currently, it is only used in the unsupported taint tracking mode and only for tainted symbols. I would like a different smaller default for all expressions. https://reviews.llvm.org/D35450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference
zaks.anna added a comment. Is this blocked on the same reasons as what was raised in https://reviews.llvm.org/D35109? https://reviews.llvm.org/D35110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35450: [analyzer] Support generating and reasoning over more symbolic constraint types
zaks.anna added a comment. > But I've never used the taint tracking mode, so I don't know what would be a > reasonable default for MaxComp. that one is very experimental anyway. I'd just keep the functional changes to tain out of this patch and use the current default that taint uses. https://reviews.llvm.org/D35450 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37400: [StaticAnalyzer] Fix failures due to the iteration order of ExplodedNode
zaks.anna added a comment. Thanks for addressing the non-determinism!!! The ExplodedNodeSet is predominantly used to hold very small sets and it is used quite a bit in the analyzer. Maybe we could we use SmallSetVector here instead? https://reviews.llvm.org/D37400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37400: [StaticAnalyzer] Fix failures due to the iteration order of ExplodedNode
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thank you! Anna https://reviews.llvm.org/D37400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37809: [analyzer] PthreadLock: Refactor, use PostCall API. NFC.
zaks.anna added a comment. How about committing the refactor of the code without test modifications. And committing changes to the test separately? https://reviews.llvm.org/D37809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37806: [analyzer] PthreadLock: Fix return values of XNU lock functions.
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:271 } -assert(lockFail && lockSucc); -C.addTransition(lockFail); - +// We might want to handle the case when the mutex lock function was inlined +// and returned an Unknown or Undefined value. This comment is repeated several times... Comment at: test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h:29 +extern void lck_mtx_unlock(lck_mtx_t *); +extern boolean_t lck_mtx_try_lock(lck_mtx_t *); extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp); Should there be a test added that uses the function? https://reviews.llvm.org/D37806 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40668: [Blocks] Inherit sanitizer options from parent decl
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. LGTM. Thanks! I was wondering if there are other places where this propagation needs to be added, for example, other calls to GenerateBlockFunction. https://reviews.llvm.org/D40668 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36067: [analyzer] Create infrastructure for organizing and declaring analyzer configs.
zaks.anna added a comment. >> I tried to keep this as a minimal starting example because this currently >> blocks @yamaguchi 's GSoC project for bash completion. There we want to >> complete the values for -analyzer-config and we currently don't have a good >> way to get a complete list of available configs from the driver :). It is not clear that we want to make the analyzer options user visible. These often used for staging purposes, where we develop a feature or a set of heuristics behind a flag. We do not support changing a lot of these options. https://reviews.llvm.org/D36067 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement
zaks.anna added a comment. > What do you suggest? Should we widen the type of the difference, or abandon > this patch and revert back to the local solution I originally used in the > iterator checker? Does the local solution you used in the iterator checker not have the same problem? https://reviews.llvm.org/D35109 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27918: [analyzer] OStreamChecker
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+CheckerContext &C) const {
gamesh411 wrote:
> NoQ wrote:
> > One of the practical differences between `checkPostCall` and `evalCall` is
> > that in the latter you have full control over the function execution,
> > including invalidations that the call causes. Your code not only sets the
> > return value, but also removes invalidations that normally happen. Like,
> > normally when an unknown function is called, it is either inlined and
> > therefore modeled directly, or destroys all information about any global
> > variables or heap memory that it might have touched. By implementing
> > `evalCall`, you are saying that the only effect of calling `operator<<()`
> > on a `basic_ostream` is returning the first argument lvalue, and nothing
> > else happens; inlining is suppressed, invalidation is suppressed.
> >
> > I'm not sure if it's a good thing. On one hand, this is not entirely true,
> > because the operator changes the internal state of the stream and maybe of
> > some global stuff inside the standard library. On the other hand, it is
> > unlikely to matter in practice, as far as i can see.
> >
> > Would it undermine any of your efforts here if you add a manual
> > invalidation of the stream object and of the `GlobalSystemSpaceRegion`
> > memory space (you could construct a temporary `CallEvent` and call one of
> > its methods if it turns out to be easier)?
> >
> > I'm not exactly in favor of turning this into `checkPostCall()` because
> > binding expressions in that callback may cause hard-to-notice conflicts
> > across multiple checkers. Some checkers may even take the old value before
> > it's replaced. For `evalCall()` we at least have an assert.
> >
> > If you intend to keep the return value as the only effect, there's option
> > of making a synthesized body in our body farm, which is even better at
> > avoiding inter-checker conflicts. Body farms were created for that specific
> > purpose, even though they also have their drawbacks (sometimes `evalCall`
> > is more flexible than anything we could synthesize, eg. D20811).
> >
> > If you have considered all the alternative options mentioned above and
> > rejected them, could you add a comment explaining why? :)
> I am not familiar with the BodyFarm approach, however I tried something
> along the lines of:
> auto CEvt =
> ResultEqualsFirstParam->getStateManager().getCallEventManager().getSimpleCall(CE,
> S, C.getLocationContext());
> ProgramStateRef StreamStateInvalidated =
> CEvt->invalidateRegions(C.blockCount());
>
> It however broke test2 (where the state is set to hex formatting, then, back
> to dec). Any tips why resetting regions could cause problems?
>
I agree that we should not use evalCall(), especially, in an opt-in checker, as
it can introduce subtle/hard to debug issues. As was mentioned, if a checker
implements evalCall(), the usual evaluation by the analyzer core does not
occur, which could lead to various unpredictable results.
https://reviews.llvm.org/D27918
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D158071: [clang] Remove rdar links; NFC
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Looks like @NoQ wetted the remaining code changes. The rest LGTM. Thank you for preparing the patch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158071/new/ https://reviews.llvm.org/D158071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38674: [analyzer] MisusedMovedObjectChecker: More precise warning message
zaks.anna added a comment. Please, commit. https://reviews.llvm.org/D38674 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37437: [analyzer] Fix some checker's output plist not containing the checker name
zaks.anna added a comment. Just to be clear, since this leads to regression to the checker API, I am asking to look into other ways of solving this problem. For example, is there a way to ensure that the checker names are set at construction? https://reviews.llvm.org/D37437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38844: [analyzer] Make issue hash related tests more concise
zaks.anna added a comment.
Please, change the commit description to be more comprehensive.
Comment at: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:68
// (globals should not be invalidated, etc), hence the use of evalCall.
- FnCheck Handler = llvm::StringSwitch(C.getCalleeName(CE))
-.Case("clang_analyzer_eval", &ExprInspectionChecker::analyzerEval)
-.Case("clang_analyzer_checkInlined",
- &ExprInspectionChecker::analyzerCheckInlined)
-.Case("clang_analyzer_crash", &ExprInspectionChecker::analyzerCrash)
-.Case("clang_analyzer_warnIfReached",
- &ExprInspectionChecker::analyzerWarnIfReached)
-.Case("clang_analyzer_warnOnDeadSymbol",
- &ExprInspectionChecker::analyzerWarnOnDeadSymbol)
-.StartsWith("clang_analyzer_explain",
&ExprInspectionChecker::analyzerExplain)
-.StartsWith("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump)
-.Case("clang_analyzer_getExtent",
&ExprInspectionChecker::analyzerGetExtent)
-.Case("clang_analyzer_printState",
- &ExprInspectionChecker::analyzerPrintState)
-.Case("clang_analyzer_numTimesReached",
- &ExprInspectionChecker::analyzerNumTimesReached)
-.Default(nullptr);
+ FnCheck Handler =
+ llvm::StringSwitch(C.getCalleeName(CE))
Unrelated change?
Comment at: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:78
+&ExprInspectionChecker::analyzerWarnOnDeadSymbol)
+ .StartsWith("clang_analyzer_explain",
+ &ExprInspectionChecker::analyzerExplain)
I cannot tell what changed here...
https://reviews.llvm.org/D38844
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38728: [analyzer] Use the signature of the primary template for issue hash calculation
zaks.anna accepted this revision. zaks.anna added a comment. LGTM! https://reviews.llvm.org/D38728 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39269: [Analyzer] [Tests] Do not discard output from CmpRuns.py when running integration tests
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. What kind of output will this start displaying? (I believe currently the script does print the summary of the objects that are added or deleted.) https://reviews.llvm.org/D39269 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38844: [analyzer] Make issue hash related tests more concise
zaks.anna added a comment.
I'd also include some info on how it's now possible to dump the issue hash. You
introduce a new debugging function here "clang_analyzer_hashDump" but it's not
mentioned in the commit message.
Thanks!
Comment at: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:68
// (globals should not be invalidated, etc), hence the use of evalCall.
- FnCheck Handler = llvm::StringSwitch(C.getCalleeName(CE))
-.Case("clang_analyzer_eval", &ExprInspectionChecker::analyzerEval)
-.Case("clang_analyzer_checkInlined",
- &ExprInspectionChecker::analyzerCheckInlined)
-.Case("clang_analyzer_crash", &ExprInspectionChecker::analyzerCrash)
-.Case("clang_analyzer_warnIfReached",
- &ExprInspectionChecker::analyzerWarnIfReached)
-.Case("clang_analyzer_warnOnDeadSymbol",
- &ExprInspectionChecker::analyzerWarnOnDeadSymbol)
-.StartsWith("clang_analyzer_explain",
&ExprInspectionChecker::analyzerExplain)
-.StartsWith("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump)
-.Case("clang_analyzer_getExtent",
&ExprInspectionChecker::analyzerGetExtent)
-.Case("clang_analyzer_printState",
- &ExprInspectionChecker::analyzerPrintState)
-.Case("clang_analyzer_numTimesReached",
- &ExprInspectionChecker::analyzerNumTimesReached)
-.Default(nullptr);
+ FnCheck Handler =
+ llvm::StringSwitch(C.getCalleeName(CE))
xazax.hun wrote:
> zaks.anna wrote:
> > Unrelated change?
> Since I touched this snippet I reformatted it using clang-format. Apart from
> adding a new case before the default all other changes are formatting
> changes. I will revert the formatting changes. So in general, we prefer to
> minimize the diffs over converging to be clang-formatted?
It's much easier to review when the diff does not contain formatting changes
intermixed with functional changes. Looks like you can configure clang-format
to only format the diff.
Repository:
rL LLVM
https://reviews.llvm.org/D38844
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39428: [Analyzer] As suggested, use value storage for BodyFarm
zaks.anna added a comment. > Also if you check the code snippets in the coding standard: > https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto > you can see that where we officially put the references. Correct! The reference should not go with the type name. George, please, address the comments before committing. https://reviews.llvm.org/D39428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39964: Change code owner for Clang Static Analyzer to Devin Coughlin.
zaks.anna created this revision. I've sent the email to cfg-dev and the community is supportive of this change. https://reviews.llvm.org/D39964 Files: CODE_OWNERS.TXT Index: CODE_OWNERS.TXT === --- CODE_OWNERS.TXT +++ CODE_OWNERS.TXT @@ -25,6 +25,10 @@ E: echri...@gmail.com D: Debug Information, inline assembly +N: Devin Coughlin +E: dcough...@apple.com +D: Clang Static Analyzer + N: Doug Gregor E: dgre...@apple.com D: Emeritus owner @@ -41,10 +45,6 @@ E: an...@korobeynikov.info D: Exception handling, Windows codegen, ARM EABI -N: Anna Zaks -E: ga...@apple.com -D: Clang Static Analyzer - N: John McCall E: rjmcc...@apple.com D: Clang LLVM IR generation Index: CODE_OWNERS.TXT === --- CODE_OWNERS.TXT +++ CODE_OWNERS.TXT @@ -25,6 +25,10 @@ E: echri...@gmail.com D: Debug Information, inline assembly +N: Devin Coughlin +E: dcough...@apple.com +D: Clang Static Analyzer + N: Doug Gregor E: dgre...@apple.com D: Emeritus owner @@ -41,10 +45,6 @@ E: an...@korobeynikov.info D: Exception handling, Windows codegen, ARM EABI -N: Anna Zaks -E: ga...@apple.com -D: Clang Static Analyzer - N: John McCall E: rjmcc...@apple.com D: Clang LLVM IR generation ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27740: [analyzer] Include type name in Retain Count Checker diagnostics
zaks.anna updated this revision to Diff 81429.
zaks.anna added a comment.
Devin did not like the '*' in the diagnostic for ObjC objects, so remove the
'*'.
https://reviews.llvm.org/D27740
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/edges-new.mm
test/Analysis/inlining/path-notes.m
test/Analysis/objc-arc.m
test/Analysis/plist-output-alternate.m
test/Analysis/plist-output.m
test/Analysis/retain-release-arc.m
test/Analysis/retain-release-path-notes-gc.m
test/Analysis/retain-release-path-notes.m
Index: test/Analysis/retain-release-path-notes.m
===
--- test/Analysis/retain-release-path-notes.m
+++ test/Analysis/retain-release-path-notes.m
@@ -44,100 +44,100 @@
void creationViaAlloc () {
- id leaked = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id leaked = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void creationViaCFCreate () {
- CFTypeRef leaked = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object with a +1 retain count}}
+ CFTypeRef leaked = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object of type CFTypeRef with a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void acquisitionViaMethod (Foo *foo) {
- id leaked = [foo methodWithValue]; // expected-note{{Method returns an Objective-C object with a +0 retain count}}
+ id leaked = [foo methodWithValue]; // expected-note{{Method returns an instance of id with a +0 retain count}}
[leaked retain]; // expected-note{{Reference count incremented. The object now has a +1 retain count}}
[leaked retain]; // expected-note{{Reference count incremented. The object now has a +2 retain count}}
[leaked release]; // expected-note{{Reference count decremented. The object now has a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void acquisitionViaProperty (Foo *foo) {
- id leaked = foo.propertyValue; // expected-note{{Property returns an Objective-C object with a +0 retain count}}
+ id leaked = foo.propertyValue; // expected-note{{Property returns an instance of id with a +0 retain count}}
[leaked retain]; // expected-note{{Reference count incremented. The object now has a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void acquisitionViaCFFunction () {
- CFTypeRef leaked = CFGetSomething(); // expected-note{{Call to function 'CFGetSomething' returns a Core Foundation object with a +0 retain count}}
+ CFTypeRef leaked = CFGetSomething(); // expected-note{{Call to function 'CFGetSomething' returns a Core Foundation object of type CFTypeRef with a +0 retain count}}
CFRetain(leaked); // expected-note{{Reference count incremented. The object now has a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void explicitDealloc () {
- id object = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id object = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
[object dealloc]; // expected-note{{Object released by directly sending the '-dealloc' message}}
[object class]; // expected-warning{{Reference-counted object is used after it is released}} // expected-note{{Reference-counted object is used after it is released}}
}
void implicitDealloc () {
- id object = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id object = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
[object release]; // expected-note{{Object released}}
[object class]; // expected-warning{{Reference-counted object is used after it is released}} // expected-note{{Reference-counted object is used after it is released}}
}
void overAutorelease () {
- id object = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain cou
[PATCH] D27740: [analyzer] Include type name in Retain Count Checker diagnostics
zaks.anna updated this revision to Diff 81482.
zaks.anna added a comment.
Address Devin's comment regarding 'id'.
https://reviews.llvm.org/D27740
Files:
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
test/Analysis/edges-new.mm
test/Analysis/inlining/path-notes.m
test/Analysis/objc-arc.m
test/Analysis/plist-output-alternate.m
test/Analysis/retain-release-arc.m
test/Analysis/retain-release-path-notes-gc.m
test/Analysis/retain-release-path-notes.m
Index: test/Analysis/retain-release-path-notes.m
===
--- test/Analysis/retain-release-path-notes.m
+++ test/Analysis/retain-release-path-notes.m
@@ -44,12 +44,12 @@
void creationViaAlloc () {
- id leaked = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id leaked = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void creationViaCFCreate () {
- CFTypeRef leaked = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object with a +1 retain count}}
+ CFTypeRef leaked = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object of type CFTypeRef with a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
@@ -68,25 +68,25 @@
}
void acquisitionViaCFFunction () {
- CFTypeRef leaked = CFGetSomething(); // expected-note{{Call to function 'CFGetSomething' returns a Core Foundation object with a +0 retain count}}
+ CFTypeRef leaked = CFGetSomething(); // expected-note{{Call to function 'CFGetSomething' returns a Core Foundation object of type CFTypeRef with a +0 retain count}}
CFRetain(leaked); // expected-note{{Reference count incremented. The object now has a +1 retain count}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
void explicitDealloc () {
- id object = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id object = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
[object dealloc]; // expected-note{{Object released by directly sending the '-dealloc' message}}
[object class]; // expected-warning{{Reference-counted object is used after it is released}} // expected-note{{Reference-counted object is used after it is released}}
}
void implicitDealloc () {
- id object = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id object = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
[object release]; // expected-note{{Object released}}
[object class]; // expected-warning{{Reference-counted object is used after it is released}} // expected-note{{Reference-counted object is used after it is released}}
}
void overAutorelease () {
- id object = [[NSObject alloc] init]; // expected-note{{Method returns an Objective-C object with a +1 retain count}}
+ id object = [[NSObject alloc] init]; // expected-note{{Method returns an instance of NSObject with a +1 retain count}}
[object autorelease]; // expected-note{{Object autoreleased}}
[object autorelease]; // expected-note{{Object autoreleased}}
return; // expected-warning{{Object autoreleased too many times}} expected-note{{Object was autoreleased 2 times but the object has a +1 retain count}}
@@ -99,19 +99,19 @@
}
void makeCollectableIgnored () {
- CFTypeRef leaked = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object with a +1 retain count}}
+ CFTypeRef leaked = CFCreateSomething(); // expected-note{{Call to function 'CFCreateSomething' returns a Core Foundation object of type CFTypeRef with a +1 retain count}}
CFMakeCollectable(leaked); // expected-note{{When GC is not enabled a call to 'CFMakeCollectable' has no effect on its argument}}
NSMakeCollectable(leaked); // expected-note{{When GC is not enabled a call to 'NSMakeCollectable' has no effect on its argument}}
return; // expected-warning{{leak}} expected-note{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
}
CFTypeRef CFCopyRuleViolation () {
- CFTypeRef object = CFGetSomething(); // expected-note{{Call to function 'CFGetSomething' returns a Core
[PATCH] D27753: [analyzer] alpha.security.DirtyScalar Checker
zaks.anna added a comment. > Did you checked if same warnings may be emitted by another checkers? For > example, > ArrayBoundChecker may warn if index is tainted. I second that. The GenericTaintChecker also reports uses of tainted values. It is not clear that we should add a new separate checker instead of adding the missing cases to the existing checkers. Thank you! Anna. https://reviews.llvm.org/D27753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.
zaks.anna added a comment. > I am doing it right now. Unfortunately I found a crash which I fixed, Is it fixed in this patch? > but then it turned out that overwrites of the iterator variable are not > handled. I am working on this > problem. My suggestion is to commit this patch and address the iterator variable overwrites separately, so that it would be more incremental and easier to review. Does this sound good to you? https://reviews.llvm.org/D25660 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.
zaks.anna added a comment. And thank you for the awesome work and addressing the review comments!!! https://reviews.llvm.org/D25660 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27710: [analyzer] Prohibit ExplodedGraph's edges duplicating
zaks.anna added a comment. Are there any negative effects from having the duplicates in edges? When does this occur? It's a bit suspicious since we have a FromN, and a path is split at that node, resulting in successors that are the same. Is it possible that whoever split the state did not encode enough interesting info? Repository: rL LLVM https://reviews.llvm.org/D27710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.
zaks.anna added a comment.
Looks great overall. I have minor comments below. Thanks for the awesome
comments!!!
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:152
+
+ ProgramStateRef State = C.getState();
+
This could be moved up as you are using the state on the previous line.
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:159
+
+ State = assumeValuesEqual(ThisSuccess, BooleanArgVal, State, C);
+ C.addTransition(State);
What happens when they are known not to be equal? I am guessing the transition
is just not added, correct? Do you test for that (I did not check.)?
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:172
+const CXXConstructorCall *Call, CheckerContext &C) const {
+ assert(Call->getNumArgs() > 0);
+
"getNumArgs() == 1" ?
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:209
+
+ if (CtorDecl->getNumParams() == 0)
+return;
It seems that the num of parameter checking logic here is less restrictive than
it could be and that makes this a bit hard to read without looking into the
'model' methods. Ex: I think there are 2 cases:
- Constructor taking a bool can have either 1 or 2 arguments.
- Copy constructor taking exactly one argument.
https://reviews.llvm.org/D27773
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27710: [analyzer] Prohibit ExplodedGraph's edges duplicating
zaks.anna added a comment. To deal with non-determinism, you can sort the results by non-pointer values, such as identifiers, before producing the warnings. It is not clear if you want to print all warnings or only the first one. Is it an option to list all dead symbols in one warning message? Do we support attaching multiple bug reports to the same node? Repository: rL LLVM https://reviews.llvm.org/D27710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27710: [analyzer] Prohibit ExplodedGraph's edges duplicating
zaks.anna added a comment. If this is a common mistake for checker writers, we could consider adding assertions that check for this situation. We should make sure that we do not to add any release builds overhead. Maybe we could put this check behind NDEBUG or ensure that whatever code is added to support the assertion is optimized away. As Artem points out, the checkers in tree do not create a node for every bug reported - there is no reason to do that. Repository: rL LLVM https://reviews.llvm.org/D27710 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker
zaks.anna created this revision.
zaks.anna added a reviewer: dcoughlin.
zaks.anna added subscribers: cfe-commits, dergachev.a.
The checker has several false positives that this patch addresses:
1. Do not check if the return status has been compared to error (or no error)
at the time when leaks are reported since the status symbol might no longer be
alive. Instead, pattern match on the assume and stop tracking allocated symbols
on error paths.
2. The checker used to report error when an unknown symbol was freed. This
could lead to false positives, let's not repot those. This leads to loss of
coverage in double frees.
3. Do not enforce that we should only call free if we are sure that error was
not returned and the pointer is not null. That warning is too noisy and we
received several false positive reports about it. (I removed: "Only call free
if a valid (non-NULL) buffer was returned")
1. Use !isDead instead of isLive in leak reporting. Otherwise, we report leaks
for objects we loose track of. This change triggered change #1.
This also adds checker specific dump to the state.
https://reviews.llvm.org/D28330
Files:
lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
test/Analysis/keychainAPI.m
Index: test/Analysis/keychainAPI.m
===
--- test/Analysis/keychainAPI.m
+++ test/Analysis/keychainAPI.m
@@ -1,13 +1,13 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI %s -verify
+// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI -fblocks %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
// Fake typedefs.
typedef unsigned int OSStatus;
typedef unsigned int SecKeychainAttributeList;
typedef unsigned int SecKeychainItemRef;
typedef unsigned int SecItemClass;
typedef unsigned int UInt32;
-typedef unsigned int CFTypeRef;
-typedef unsigned int UInt16;
typedef unsigned int SecProtocolType;
typedef unsigned int SecAuthenticationType;
typedef unsigned int SecKeychainAttributeInfo;
@@ -77,7 +77,7 @@
void *outData;
st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);
if (st == GenericError)
-SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Only call free if a valid (non-NULL) buffer was returned}}
+SecKeychainItemFreeContent(ptr, outData);
} // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
// If null is passed in, the data is not allocated, so no need for the matching free.
@@ -101,14 +101,6 @@
SecKeychainItemFreeContent(ptr, outData);
}
-void fooOnlyFree() {
- unsigned int *ptr = 0;
- OSStatus st = 0;
- UInt32 length;
- void *outData = &length;
- SecKeychainItemFreeContent(ptr, outData);// expected-warning{{Trying to free data which has not been allocated}}
-}
-
// Do not warn if undefined value is passed to a function.
void fooOnlyFreeUndef() {
unsigned int *ptr = 0;
@@ -220,11 +212,27 @@
if (st == noErr)
SecKeychainItemFreeContent(ptr, outData[3]);
}
- if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
+ if (length) { // TODO: We do not report a warning here since the symbol is no longer live, but it's not marked as dead.
length++;
}
return 0;
-}// no-warning
+}
+
+int testErrorCodeAsLHS(CFTypeRef keychainOrArray, SecProtocolType protocol,
+SecAuthenticationType authenticationType, SecKeychainItemRef *itemRef) {
+ unsigned int *ptr = 0;
+ OSStatus st = 0;
+ UInt32 length;
+ void *outData;
+ st = SecKeychainFindInternetPassword(keychainOrArray,
+ 16, "server", 16, "domain", 16, "account",
+ 16, "path", 222, protocol, authenticationType,
+ &length, &outData, itemRef);
+ if (noErr == st)
+SecKeychainItemFreeContent(ptr, outData);
+
+ return 0;
+}
void free(void *ptr);
void deallocateWithFree() {
@@ -251,7 +259,6 @@
extern const CFAllocatorRef kCFAllocatorNull;
extern const CFAllocatorRef kCFAllocatorUseContext;
CFStringRef CFStringCreateWithBytesNoCopy(CFAllocatorRef alloc, const uint8_t *bytes, CFIndex numBytes, CFStringEncoding encoding, Boolean externalFormat, CFAllocatorRef contentsDeallocator);
-extern void CFRelease(CFStringRef cf);
void DellocWithCFStringCreate1(CFAllocatorRef alloc) {
unsigned int *ptr = 0;
@@ -333,11 +340,11 @@
SecKeychainItemFreeContent(0, pwdBytes);
}
-void radar10508828_2() {
+void radar10508828_20092614() {
UInt32 pwdLen = 0;
void* pwdBytes = 0;
OSStatus rc = SecKeychainFindGenericPassword(0, 3, "foo", 3, "bar", &pwdLen, &pwdBytes, 0);
- SecKeychainItemFreeContent(0, pwdBytes); // expected-warning {{Only call free if a valid (non-NULL) buffer was returned}}
+ SecKeychainItemFreeContent(0, pwdBytes);
}
//Example from bug 10797.
@@ -426,3 +433,24 @@
SecKeychainItemFree
[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker
zaks.anna updated this revision to Diff 83160.
zaks.anna added a comment.
Addressed all comments
https://reviews.llvm.org/D28330
Files:
lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
test/Analysis/keychainAPI.m
Index: test/Analysis/keychainAPI.m
===
--- test/Analysis/keychainAPI.m
+++ test/Analysis/keychainAPI.m
@@ -1,13 +1,13 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI %s -verify
+// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI -fblocks %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
// Fake typedefs.
typedef unsigned int OSStatus;
typedef unsigned int SecKeychainAttributeList;
typedef unsigned int SecKeychainItemRef;
typedef unsigned int SecItemClass;
typedef unsigned int UInt32;
-typedef unsigned int CFTypeRef;
-typedef unsigned int UInt16;
typedef unsigned int SecProtocolType;
typedef unsigned int SecAuthenticationType;
typedef unsigned int SecKeychainAttributeInfo;
@@ -77,7 +77,7 @@
void *outData;
st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);
if (st == GenericError)
-SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Only call free if a valid (non-NULL) buffer was returned}}
+SecKeychainItemFreeContent(ptr, outData);
} // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
// If null is passed in, the data is not allocated, so no need for the matching free.
@@ -101,14 +101,6 @@
SecKeychainItemFreeContent(ptr, outData);
}
-void fooOnlyFree() {
- unsigned int *ptr = 0;
- OSStatus st = 0;
- UInt32 length;
- void *outData = &length;
- SecKeychainItemFreeContent(ptr, outData);// expected-warning{{Trying to free data which has not been allocated}}
-}
-
// Do not warn if undefined value is passed to a function.
void fooOnlyFreeUndef() {
unsigned int *ptr = 0;
@@ -220,11 +212,27 @@
if (st == noErr)
SecKeychainItemFreeContent(ptr, outData[3]);
}
- if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
+ if (length) { // TODO: We do not report a warning here since the symbol is no longer live, but it's not marked as dead.
length++;
}
return 0;
-}// no-warning
+}
+
+int testErrorCodeAsLHS(CFTypeRef keychainOrArray, SecProtocolType protocol,
+SecAuthenticationType authenticationType, SecKeychainItemRef *itemRef) {
+ unsigned int *ptr = 0;
+ OSStatus st = 0;
+ UInt32 length;
+ void *outData;
+ st = SecKeychainFindInternetPassword(keychainOrArray,
+ 16, "server", 16, "domain", 16, "account",
+ 16, "path", 222, protocol, authenticationType,
+ &length, &outData, itemRef);
+ if (noErr == st)
+SecKeychainItemFreeContent(ptr, outData);
+
+ return 0;
+}
void free(void *ptr);
void deallocateWithFree() {
@@ -251,7 +259,6 @@
extern const CFAllocatorRef kCFAllocatorNull;
extern const CFAllocatorRef kCFAllocatorUseContext;
CFStringRef CFStringCreateWithBytesNoCopy(CFAllocatorRef alloc, const uint8_t *bytes, CFIndex numBytes, CFStringEncoding encoding, Boolean externalFormat, CFAllocatorRef contentsDeallocator);
-extern void CFRelease(CFStringRef cf);
void DellocWithCFStringCreate1(CFAllocatorRef alloc) {
unsigned int *ptr = 0;
@@ -333,11 +340,11 @@
SecKeychainItemFreeContent(0, pwdBytes);
}
-void radar10508828_2() {
+void radar10508828_20092614() {
UInt32 pwdLen = 0;
void* pwdBytes = 0;
OSStatus rc = SecKeychainFindGenericPassword(0, 3, "foo", 3, "bar", &pwdLen, &pwdBytes, 0);
- SecKeychainItemFreeContent(0, pwdBytes); // expected-warning {{Only call free if a valid (non-NULL) buffer was returned}}
+ SecKeychainItemFreeContent(0, pwdBytes);
}
//Example from bug 10797.
@@ -426,3 +433,24 @@
SecKeychainItemFreeContent(attrList, outData);
}
+typedef struct AuthorizationValue {
+int length;
+void *data;
+} AuthorizationValue;
+typedef struct AuthorizationCallback {
+OSStatus (*SetContextVal)(AuthorizationValue *inValue);
+} AuthorizationCallback;
+static AuthorizationCallback cb;
+int radar_19196494() {
+ @autoreleasepool {
+AuthorizationValue login_password = {};
+UInt32 passwordLength;
+void *passwordData = 0;
+OSStatus err = SecKeychainFindGenericPassword(0, 0, "", 0, "", (UInt32 *)&login_password.length, (void**)&login_password.data, 0);
+cb.SetContextVal(&login_password);
+if (err == noErr) {
+ SecKeychainItemFreeContent(0, login_password.data);
+}
+ }
+ return 0;
+}
Index: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
@@ -28,7 +28,8 @@
namespace {
class
[PATCH] D28348: Taught the analyzer about Glib API to check Memory-leak
zaks.anna added a comment. Thanks for the patch! Could you, please, resubmit the patch with context as described here http://llvm.org/docs/Phabricator.html Also, please, add tests. See test/Analysis/ for examples. Repository: rL LLVM https://reviews.llvm.org/D28348 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28330: [analyzer] Fix false positives in Keychain API checker
zaks.anna added a comment. I did not think of solution #1! It's definitely better than the pattern matching I've added here. However, this checker fires so infrequently, that I do not think it's worth investing more time into perfecting it. I suspect the solution #2 is what this checker was trying to use to begin with. It marks the return symbol as dependent on the allocated symbol by calling: C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol); However, addSymbolDependency only worked for isLive() and not for !isDead(). Would be good to investigate this further as other checkers such as malloc also use addSymbolDependency. https://reviews.llvm.org/D28330 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28387: [tsan] Do not report errors in __destroy_helper_block_
zaks.anna created this revision.
zaks.anna added a reviewer: kubabrecka.
zaks.anna added a subscriber: cfe-commits.
There is a synchronization point between the reference count of a block
dropping to zero and it's destruction, which TSan does not observe. Do not
report errors in the compiler-emitted block destroy method and everything
called from it.
This is similar to https://reviews.llvm.org/D25857
https://reviews.llvm.org/D28387
Files:
lib/CodeGen/CodeGenFunction.cpp
test/CodeGen/sanitize-thread-no-checking-at-run-time.m
Index: test/CodeGen/sanitize-thread-no-checking-at-run-time.m
===
--- test/CodeGen/sanitize-thread-no-checking-at-run-time.m
+++ test/CodeGen/sanitize-thread-no-checking-at-run-time.m
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o
- %s | FileCheck -check-prefix=WITHOUT %s
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o
- %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -fblocks
-emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -fblocks
-emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
__attribute__((objc_root_class))
@interface NSObject
@@ -26,9 +28,14 @@
}
@end
-// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
-
// TSAN: initialize{{.*}}) [[ATTR:#[0-9]+]]
// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
// TSAN: cxx_destruct{{.*}}) [[ATTR:#[0-9]+]]
+
+void test2(id x) {
+ extern void test2_helper(id (^)(void));
+ test2_helper(^{ return x; });
+// TSAN: define internal void @__destroy_helper_block_(i8*) [[ATTR:#[0-9]+]]
+}
+
// TSAN: attributes [[ATTR]] = { noinline nounwind {{.*}}
"sanitize_thread_no_checking_at_run_time" {{.*}} }
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -705,6 +705,11 @@
return false;
}
+static void markAsIgnoreThreadCheckingAtRuntime(llvm::Function *Fn) {
+ Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
+ Fn->removeFnAttr(llvm::Attribute::SanitizeThread);
+}
+
void CodeGenFunction::StartFunction(GlobalDecl GD,
QualType RetTy,
llvm::Function *Fn,
@@ -748,16 +753,19 @@
Fn->addFnAttr(llvm::Attribute::SafeStack);
// Ignore TSan memory acesses from within ObjC/ObjC++ dealloc, initialize,
- // .cxx_destruct and all of their calees at run time.
+ // .cxx_destruct, __destroy_helper_block_ and all of their calees at run
time.
if (SanOpts.has(SanitizerKind::Thread)) {
if (const auto *OMD = dyn_cast_or_null(D)) {
IdentifierInfo *II = OMD->getSelector().getIdentifierInfoForSlot(0);
if (OMD->getMethodFamily() == OMF_dealloc ||
OMD->getMethodFamily() == OMF_initialize ||
(OMD->getSelector().isUnarySelector() &&
II->isStr(".cxx_destruct"))) {
-Fn->addFnAttr("sanitize_thread_no_checking_at_run_time");
-Fn->removeFnAttr(llvm::Attribute::SanitizeThread);
+markAsIgnoreThreadCheckingAtRuntime(Fn);
}
+} else if (const auto *FD = dyn_cast_or_null(D)) {
+ IdentifierInfo *II = FD->getIdentifier();
+ if (II && II->isStr("__destroy_helper_block_"))
+markAsIgnoreThreadCheckingAtRuntime(Fn);
}
}
Index: test/CodeGen/sanitize-thread-no-checking-at-run-time.m
===
--- test/CodeGen/sanitize-thread-no-checking-at-run-time.m
+++ test/CodeGen/sanitize-thread-no-checking-at-run-time.m
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
-// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -fblocks -emit-llvm -o - %s | FileCheck -check-prefix=WITHOUT %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c++ -fblocks -emit-llvm -o - %s -fsanitize=thread | FileCheck -check-prefix=TSAN %s
+
+// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
__attribute__((objc_root_class))
@interface NSObject
@@ -26,9 +28,14 @@
}
@end
-// WITHOUT-NOT: "sanitize_thread_no_checking_at_run_time"
-
// TSAN: initialize{{.*}}) [[ATTR:#[0-9]+]]
// TSAN: dealloc{{.*}}) [[ATTR:#[0-9]+]]
// TSAN: cxx_destruct{{.*}}) [[ATTR:#[0-9]+]]
+
+void test2(id x) {
+ extern void test2_helper(id (^)(void));
+ test2_helper(^{ return x; });
+// TSAN: define internal void @__destroy_helper_block_(i8*) [[ATTR:#[0-9]+]]
+}
+
// TSAN: attributes [[ATTR]] = { noinline nounwind {{.*}} "s
[PATCH] D28348: [analyzer] Taught the analyzer about Glib API to check Memory-leak
zaks.anna added a comment.
Phabricator still says that context is not available. Please, pass -U when
generating the patch.
Thanks!
Anna
Comment at: test/Analysis/gmalloc.c:1
+// RUN: %clang_cc1 -analyze
-analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection
-analyzer-store=region -verify %s
+
Please, just include core checkers and the Malloc checker. There is no need for
UnreachableCode, CastSize and ExprInspection here.
Comment at: test/Analysis/gmalloc.c:43
+ g_free(g2);
+ g_free(g2); // g2 Double-free
+ g_free(g3);
This test will fail. For example, I am getting:
$
/Volumes/Data/ws/build/Ninja-DebugAssert+cmark-ReleaseAssert/llvm-macosx-x86_64/./bin/clang
-cc1 -internal-isystem
/Volumes/Data/ws/build/Ninja-DebugAssert+cmark-ReleaseAssert/llvm-macosx-x86_64/bin/../lib/clang/4.0.0/include
-nostdsysteminc -analyze -analyzer-checker=osx.SecKeychainAPI,unix.Malloc
-fblocks /Volumes/Data/ws/clang/test/Analysis/gmalloc.c -verify
error: no expected directives found: consider use of 'expected-no-diagnostics'
error: 'warning' diagnostics seen but not expected:
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 43: Attempt to free
released memory
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 70: Use of memory
after it is freed
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 79: Potential leak
of memory pointed to by 'g4'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 82: Potential leak
of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 83: Potential leak
of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 85: Potential leak
of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 87: Potential leak
of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 106: Potential leak
of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 107: Potential leak
of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 109: Potential leak
of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 111: Potential leak
of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 130: Potential leak
of memory pointed to by 'g6'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 131: Potential leak
of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 133: Potential leak
of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 135: Potential leak
of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 155: Potential leak
of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 157: Potential leak
of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 159: Potential leak
of memory pointed to by 'g7'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 179: Potential leak
of memory pointed to by 'g5'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 181: Potential leak
of memory pointed to by 'g8'
File /Volumes/Data/ws/clang/test/Analysis/gmalloc.c Line 183: Potential leak
of memory pointed to by 'g7'
22 errors generated.
The "-verify" option you pass on command line to the test checks that every
warning/analyzer issue matches a comment with a specific pattern:
// expected-warning {"Text of warning here"}
You should run the tests to make sure all of them pass after your patch. If you
build with ninja, you's run "ninja check-clang" if you build with make, you can
follow instructions here
http://clang-analyzer.llvm.org/checker_dev_manual.html#testing
Repository:
rL LLVM
https://reviews.llvm.org/D28348
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28445: [Analyzer] Extend taint propagation and checking
zaks.anna added a comment.
Thanks for working on this!
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:443
+ if (auto LCV = Val.getAs())
+return C.getSymbolManager().getRegionValueSymbol(LCV->getRegion());
+
This might create a new symbol. Is this what we want?
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:694
+ if (const TypedValueRegion *TVR = dyn_cast(Reg)) {
+SymbolRef Sym = getSymbolManager().getRegionValueSymbol(TVR);
+
This might create a new symbol as well. Is this what we want here?
https://reviews.llvm.org/D28445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28495: [analyzer] Support inlining of '[self classMethod]' and '[[self class] classMethod]'
zaks.anna created this revision.
zaks.anna added reviewers: dergachev.a, dcoughlin.
zaks.anna added a subscriber: cfe-commits.
https://reviews.llvm.org/D28495
Files:
lib/StaticAnalyzer/Core/CallEvent.cpp
test/Analysis/inlining/InlineObjCClassMethod.m
Index: test/Analysis/inlining/InlineObjCClassMethod.m
===
--- test/Analysis/inlining/InlineObjCClassMethod.m
+++ test/Analysis/inlining/InlineObjCClassMethod.m
@@ -1,6 +1,7 @@
// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config ipa=dynamic-bifurcate -verify %s
void clang_analyzer_checkInlined(int);
+void clang_analyzer_eval(int);
// Test inlining of ObjC class methods.
@@ -194,7 +195,9 @@
@implementation SelfUsedInParent
+ (int)getNum {return 5;}
+ (int)foo {
- return [self getNum];
+ int r = [self getNum];
+ clang_analyzer_eval(r == 5); // expected-warning{{TRUE}}
+ return r;
}
@end
@interface SelfUsedInParentChild : SelfUsedInParent
@@ -229,8 +232,41 @@
+ (void)forwardDeclaredVariadicMethod:(int)x, ... {
clang_analyzer_checkInlined(0); // no-warning
}
+@end
+
+@interface SelfClassTestParent : NSObject
+-(unsigned)returns10;
++(unsigned)returns20;
++(unsigned)returns30;
+@end
+@implementation SelfClassTestParent
+-(unsigned)returns10 { return 100; }
++(unsigned)returns20 { return 100; }
++(unsigned)returns30 { return 100; }
@end
+@interface SelfClassTest : SelfClassTestParent
+-(unsigned)returns10;
++(unsigned)returns20;
++(unsigned)returns30;
+@end
+@implementation SelfClassTest
+-(unsigned)returns10 { return 10; }
++(unsigned)returns20 { return 20; }
++(unsigned)returns30 { return 30; }
++(void)classMethod {
+ unsigned result1 = [self returns20];
+ clang_analyzer_eval(result1 == 20); // expected-warning{{TRUE}}
+ unsigned result2 = [[self class] returns30];
+ clang_analyzer_eval(result2 == 30); // expected-warning{{TRUE}}
+ unsigned result3 = [[super class] returns30];
+ clang_analyzer_eval(result3 == 100); // expected-warning{{UNKNOWN}}
+}
+-(void)instanceMethod {
+ unsigned result0 = [self returns10];
+ clang_analyzer_eval(result0 == 10); // expected-warning{{TRUE}}
+}
+@end
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -896,6 +896,38 @@
llvm_unreachable("The while loop should always terminate.");
}
+static const ObjCMethodDecl *findDefiningRedecl(const ObjCMethodDecl *MD) {
+ if (!MD)
+return MD;
+
+ // Find the redeclaration that defines the method.
+ if (!MD->hasBody()) {
+for (auto I : MD->redecls())
+ if (I->hasBody())
+MD = cast(I);
+ }
+ return MD;
+}
+
+static bool isCallToSelfClass(const ObjCMessageExpr *ME) {
+ const Expr* InstRec = ME->getInstanceReceiver();
+ if (!InstRec)
+return false;
+ const auto *InstRecIg = dyn_cast(InstRec->IgnoreParenImpCasts());
+
+ // Check that receiver is called 'self'.
+ if (!InstRecIg || !InstRecIg->getFoundDecl() ||
+ !InstRecIg->getFoundDecl()->getName().equals("self"))
+return false;
+
+ // Check that the method name is 'class'.
+ if (ME->getSelector().getNumArgs() != 0 ||
+ !ME->getSelector().getNameForSlot(0).equals("class"))
+return false;
+
+ return true;
+}
+
RuntimeDefinition ObjCMethodCall::getRuntimeDefinition() const {
const ObjCMessageExpr *E = getOriginExpr();
assert(E);
@@ -910,6 +942,7 @@
const MemRegion *Receiver = nullptr;
if (!SupersType.isNull()) {
+ // The receiver is guaranteed to be 'super' in this case.
// Super always means the type of immediate predecessor to the method
// where the call occurs.
ReceiverT = cast(SupersType);
@@ -921,15 +954,35 @@
DynamicTypeInfo DTI = getDynamicTypeInfo(getState(), Receiver);
QualType DynType = DTI.getType();
CanBeSubClassed = DTI.canBeASubClass();
- ReceiverT = dyn_cast(DynType);
+ ReceiverT = dyn_cast(DynType.getCanonicalType());
if (ReceiverT && CanBeSubClassed)
if (ObjCInterfaceDecl *IDecl = ReceiverT->getInterfaceDecl())
if (!canBeOverridenInSubclass(IDecl, Sel))
CanBeSubClassed = false;
}
-// Lookup the method implementation.
+// Handle special cases of '[self classMethod]' and
+// '[[self class] classMethod]', which are treated by the compiler as
+// instance (not class) messages. We will statically dispatch to those.
+if (auto *PT = dyn_cast_or_null(ReceiverT)) {
+ // For [self classMethod], return the compiler visible declaration.
+ if (PT->getObjectType()->isObjCClass() &&
+ Receiver == getSelfSVal().getAsRegion())
+return RuntimeDefinition(findDefiningRedecl(E->getMethodDecl()));
+
+ // Similarly, handle [[self class] classMethod].
+ // TODO: We are currently doing a syntactic match for this pattern with is
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
zaks.anna added a comment. Please, subscribe cfe-commits list on the patches as described in http://llvm.org/docs/Phabricator.html. Thanks! Anna Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31029: [analyzer] Fix logical not for pointers with different bit width
zaks.anna added a comment. Are there other cases where makeNull would need to be replaced? Repository: rL LLVM https://reviews.llvm.org/D31029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
zaks.anna added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46 -} // end GR namespace +bool isExprResultConformsComparisonRule(CheckerContext &C, +BinaryOperatorKind CompRule, NoQ wrote: > Because you are making these functions public, i think it's worth it to make > it obvious what they do without looking at the particular checker code. > Comments are definitely worth it. I think function names could be worded > better; i suggest `exprComparesTo(const Expr *LHSExpr, BinaryOperatorKind > ComparisonOp, SVal RHSVal, CheckerContext &C);` and `isGreaterOrEqual(...)`; > i'm personally fond of having CheckerContext at the back because that's where > we have it in checker callbacks, but that's not important. + 1 on Artem's comment of making the names more clear and providing documentation. Also, should these be part of CheckerContext? Repository: rL LLVM https://reviews.llvm.org/D30295 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31982: [analyzer] Improve suppression for inlined defensive checks when operator& is involved.
zaks.anna added a comment.
Thanks!!!
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:965
+
+// Performing operator `&' on an lvalue expression is essentially a no-op.
+// Then, if we are taking addresses of fields or elements, these are also
NoQ wrote:
> alexshap wrote:
> > "Address-of" operator can be overloaded,
> > just wondering - doest this code work correctly in that case ?
> In this case we'd see a `CXXOperatorCallExpr` instead of `UnaryOperator` (all
> hail clang AST!).
Adding a test case for that would be good.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:968
+// unlikely to matter.
+// FIXME: Currently offsets of fields are computed incorrectly,
+// being always equal to 0. See the FIXME in StoreManager's
Incorrect implies that there is a better "correct" model and invites a fix. Do
we know what better model would be? If so, we could add that to the comment. If
not, I'd prefer explaining why it works this way (similarly to how you did in
the comment below). Maybe adding an example of what does not work. And you
could add a FIXME to say that it's worth investigating if there is a better way
of handling it. (The majority of this info should probably go to Store.cpp)
Also, maybe it's just the choice of words here. "Incorrect" sounds like
something that needs to be corrected. Whereas you could use something like "is
modeled imprecisely with respect to what happens at execution time", which
could still mean that this is how we do want to model it going forward.
It seems that the problem with modeling this way is demonstrated with a test
case in explain-svals.cpp. So the benefits of the current modeling seem to be
worth it.
Can we add a note along the path saying that "p" in "p->f" is null? This would
address the user confusion with imprecise modeling.
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:977
+ Ex = Op->getSubExpr()->IgnoreParenCasts();
+ while (true) {
+if (const auto *ME = dyn_cast(Ex)) {
Why do we need the "while (true)"? Can we just use "dyn_cast(Ex)"
as the loop condition?
Take a look at the getDerefExpr(const Stmt *S) and see if that would be a
better place to add this code. Maybe not..
Comment at: lib/StaticAnalyzer/Core/Store.cpp:405
case loc::ConcreteIntKind:
// While these seem funny, this can happen through casts.
// FIXME: What we should return is the field offset. For example,
Could you rephrase this existing comment while you are here? Using word "funny"
seems content-free and a bit strange.
Comment at: lib/StaticAnalyzer/Core/Store.cpp:409
// like this work properly: &(((struct foo *) 0xa)->f)
+// However, that's not easy to fix without reducing our abilities
+// to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
Thanks for adding the explanation!
Can you think of other cases where the same would apply? (Ex: array index)
https://reviews.llvm.org/D31982
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27090: Add LocationContext as a parameter to checkRegionChanges
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. This needs to be committed. https://reviews.llvm.org/D27090 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. This is done. https://reviews.llvm.org/D27773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28348: [analyzer] Taught the analyzer about Glib API to check Memory-leak
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:810
+if (CE->getNumArgs() == 2)
+ State = ProcessZeroAllocation(C, CE, 1, State);
} else if (CE->getNumArgs() == 3) {
Why did you remove the old behavior here and below?
I would expect this patch to be strictly additive. If gmalloc APIs take a
different number of arguments, please, process them separately. You might need
to factor out the processing code to avoid code duplication.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:838
State = ProcessZeroAllocation(C, CE, 1, State);
+ if (CE->getNumArgs() > 2)
+State = ProcessZeroAllocation(C, CE, 2, State);
Should this be conditional on the number of arguments instead of adding two
calls to ProcessZeroAllocation?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:846
State = ProcessZeroAllocation(C, CE, 0, State);
- State = ProcessZeroAllocation(C, CE, 1, State);
-} else if (FunI == II_free) {
+} else if (FunI == II_free || FunI == II_g_free) {
State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
This change in how calloc is handled broke the Analysis/malloc.c test.
Repository:
rL LLVM
https://reviews.llvm.org/D28348
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna added a comment. I think it's more valuable to report a warning here even if the error message is not very precise. Marking something as in bounds when we know it's not does not feel right and could lead to inconsistent states down the road. Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27202: [analyzer] Do not conjure a symbol for return value of a conservatively evaluated function
zaks.anna added a comment. From what I recall, it is not clear that this patch is the step in the right direction. At least, it need more investigation. https://reviews.llvm.org/D27202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28765: CStringChecker can crash when uninitialized checks are disabled
zaks.anna added a comment. > It is not supported to run the analyzer with some of the core checkers turned > off. Correct. > Maybe we should change the behavior such that turning off core checkers turn > off the warnings from those checkers but not the checkers themselves? Having this as the default behavior for "disable a checker" could be confusing. however, introducing a new flag for **silencing** warnings from a checker sounds fine. What is the motivation for disabling the core checkers in this particular case? https://reviews.llvm.org/D28765 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend
zaks.anna added a comment. Thanks for working on this Dominic!!! Can you talk a bit about your motivation for working on this project and what your goals are? Have you compared the performance when using Z3 vs the current builtin solver? I saw that you mention performance issues on large codebases, but could you give some specific numbers of the tradeoffs between performance, code coverage, and the quality of reported bugs. (One very rough idea to use a stronger solver while still keeping the analyzer performant would be to only use it for false positive refutation.) > I looked around for a generic smt-lib interface earlier, but couldn't find > much available, and since I wanted floating-point arithmetic support, I ended > up just directly using the Z3 C interface (the C++ interface uses exceptions, > which causes problems). As far as I know, CVC4 doesn't have built-in > floating-point support. But longer term, I'd agree that this backend should > be made more generic. Ok. Though I'd love to see an interface that supports CVC4! > Another issue is that some testcases will have different results depending on > the constraint manager in use, but I don't believe it's possible to change > the expected testcase output based on the presence of a feature flag. Unless > this changes, there'll need to be (mostly) duplicate testcases for each > constraint manager. How different the results are? Is it the case that you get more warnings with Z3 in most cases? Would it be possible to divide the tests into the ones that work similarly with both solvers and the ones that are only expected to report warnings with Z3? I know this won't work in general, but might be much cleaner than polluting every test with a ton of #ifdefs. > Furthermore, this and the child patches will cause the static analyzer to > generate more complex constraints at runtime. But, I'm not sure if just > having RangeConstraintManager ignore unsupported constraints will be > sufficient, from a performance and correctness perspective. This is probably the most important question to answer. Maintaining the performance and correctness of the analyzer mode that is using RangeConstraintManager is very important as this is the mode most users will use at least for a while. > My personal preference would be to directly merge in this constraint manager > without using the plugin approach, because it would simplify some of the > testing and changes to the build system (e.g. the APFloat linking issue). But > I'm not sure if this would be ok? Most likely that would be possible. I'd also like to second Ryan's suggestion to look at his patch to add support for STP. It was very close to landing other than some build system integration issues. https://reviews.llvm.org/D28952 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29151: [clang-tidy] Add misc-invalidated-iterators check.
zaks.anna added a comment. The static analyzer is definitely the place to go for bug detection that requires path sensitivity. It's also reasonably good for anything that needs flow-sensitivity. Although, most flow-sensitive analysis (such as liveness) now live in lib/Analysis/, which is used by both Sema and the Clang Static Analyzer. Both path-sensitive and flow-sensitive analysis are based on clang's CFG. I do not know of clang-tidy uses CFG or lib/Analysis/. Here are the wikipedia definitions of sensitivity I am referring to: //- A flow-sensitive analysis takes into account the order of statements in a program. For example, a flow-insensitive pointer alias analysis may determine "variables x and y may refer to the same location", while a flow-sensitive analysis may determine "after statement 20, variables x and y may refer to the same location". - A path-sensitive analysis computes different pieces of analysis information dependent on the predicates at conditional branch instructions. For instance, if a branch contains a condition x>0, then on the fall-through path, the analysis would assume that x<=0 and on the target of the branch it would assume that indeed x>0 holds. // There is work underway in the analyzer for adding IteratorPastEnd checker. The first commit was rather large and has been reviewed here https://reviews.llvm.org/D25660. That checker is very close to be moved out of alpha. Moving it out of alpha is pending evaluation on real codebases to ensure that the false positive rates are low and enhancements to error reporting. > Other problem is that it would be probably a part > of one of the alpha checks, that are not developed and I don't know if they > will ever be fully supported. There seems to be a misconception about what the alpha checkers are. All checkers start in alpha package to allow in-tree incremental development. Once the checkers are complete, they should move out of alpha. The criteria is that the code should pass the review, the checker needs to have low false positive rates, finally, the error reports should be of good quality (some checks greatly benefit from extra path hints that can be implemented with a visitor). These are motivated by providing a good user experience to end users. (We do have several checkers that are "stuck in alpha". A lot of them are abandoned experiments that just need to be deleted; others could probably be made production quality with some extra effort.) Repository: rL LLVM https://reviews.llvm.org/D29151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29151: [clang-tidy] Add misc-invalidated-iterators check.
zaks.anna added a comment. Before clang-tidy came into existence the guidelines were very clear. One should write a clang warning if the diagnostic: - can be implemented without path-insensitive analysis (including flow-sensitive) - is checking for language rules (not specific API misuse) In my view, this should still be the rule going forward because compiler warnings are most effective in reaching users. The Clang Static Analyzer used to be the place for all other diagnostics. Most of the checkers it contains rely on path-sensitive analysis. Note that one might catch the same bug with flow-sensitive as well as path-sensitive analysis. So in some of those cases, we have both warnings as well as analyzer checkers finding the same type of user error. However, the checkers can catch more bugs since they are path-sensitive. The analyzer also has several flow-sensitive/ AST matching checkers. Those checks could not have been written as warnings mainly because they check for APIs, which are not part of the language. My understanding is that clang-tidy supports fixits, which the clang static analyzer currently does not support. However, there could be other benefits to placing not path-sensitive checks there as well. I am not sure. I've also heard opinions that it's more of a linter tool, so the user expectations could be different. > Even right now there are clang-tidy checks that finds subset of alpha checks, > but probably having lower false positive rate. Again, alpha checks are unfinished work, so we should not use them as examples of checkers that have false positives. Some of them are research projects and should probably be deleted. Repository: rL LLVM https://reviews.llvm.org/D29151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29151: [clang-tidy] Add misc-invalidated-iterators check.
zaks.anna added a comment. > I tried to come up with some advice on where checks should go in > http://clang.llvm.org/extra/clang-tidy/#choosing-the-right-place-for-your-check The guidelines stated on the clang-tidy page seem reasonable to me. > For example, IMO, AST-based analyses make more sense in clang-tidy for a few > reasons: > > They usually are easier expressible in terms of AST matchers, and clang-tidy > allows to use AST matchers with less boilerplate. > Clang Static Analyzer is linked into clang, where AST matchers are > undesired, since they tend to blow the size of binary significantly. > It's more consistent to keep all similar analyses together, it simplifies > search for already implemented similar functionality and code reviews. I agree that there is a gray area specifically in the AST-matching-based bug finding, which unfortunately leads to duplication of effort. However, we currently cannot ban/move all AST-based checks out of the analyzer. The main problem I see with restricting the AST-based analysis to clang-tidy is impact on the end users. While clang-tidy does export the clang static analyzer results, the reverse does not hold. There are many end users who do not use clang-tidy but do use the clang static analyzer (either directly or through scan-build). Until that is the case, I do not think we should disallow AST based checks from the analyzer, especially if they come from contributors who are interested in contributing to this tool. Note that the format in which the results are presented from these tools is not uniform, which makes transition more complicated. The AST matchers can be used from the analyzer and if the code size becomes a problem, we could consider moving the analyzer out of the clang codebase. Generally, I am not a big fan of the split between the checks based on the technology used to implement them since it does not lead to a great user interface - the end users do not think in terms of path-sensitive/flow-sensitive/AST-matching, they just want to enable specific functionality such as find bugs or ensure they follow coding guidelines. This is why I like the existing guidelines with their focus on what clang-tidy is from user perspective: > clang-tidy check is a good choice for linter-style checks, checks that are > related to a certain coding style, checks that address code readability, etc. Another thing that could be worth adding here is that clang-tidy finds bugs that tend to be easy to fix and, in most cases, provide the fixits. (I believe, this is one of the major strengths of the clang-tidy tool!) > Flow-sensitive analyses (that don't need any path-sensitivity) seem to be > equally suitable for SA and clang-tidy (unless I'm missing something), so I > don't feel strongly as to where they should go. As far as I know, currently, all flow-sensitive analysis are in the Analysis library in clang. These are analysis for compiler warnings and analysis used by the static analyzer. I believe this is where future analysis should go as well, especially, for analysis that could have multiple clients. If clang code size impact turns out to be a serious problem, we could also have that library provide APIs that could be used from other tools/checks. (Note, the analysis could be implemented in the library, but the users of the analysis (checks) can be elsewhere.) Regarding the points about "heuristics" and "flexibility", the analyzer is full of heuristics and fuzzy API matching. These techniques are very common in static analysis in general as we are trying to solve undecidable problems and heuristics is the only way to go. Repository: rL LLVM https://reviews.llvm.org/D29151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D16403: Add scope information to CFG
zaks.anna added a comment. > @dcoughlin As a reviewer of both patches - could you tell us what's the > difference between them? And how are we going to resolve this issue? Unfortunately, @dcoughlin is on vacation this week; should be back next week. Repository: rL LLVM https://reviews.llvm.org/D16403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34508: [Analyzer] Bug Reporter Visitor to Display Values of Variables - PRELIMINARY!
zaks.anna added a comment. I agree that we should not print the values of all variables. The users will be overwhelmed with the huge amount of information. It is very valuable to highlight just the right information. (I believe even the current diagnostics often produce too much info and highlighting the more important notes would be very valuable.) However, the examples you presented are **very compelling**. Are there ways we could highlight the same information without printing values of all variables? For example, for the overflow case, we could experiment with printing notes along the path at the locations a variable overflows. This would be very valuable for the array overflow alpha checker, which often flags the bugs that only occur if an integer value along the path overflows. I am not sure how noisy this approach will be. If it is too noisy, we could refine this further. For the uninitialized variable example, we could have some pattern-matching logic that would check if the expression is an array element and if so print the value of the index. (Another problem with variables that are failed to be initialized in a function is that we do not display the path on which the variable is not initialized, making it hard to understand the reports. Though, that is a separate problem.) https://reviews.llvm.org/D34508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15227: [analyzer] Valist checkers.
zaks.anna added a comment. @xazax.hun, Can we move this out of alpha? Have this checkers been tested on a large codebase? What are false positive rates? Thanks! Anna Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28297: [StaticAnalyzer] Fix crash in CastToStructChecker
zaks.anna added a comment. Please, make sure future reviews go through cfg-dev list. See http://llvm.org/docs/Phabricator.html. Repository: rL LLVM https://reviews.llvm.org/D28297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna added a comment. Does the code you added detects array out of bounds cases without false positives? Is it an option to just have this checkers produce a more precise error message in the specific case. A lot of work will probably need to be done to implement a proper array out of bounds checking and no-one is working on that. Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27753: [analyzer] alpha.security.DirtyScalar Checker
zaks.anna added a comment. > In this checker, I give warnings for values which are both tainted and were > also not checked by the programmer. So unlike GenericTaintChecker, I do > implement the boundedness check here for certain, interesting constructs > (which is controlled by the critical option). GenericTaintChecker focuses > purely on taintedness, almost like a service for other checkers. I've added a > new rule to it, improving the taintedness logic, but I felt mixing the bound > checking logic into it would make the two ideas inseparable. I'd like to step back a bit. In my view, the taint implementation should consist of three elements: 1. taint source 2. taint sink 3. cleansing rules I always considered the taint analysis in the analyzer not fully implemented because #3 was missing. It sounds a lot like non-"dirty" scalars would be the same as values that went through cleansing - they should be considered not tainted anymore. Now, are cleansing rules checker specific or generic? If they are generic, these rules should definitely be part of GenericTaintChecker and every checker using taint would utilize them. WDYT? (We can talk about the array bound checking part separately.) https://reviews.llvm.org/D27753 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28348: [analyzer] Taught the analyzer about Glib API to check Memory-leak
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:885
+return;
+ State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State);
+ State = ProcessZeroAllocation(C, CE, 0, State);
I am not sure this is correct as the third argument is "size of allocation",
which in this case would be the value of CE->getArg(0) times the value of
CE->getArg(2).
The current implementation of MallocMemAux would need to be extended to
incorporate this:
` // Set the region's extent equal to the Size parameter.
const SymbolicRegion *R =
dyn_cast_or_null(RetVal.getAsRegion());
if (!R)
return nullptr;
if (Optional DefinedSize =
Size.getAs()) {
SValBuilder &svalBuilder = C.getSValBuilder();
DefinedOrUnknownSVal Extent = R->getExtent(svalBuilder);
DefinedOrUnknownSVal extentMatchesSize =
svalBuilder.evalEQ(State, Extent, *DefinedSize);
State = State->assume(extentMatchesSize, true);
assert(State);
}`
My suggestion is to submit the patch without the 'n' variants and extend
MallocMemAux to deal with them as a follow up patch.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:889
+} else if (FunI == II_g_realloc_n || FunI == II_g_try_realloc_n) {
+ if (CE->getNumArgs() < 2)
+return;
Should this be 'getNumArgs() < 3' ?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:891
+return;
+ State = ReallocMem(C, CE, false, State);
+ State = ProcessZeroAllocation(C, CE, 1, State);
Unfortunately, ReallocMem also assumes a single size argument:
` // Get the size argument. If there is no size arg then give up.
const Expr *Arg1 = CE->getArg(1);
if (!Arg1)
return nullptr;`
Repository:
rL LLVM
https://reviews.llvm.org/D28348
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29643: [analyzer] Do not duplicate call graph nodes for function that has definition and forward declaration.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Looks good. Thank you for catching this! Do you have commit access or should I commit on your behalf? https://reviews.llvm.org/D29643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29303: In VirtualCallChecker, handle indirect calls
zaks.anna added a comment. Has this been cherry-picked into the clang 4.0 release branch? If not, we should definitely do that! Repository: rL LLVM https://reviews.llvm.org/D29303 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15227: [analyzer] Valist checkers.
zaks.anna added a comment. > But as far as I remember, this produced false negatives in the tests not > false positives. Could you double check that? Maybe you still have some notes in your mail box or just by looking at the code. Did none of the checks work or just some of them? Also, which platforms did this not work on? It would be great to move all of the useful checks out of alpha since alpha essentially means "unsupported" and we do not recommend turning those checkers on. Thanks! Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28348: [analyzer] Taught the analyzer about Glib API to check Memory-leak
zaks.anna added a comment. Could you please split the patch into two - one with the previously reviewed support for functions that take a single size value and another patch that models the two size arguments (num and size). It's easier to review patches if they do not grow new functionality. Splitting the patch would also play nicely with the incremental development policy of LLVM. Thanks! Repository: rL LLVM https://reviews.llvm.org/D28348 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:76
if (Ex) {
+ bool ArrayIndexOutOfBounds = false;
+ if (isa(Ex)) {
Please, pull this out into a sub-rutine. Thanks!
https://reviews.llvm.org/D28278
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30341: [analyzer] clarify error messages about uninitialized function arguments
zaks.anna added a comment.
I agree this can clarify the error message quite a bit!
Comment at: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp:160
if (ParamDecl->getType()->isPointerType()) {
-Message = "Function call argument is a pointer to uninitialized value";
+Message = "Function call argument number '" +
+ std::to_string(ArgumentNumber+1) +
Let's use llvm::getOrdinalSuffix() so that we write "1st argument" instead of
"argument number '1'".
Comment at: lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp:211
// Generate a report for this bug.
- StringRef Desc =
- describeUninitializedArgumentInCall(Call, IsFirstArgument);
+ std::string Desc =
+ describeUninitializedArgumentInCall(Call, ArgumentNumber);
Have you considered using llvm::raw_svector_ostream here as well as passing it
an argument to describeUninitializedArgumentInCall? For example, see
MallocChecker.cpp.
Repository:
rL LLVM
https://reviews.llvm.org/D30341
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30289: [Analyzer] Add bug visitor for taint checker
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Looks great! https://reviews.llvm.org/D30289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29419: [Analyzer] Checker for mismatched iterators
zaks.anna added a comment. > So it would be a wast of resources to duplicate these data. So now I am > also working on the merged version. Would it make sense to just resume the review on the merged patch? https://reviews.llvm.org/D29419 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28348: [analyzer] Taught the analyzer about Glib API to check Memory-leak
zaks.anna added a comment.
> Firstly I uploaded Glib-MallocChecker-single-size-value.patch for code
> review, if submitted to UPSTREAM, then upload another one, correct?
Yes. By the way, you can model XXX_n variants similarly to how calloc is
modeled (see CallocMem).
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:785
-if (FunI == II_malloc) {
+if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_malloc0 ||
+FunI == II_g_try_malloc || FunI == II_g_try_malloc0) {
g_malloc0 needs to be initialized with zeros, not UndefinedVal(). See the
relevant part in MallocChecker::CallocMem:
SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);
return MallocMemAux(C, CE, TotalSize, zeroVal, State);
Repository:
rL LLVM
https://reviews.llvm.org/D28348
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28278: [StaticAnalyzer] dont show wrong 'garbage value' warning when there is array index out of bounds
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thank you! Repository: rL LLVM https://reviews.llvm.org/D28278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30406: [Analyzer] Add support for displaying cross-file diagnostic paths in HTML output
zaks.anna added a comment. No multi-file support is a long outstanding limitation of scan-build html output. Great to see the patch!! Thank you for working on it! > It's not as immediately clear this is a multi-file output. In addition to Artem's suggestions, you might want to insert multiple lines of padding to make the distinction on the border more clear. I think it would help especially when scrolling a large report like in the link for the Linux source. Also, could you put this behind an option or introduce a new format like -analyzer-output=plist-multi-file but for html? Just in case someone is relying on a single file output format, we'd want to have an option to give them to turn it on. https://reviews.llvm.org/D30406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30489: [analyzer] catch out of bounds for VLA
zaks.anna added a comment. Gábor's suggestion sounds good to me. I think ArrayBoundCheckerV2 checker has a higher chance to be productized / moved out of alpha in the future. Should we just remove ArrayBoundChecker.cpp or is there a value in keeping it around? Repository: rL LLVM https://reviews.llvm.org/D30489 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
