[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] 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] 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] 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] 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", ::analyzerEval)
-.Case("clang_analyzer_checkInlined",
- ::analyzerCheckInlined)
-.Case("clang_analyzer_crash", ::analyzerCrash)
-.Case("clang_analyzer_warnIfReached",
- ::analyzerWarnIfReached)
-.Case("clang_analyzer_warnOnDeadSymbol",
- ::analyzerWarnOnDeadSymbol)
-.StartsWith("clang_analyzer_explain",
::analyzerExplain)
-.StartsWith("clang_analyzer_dump", ::analyzerDump)
-.Case("clang_analyzer_getExtent",
::analyzerGetExtent)
-.Case("clang_analyzer_printState",
- ::analyzerPrintState)
-.Case("clang_analyzer_numTimesReached",
- ::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] 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] 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] 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", ::analyzerEval)
-.Case("clang_analyzer_checkInlined",
- ::analyzerCheckInlined)
-.Case("clang_analyzer_crash", ::analyzerCrash)
-.Case("clang_analyzer_warnIfReached",
- ::analyzerWarnIfReached)
-.Case("clang_analyzer_warnOnDeadSymbol",
- ::analyzerWarnOnDeadSymbol)
-.StartsWith("clang_analyzer_explain",
::analyzerExplain)
-.StartsWith("clang_analyzer_dump", ::analyzerDump)
-.Case("clang_analyzer_getExtent",
::analyzerGetExtent)
-.Case("clang_analyzer_printState",
- ::analyzerPrintState)
-.Case("clang_analyzer_numTimesReached",
- ::analyzerNumTimesReached)
-.Default(nullptr);
+ FnCheck Handler =
+ llvm::StringSwitch(C.getCalleeName(CE))
Unrelated change?
Comment at: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:78
+::analyzerWarnOnDeadSymbol)
+ .StartsWith("clang_analyzer_explain",
+ ::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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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 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] 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] 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] 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] D27918: [analyzer] OStreamChecker
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+CheckerContext ) 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] 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] 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] 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] 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] 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] 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] 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] 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 = 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] D32437: [analyzer] Nullability: fix a crash when adding notes inside a synthesized property accessor.
zaks.anna added a comment.
Thanks! A couple of minor comments below.
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:699
+ // We cannot place diagnostics on autosynthesized code.
+ // Put them onto the call site through which we jumped into autosynthesized
+ // code for the first time.
Nit: You could probably factor this into a nicely named helper function.
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:704
+const LocationContext *ParentLC = LC->getParent();
+while (ParentLC->getAnalysisDeclContext()->isBodyAutosynthesized()) {
+ LC = ParentLC;
Is ParentLC guaranteed never to be null? I would prefer checking for it inside
the while. Even if it is not likely to happen now, the code could evolve to
handle more cases..
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:932
+MD->isPropertyAccessor() &&
+CalleeCtx->getAnalysisDeclContext()->isBodyAutosynthesized());
}
There was a case where we wanted to produce events for auto synthesized code,
could you add a comment on what they were? (Otherwise, it's not clear why we
need the isPropertyAccessor check.)
https://reviews.llvm.org/D32437
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33191: [analyzer] ObjCGenerics: account for __kindof specifiers met along a chain of casts.
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:464
ASTContext ) {
+ if (hasKindofArgs(StaticUpperBound)) {
+// If the upper bound type has more __kindof specs, we drop all the info,
I am concerned that this might cause regressions as other logic in this
function is not triggered when this condition fires. For example, this would
only be safe if Current and StaticUpperBoound are the same type apart from
__kindof modifier.
https://reviews.llvm.org/D33191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33263: [scan-build] Patch to scan-build tool to support "--target=" flag
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Looks good. Thank you! Do you have commit access or should we commit? https://reviews.llvm.org/D33263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.
zaks.anna added a comment. Thank you for the patch! Could you please re-submit the patch with context? Instructions on how to do that can be found here: http://llvm.org/docs/Phabricator.html Repository: rL LLVM https://reviews.llvm.org/D32449 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32437: [analyzer] Nullability: fix a crash when adding notes inside a synthesized property accessor.
zaks.anna added a comment. > That wouldn't work this way because we'd have the completely redundant > "calling property accessor" piece before that, and "returning..." after that. I think we should not print "calling" and "returning" for calling into and returning from autogenerated code, If we do, we should fix that as well. Wouldn't it be simpler to just not emit those nodes if we see that they are calling autogenerated code? (Do we have a valid source location in autogenerated context? If not, we could just not print "calling" and "returning" for calls into a body that does not have proper location info.) If you looked into this already and these are not useful suggestions, please, disregard:) https://reviews.llvm.org/D32437 ___ 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.
Sorry for the delay!!!
Comment at:
include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:180
+ inline const llvm::APSInt (QualType T,
+ bool isUnsigned = true) {
+return getValue(0, Ctx.getTypeSize(T), isUnsigned);
The isUnsigned parameter is not used and is not necessary as the type itself
has this info.
Not clear if this function is expected to work for all types or only scalar
types (maybe assert that the input is isScalarType()).
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] D32437: [analyzer] Nullability: fix a crash when adding notes inside a synthesized property accessor.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. > These new "extra notes" of mine might be useful (we could put them at > property declaration), i could add them if everybody likes this idea. What are these? Is there a PR? It would be great if we could place a note on a parent that has location. For example, can we walk up the location context? If you do not fix it in this PR, it's definitely worth a FIXME. https://reviews.llvm.org/D32437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32702: [analyzer] Fix 'Memory Error' bugtype capitalization.
zaks.anna accepted this revision. zaks.anna added a comment. Thanks! https://reviews.llvm.org/D32702 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32291: [analyzer] Implement handling array subscript into null pointer, improve null dereference checks for array subscripts
zaks.anna added inline comments.
Comment at: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:80
}
-else if (isDeclRefExprToReference(E)) {
+else if (isa(E)) {
return E;
Not sure what this does, but looks like we are stricter here now.
Also, since you are changing bug reporter visitor, shouldn't there be tests for
diagnostic paths?
Repository:
rL LLVM
https://reviews.llvm.org/D32291
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32328: [analyzer] Fix assert in ExprEngine::processSwitch
zaks.anna added a comment. Do you have commit access? Repository: rL LLVM https://reviews.llvm.org/D32328 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32328: [analyzer] Fix assert in ExprEngine::processSwitch
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/D32328 ___ 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. I agree that scan-build or scan-build-py integration is an important issue to resolve here. What I envision is that users will just call scan-build and pass -whole-project as an option to it. Everything else will happen automagically:) Another concern is dumping the ASTs to the disk. There are really 2 concerns here. First one is the high disk usage, which is a blocker for having higher adoption. Second is that I am not sure how complete and reliable AST serialization and deserialization are. Are those components used for something else that is running in production or are we just utilizing -ast-dump, which is used for debugging? I do not quite understand why AST serialization is needed at all. Can we instead recompile the translation units on demand into a separate ASTContext and then ASTImport? 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] D31975: [Analyzer] Iterator Checkers
zaks.anna added a comment. @baloghadamsoftware Thanks for working on this! However, this patch is getting too big. Could you, please, split it into incremental patches so that it would be easier to review? More motivation of why this is very important is here http://llvm.org/docs/DeveloperPolicy.html#incremental-development https://reviews.llvm.org/D31975 ___ 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] 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 , +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 );` 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] 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] D30798: [analyzer] Turn suppress-c++-stdlib on by default
zaks.anna added a comment. Correct, this will suppress some valid warnings that occur due to user errors and valid warnings coming from the standard library. However, I believe this is the right choice right now since we know that the analyzer is not currently effective in understanding invariants coming from libc++ headers and users have no ability to help the analyzer (ex: by inserting assertions). This results in a wave of false positives every time the headers change. Furthermore, we do not perform sufficient evaluation and testing for every new version of the headers. Repository: rL LLVM https://reviews.llvm.org/D30798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.
zaks.anna added a comment. Thank you! Repository: rL LLVM https://reviews.llvm.org/D30593 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30762: [ubsan] Add a nullability sanitizer
zaks.anna added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:101 + ``-fsanitize=nullability-assign``, and the argument check with + ``-fsanitize=nullability-arg``. While violating nullability rules does + not result in undefined behavior, it is often unintentional, so UBSan vsk wrote: > zaks.anna wrote: > > Have you investigated if -fsanitize=nullability-arg is too noisy? I think > > we should somehow "recommend" return and assignment check to most users. > > This is not clear from the help and options provided. The main concern here > > is that someone would try -fsanitize=nullability, see a ton of warnings > > they don't care about and think that the whole check is too noisy. > > > > I cannot come up with a great solution. Options that come into mind are: > > - Drop "arg" check completely. > > - Remove arg from the nullability group. > > - Have 2 groups for nullability. > > > > We can also wait for concrete data before making a decision here and > > address this in subsequent commits. > The nullability-arg check was not noisy on a small sample (n=4) of > Apple-internal projects. I only collected 11 diagnostics in total, all of > which seemed actionable. I.e the issues were not isolated to system headers: > the project code actually did violate arg nullability. Based on what I've > seen so far, I'd like to start off with including the arg check in a > "nullability-full" group. If arg check noisiness becomes an issue, then I'd > like to create a new "nullability-partial" group with just the > return/assignment checks. > > TL;DR: let's rename -fsanitize=nullability to -fsanitize=nullability-full, > keep the arg check, and introduce a nullability-partial group later if we > need to. Wdyt? > > Keeping things as is and introducing a new group if this becomes a problem sounds good to me. You could use "nullability" and "nullability-pedantic". That way you do not need to change the name and get to use "nullability". https://reviews.llvm.org/D30762 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30798: [analyzer] Turn suppress-c++-stdlib on by default
zaks.anna added a comment. I've committed the change, but would very much appreciate community feedback here if if there is any! Repository: rL LLVM https://reviews.llvm.org/D30798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30593: Add correct "-isystem"/"-isysroot" warning handling to static analysis' BugReporter.
zaks.anna added a comment. @kmarshall, We are going to turn this off by default, see https://reviews.llvm.org/D30798. Please, do file a bug that lists all the issues you are seeing and desirably instructions on how to reproduce. (Please, list even the cases you are not 100% sure are false positives) Thank you! Repository: rL LLVM https://reviews.llvm.org/D30593 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30798: [analyzer] Turn suppress-c++-stdlib on by default
zaks.anna created this revision.
We have several reports of false positives coming from libc++. For example,
there are reports of false positives in std::regex, std::wcout, and also a
bunch of issues are reported in https://reviews.llvm.org/D30593. In many cases,
the analyzer trips over the complex libc++ code invariants. Let's turn off the
reports coming from these headers until we can re-evalate the support.
We can turn this back on once we individually suppress all known false
positives and perform deeper evaluation on large codebases that use libc++.
We'd also need to commit to doing these evaluations regularly as libc++ headers
change.
https://reviews.llvm.org/D30798
Files:
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
test/Analysis/diagnostics/explicit-suppression.cpp
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-analyzer-config suppress-c++-stdlib=false -verify %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-analyzer-config suppress-c++-stdlib=true -DSUPPRESSED=1 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection
-DSUPPRESSED=1 -verify %s
#ifdef SUPPRESSED
// expected-no-diagnostics
Index: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
===
--- lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -230,7 +230,7 @@
bool AnalyzerOptions::shouldSuppressFromCXXStandardLibrary() {
return getBooleanOption(SuppressFromCXXStandardLibrary,
"suppress-c++-stdlib",
- /* Default = */ false);
+ /* Default = */ true);
}
bool AnalyzerOptions::shouldReportIssuesInMainSourceFile() {
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config suppress-c++-stdlib=false -verify %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config suppress-c++-stdlib=true -DSUPPRESSED=1 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DSUPPRESSED=1 -verify %s
#ifdef SUPPRESSED
// expected-no-diagnostics
Index: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
===
--- lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -230,7 +230,7 @@
bool AnalyzerOptions::shouldSuppressFromCXXStandardLibrary() {
return getBooleanOption(SuppressFromCXXStandardLibrary,
"suppress-c++-stdlib",
- /* Default = */ false);
+ /* Default = */ true);
}
bool AnalyzerOptions::shouldReportIssuesInMainSourceFile() {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30762: [ubsan] Add a nullability sanitizer
zaks.anna added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:101 + ``-fsanitize=nullability-assign``, and the argument check with + ``-fsanitize=nullability-arg``. While violating nullability rules does + not result in undefined behavior, it is often unintentional, so UBSan Have you investigated if -fsanitize=nullability-arg is too noisy? I think we should somehow "recommend" return and assignment check to most users. This is not clear from the help and options provided. The main concern here is that someone would try -fsanitize=nullability, see a ton of warnings they don't care about and think that the whole check is too noisy. I cannot come up with a great solution. Options that come into mind are: - Drop "arg" check completely. - Remove arg from the nullability group. - Have 2 groups for nullability. We can also wait for concrete data before making a decision here and address this in subsequent commits. Comment at: lib/CodeGen/CGDecl.cpp:1907 + + // Skip the return value nullability check if the nullability preconditions + // are broken. I would add a comment explaining why this is needed, similar to what you included in the commit message: "One point of note is that the nullability-return check is only allowed to kick in if all arguments to the function satisfy their nullability preconditions. This makes it necessary to emit some null checks in the function body itself." Maybe even rename CanCheckRetValNullability into RetValNullabilityPrecondition. I like this more than "CanCheck" because it's just a precondition value. You check if it is satisfied (evaluates to true or false) later when it's used in a branch. https://reviews.llvm.org/D30762 ___ 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. > All this leads to the need of several types of taintednesses (string > taintedness, array taintedness, general bound check taintedness) because the > cleansing can only take down one type of taintedness at a time. That would be > the only way for a checker to be able to access that the taintedness type > specific to the checker is still there or was already cleansed by the central > logic. For me it shows that cleansing rules belong to specific checkers and > cannot be efficiently generalized even in case of a single int type. I do not see why we cannot have type-dependent rules in the general taint checker. Checker-specific taint rules could be added to the checkers on top of the generic rules. Though, I do not have an example of this off the top of my head. Specifically, I do not think we can issue a warning every time we are not 100% sure that an index into an array is not in bounds with respect to the region extents. It would trigger for cases when the code compares against a statically unknown value and lead to too many false positives. Stepping back a bit, what do you consider "dirty" vs "clean"? It seems that you are looking for prove that the values are known to be within the bounds of min and max int values. What happens if there is a comparison to an unknown symbolic value? Should that be considered as clean or tainted? Are there test cases for this? 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] D22862: [analyzer] Fix for PR15623: eliminate unwanted ProgramState checker data propagation.
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thi has been committed in r290505. https://reviews.llvm.org/D22862 ___ 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. @vlad.tsyrklevich, Do you have commit access or should we commit on your behalf? 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] D30489: [analyzer] catch out of bounds for VLA
zaks.anna requested changes to this revision. zaks.anna added a comment. This revision now requires changes to proceed. Following Gabor's suggestion, we should investigate if ArrayBoundCheckerV2 supports this. If not it's possible that we are hitting the Constraint Solver limitations. 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
[PATCH] D27092: Add RecursionChecker for finding infinite recursion
zaks.anna added a comment. This is waiting for a resolution on a CallEvent API patch as described in https://reviews.llvm.org/D27091. This is blocked on https://reviews.llvm.org/D27091. Comment at: lib/StaticAnalyzer/Checkers/RecursionChecker.cpp:29 +// this patch. +REGISTER_SET_WITH_PROGRAMSTATE(DirtyStackFrames, + const clang::StackFrameContext *) https://reviews.llvm.org/D27092 ___ 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 accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. Thanks. looks good. 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] D30406: [Analyzer] Add support for displaying cross-file diagnostic paths in HTML output
zaks.anna added a comment. > I’ve added the single file output option but I would like to keep the > multi-file option default This sounds good to me! I agree that this is a very useful addition. 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] D28348: [analyzer] Taught the analyzer about Glib API to check Memory-leak
zaks.anna accepted this revision. zaks.anna added a comment. This revision is now accepted and ready to land. > I am not clear why need to calculate the precise allocated size? The information generated by this checker is used for array bounds checking. For example, see https://reviews.llvm.org/D24307 This patch looks good. Do you have commit access or should I commit it on your behalf? 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] D30157: [analyzer] Improve valist check
zaks.anna added inline comments.
Comment at: test/Analysis/valist-uninitialized-no-undef.c:25
+ va_list va;
+ vprintf(isstring ? "%s" : "%d", va); //expected-warning{{Function 'vprintf'
is called with an uninitialized va_list argument}} expected-note{{Function
'vprintf' is called with an uninitialized va_list argument}}
expected-note{{Assuming 'isstring' is 0}} expected-note{{'?' condition is
false}}
+}
Please, split the long "expected" lines into multiple lines - one per note. It
will improve readability in non-wrapping editors. Thanks!
https://reviews.llvm.org/D30157
___
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 inline comments. 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); danielmarjamaki wrote: > zaks.anna wrote: > > Have you considered using llvm::raw_svector_ostream here as well as > > passing it an argument to describeUninitializedArgumentInCall? For example, > > see MallocChecker.cpp. > I changed so describeUninitializedArgumentInCall() returns an llvm::Twine > instead of std::string. hope you like it. > I do not think it's safe to use llvm:Twine here. See http://llvm.org/docs/ProgrammersManual.html#the-twine-class How about using llvm::raw_svector_ostream as I suggested? 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] 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
[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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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 = 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] 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] 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
