Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Anna Zaks]

zaks.anna added a comment.

This definitely needs tests!

I agree that the file name should be used for bug identification. I am not sure 
if it should be included in the hash by the analyzer or if it should be 
included by the processing script. Have you looked at CmpRuns.py and 
SATestBuild.py?

 On the other hand, the tools will be much faster if they do not have to 
 process more information from the report to identify the bugs.


The file name is another field in the same report; I do not see how this would 
result in a much slower scripts.

Here is one problem to consider:
What is included as filename? Is it the full path to the file where the issue 
is found? Is it just the file name without the path?

Does your solution support uniquing the issues when the same project is 
analyzed from different directories? 
How will you handle issues coming from 2 different projects that have the same 
file name?

Our testing scrips that is based on CmpRuns has the notion of multiple projects 
and bug reports coming from different projects are considered to be different. 
We include project name and the location of the file within the project to 
identify the issue. Not including a filename in the hash allows for that model.


http://reviews.llvm.org/D10305




___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] D11572: [Static Analyzer] Checker for OS X / iOS localizability issues

[Anna Zaks]

zaks.anna added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:14
@@ +13,3 @@
+//  2) A syntactic checker that warns against the bad practice of
+// not including a comment in NSLocalizedString macros.
+//

My quick Google search did not show any mention of this :(


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:202
@@ +201,3 @@
+void NonLocalizedStringChecker::reportLocalizationError(
+SVal S, const ObjCMethodCall M, CheckerContext C,
+int argumentNumber) const {

Adding a new transition is more debugging friendly.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:334
@@ +333,3 @@
+  SVal sv = Call.getReturnValue();
+  if (isAnnotatedAsLocalized(D) || LSF.find(IdentifierName) != LSF.end()) {
+setLocalizedState(sv, C);

Ah, I see!


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:366
@@ +365,3 @@
+  Selector S = msg.getSelector();
+  StringRef SelectorName = S.getAsString();
+  assert(!SelectorName.empty());

I am not sure if we are on the same page.. I would not store/mark this 
information in the state, but just use the code as part of the check.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:451
@@ +450,3 @@
+/// checking for (comment) is not used and thus not present in the AST,
+/// so we use Lexer on the original macro call and retrieve the value of
+/// the comment. If it's empty or nil, we raise a warning.

The point is that this macro can be used inside another, user defined macro 
where you'd no longer know which argument corresponds to comment.  The only 
way around that that I see is to only warn when these macros are called 
directly.


http://reviews.llvm.org/D11572




___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] D11572: [Static Analyzer] Checker for OS X / iOS localizability issues

[Anna Zaks]

zaks.anna added a comment.

Thanks for working on this!

Comments in line,
Anna.



Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:461
@@ +460,3 @@
+  HelpTextCheck that warns about uses of non-localized NSStrings passed to 
UI methods expecting localized strings,
+  DescFileLocalizationChecker.cpp;
+

How about shortening the message to Warn about..


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:13
@@ +12,3 @@
+// UI methods expecting localized strings
+//  2) A syntactic checker that warns against not including a comment in
+// NSLocalizedString macros.

Is the comment argument optional in some cases or is it always encouraged in 
some contexts?


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:39
@@ +38,3 @@
+private:
+  enum Kind { Unknown, NonLocalized, Localized } K;
+  LocalizedState(Kind InK) : K(InK) {}

Unknown is never used?


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:66
@@ +65,3 @@
+  mutable std::mapStringRef, std::mapStringRef, uint8_t UIMethods;
+  mutable llvm::SmallSetstd::pairStringRef, StringRef, 10 LSM;
+  mutable llvm::StringMapchar LSF; // StringSet doesn't work

Please, use more descriptive names here (or add a comment on what these are).
Why StringSet does not work?


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:201
@@ +200,3 @@
+
+  ExplodedNode *ErrNode = C.addTransition();
+  if (!ErrNode)

I believe this would just return a predecessor and not add a new transition.
If you want to create a new transition, you should tag the node with your 
checker info. For example, see CheckerProgramPointTag in the MallocChecker.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:208
@@ +207,3 @@
+  new BugReport(*BT, String should be localized, ErrNode));
+  R-addRange(M.getSourceRange());
+  R-markInteresting(S);

You can try reporting a more specific range here, for example the range of the 
argument expression if available. This is what gets highlighted in Xcode.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:235
@@ +234,3 @@
+// These special NSString methods draw to the screen
+StringRef drawAtPoint(drawAtPoint);
+StringRef drawInRect(drawInRect);

Are these temporaries needed?


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:289
@@ +288,3 @@
+/// if it is in LocStringFunctions (LSF) or the function is annotated
+void NonLocalizedStringChecker::checkPostCall(const CallEvent Call,
+  CheckerContext C) const {

Or the heuristic is triggered, right?
This applies to C++ as well.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:322
@@ +321,3 @@
+} else {
+  // Perhaps I can use a different heuristic for non-aggressive reporting?
+  const SymbolicRegion *SymReg =

Could you add a comment documenting what it is?
Is it that the literals other than the empty string are assumed to be 
non-localized?



Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:333
@@ +332,3 @@
+/// if it is in LocStringMethods or the method is annotated
+void NonLocalizedStringChecker::checkPostObjCMessage(const ObjCMethodCall msg,
+ CheckerContext C) const {

Why are we not using the same heuristic as in the case with other calls?


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:365
@@ +364,3 @@
+  StringRef stringValue = SL-getString()-getString();
+  SVal sv = C.getSVal(SL);
+  if (stringValue.empty()) {

Is it possible to see that we are dealing with an empty string from an SVal? 
That way you could keep the state lean.


Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:426
@@ +425,3 @@
+
+void EmptyLocalizationContextChecker::MethodCrawler::VisitObjCMessageExpr(
+const ObjCMessageExpr *ME) {

Could you add a comment explaining what this does. For example, we try to match 
these macros and we assume they are defined in this way.. 

Also, the point here is that we cannot use the path sensitive check because the 
macro argument we are checking for is not used, so it only appears in the macro 
call, but not in the expansion.

#define NSLocalizedString(key, comment) \
[[NSBundle mainBundle] localizedStringForKey:(key) value:@ table:nil]
#define NSLocalizedStringFromTable(key, tbl, comment) \
[[NSBundle mainBundle] localizedStringForKey:(key) value:@ 
table:(tbl)]
#define NSLocalizedStringFromTableInBundle(key, tbl, bundle, comment) \
[bundle localizedStringForKey:(key) value:@ table:(tbl)]
#define 

Re: [PATCH] D10634: Loss of Sign Checker

[Anna Zaks]

zaks.anna added a subscriber: zaks.anna.
zaks.anna added a comment.

I have a couple of high level comments.

Why do you have checkASTDecl (which is a syntactic check) in addition to the 
checkBind (which is a path-sensitive check)? Does checkASTDecl find more 
issues? can those be found using a path sensitive callback?

I am leaning toward allowing explicit assignments to -1, like in this case: 
unsigned int j = -1. The tool is much more usable if there are few false 
positives.

Some of the results look like false positives. For example, this one from 
https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view: 
fitscore.c:1077:21: warning: assigning negative value to unsigned variable 
loses sign and may cause undesired runtime behavior 
[clang-analyzer-alpha.core.LossOfSignAssign]

  namelen -= 9;  /* deleted the string 'HIERARCH ' */

It's hard to know what they are if we are only looking at the line where the 
issue is reported without seeing the pull path. Do we know that the value can 
be negative on that path?



Comment at: test/Analysis/LossOfSignAssign.c:19
@@ +18,3 @@
+  return i+j; // implicit conversion here too!
+}
+

What happens if the return type is unsigned?


http://reviews.llvm.org/D10634




___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] D11427: [Static Analyzer] Checker for Obj-C generics

[Anna Zaks]

zaks.anna added a comment.

Add tests for checking element retrieval and tracking with ObjC subscript 
syntax for arrays and dictionaries.

Also see:
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return



Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:334
@@ +333,3 @@
+
+void ObjCGenericsChecker::checkPostObjCMessage(const ObjCMethodCall M,
+   CheckerContext C) const {

Add comment explaining that this callback is used to infer the types. 
Specifically, if a class method is called on a symbol, we use it to infer the 
type of the symbol.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:338
@@ +337,3 @@
+
+  ProgramStateRef State = C.getState();
+  SymbolRef Sym = M.getReturnValue().getAsSymbol();

Move State down, where it's used.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:346
@@ +345,3 @@
+  if (MessageExpr-getReceiverKind() != ObjCMessageExpr::Class ||
+  Sel.getAsString() != class)
+return;

Clarify what you are doing here.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:414
@@ +413,3 @@
+
+  // When the receiver type is id, or some super class of the tracked type (and
+  // kindof type), look up the method in the tracked type, not in the receiver

Split funding a tighter method definition into a subroutine.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:421
@@ +420,3 @@
+  MessageExpr-getReceiverKind() == ObjCMessageExpr::Class) {
+if (ASTCtxt.getObjCIdType() == ReceiverType ||
+ASTCtxt.getObjCClassType() == ReceiverType ||

Should checking for id, class and kindof be a helper method?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:425
@@ +424,3 @@
+ ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType,
+ *TrackedType))) {
+  const ObjCInterfaceDecl *InterfaceDecl =

What if ASTCtxt.canAssignObjCInterfaces is false here? Shouldn't we warn?
Will we warn elsewhere? Let's make sure we have a test case. And add a comment.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:444
@@ +443,3 @@
+  OptionalArrayRefQualType TypeArgs =
+  (*TrackedType)-getObjCSubstitutions(Method-getDeclContext());
+  // This case might happen when there is an unspecialized override of a

Should we use the type on which this is defined and not the tracked type?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:453
@@ +452,3 @@
+// We can't do any type-checking on a type-dependent argument.
+if (Arg-isTypeDependent())
+  continue;

Why are we not checking other parameter types? Will those bugs get caught by 
casts? I guess yes..


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:458
@@ +457,3 @@
+
+QualType OrigParamType = Param-getType();
+const auto *ParamTypedef = OrigParamType-getAsTypedefType();

Why isObjCTypeParamDependent is not used here?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:485
@@ +484,3 @@
+if (ArgSym) {
+  const ObjCObjectPointerType *const *TrackedType =
+  State-getTypeParamMap(ArgSym);

Let's not shadow TrackedType.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:495
@@ +494,3 @@
+// accepted.
+if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
+ ArgObjectPtrType) 

This would be simplified if you pull out the negation; you essentially, list 
the cases where we do not warn on parameters:
 1) arg can be assigned to (subtype of) param
 2) covariant parameter types and param is a subtype of arg


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:500
@@ +499,3 @@
+  ParamObjectPtrType))) {
+  ExplodedNode *N = C.addTransition();
+  reportBug(ArgObjectPtrType, ParamObjectPtrType, N, Sym, C);

This just returns the previous state. 

If you want to create a node, you need to tag it; see the tags on leak reports.



Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:513
@@ +512,3 @@
+  ASTCtxt, *TypeArgs, ObjCSubstitutionContext::Result);
+  if (IsInstanceType)
+ResultType = QualType(*TrackedType, 0);

We should not use TrackedType in the case lookup failed.
Can the TrackedType be less specialized that the return type?
Maybe rename as IsDeclaredAsInstanceType?


Comment at: 

Re: [PATCH] D11427: [Static Analyzer] Checker for Obj-C generics

[Anna Zaks]

zaks.anna added a comment.

Here is a partial review. Some comments below and others are inline.

Thanks!
Anna.

- Add at least one test that tests for the full error message.
- Add a test that tests path-sensitive output (the plist file) after testing 
that that looks correct in Xcode. For example, how is GenericsBugVisitor 
tested? That should be easy with $ awk '{print // CHECK: $0}' in.txt  out.txt
- nit: It would be helpful if you could use descriptive names for each test.



Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:456
@@ +455,3 @@
+def ObjCGenericsChecker : CheckerObjCGenerics,
+  HelpTextChecks for type errors.,
+  DescFileObjCGenericsChecker.cpp;

Please, provide better description of the checker.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:11
@@ +10,3 @@
+// This checker tries to find type errors that the compiler is not able to 
catch
+// due to the implicit conversions that was introduced for backward
+// compatibility.

was - were

Please, add some documentation on what this checker is trying to catch and how 
it works.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:92
@@ +91,3 @@
+// ProgramState trait - a map from symbol to its type with specified params.
+REGISTER_MAP_WITH_PROGRAMSTATE(TypeParamMap, SymbolRef,
+   const ObjCObjectPointerType *)

type with specified params - specialized type?

Also, maybe we should move this above the GenericsBugVisitor declaration for 
greater visibility?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:173
@@ +172,3 @@
+const ObjCObjectPointerType *MostInformativeCandidate, ASTContext C) {
+  // Checking if from and two are the same classes modulo specialization.
+  if (From-getInterfaceDecl()-getCanonicalDecl() ==

two - to


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:197
@@ +196,3 @@
+static const ObjCObjectPointerType *
+getMostInformativeDerivedClass(const ObjCObjectPointerType *From,
+   const ObjCObjectPointerType *To, ASTContext C) 
{

Could you add a comment on what this does?
Is there a precondition on the arguments (To and From)? We seem to be walking 
up the hierarchy of To.
Is To always guaranteed to have a super class? What ensures that?



Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:224
@@ +223,3 @@
+
+  ASTContext ASTCtxt = C.getASTContext();
+

Move this after the check?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:229
@@ +228,3 @@
+
+  // In order to detect subtype relation properly, strip the kindofness.
+  OrigObjectPtrType = OrigObjectPtrType-stripObjCKindOfTypeAndQuals(ASTCtxt);

Could you explain more why are we skipping kindoff?
Is there a test case for this?
Let's add more __kindof tests.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:253
@@ +252,3 @@
+  // If OrigObjectType could convert to DestObjectType, this could be an
+  // implicit cast. Handle it as implicit cast.
+  if (isaExplicitCastExpr(CE)  !OrigToDest) {

Comment does not seem to match the code below.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:257
@@ +256,3 @@
+// However a downcast may also lose information. E. g.:
+//   MutableMapT, U : Map
+// The downcast to mutable map loses the information about the types of the

Should the Map be specialized in this example?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:270
@@ +269,3 @@
+}
+return;
+  }

We ignore the cast on the else branch (mismatched type)?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:280
@@ +279,3 @@
+  State = State-setTypeParamMap(Sym, OrigObjectPtrType);
+  C.addTransition(State);
+}

Simplify the code by using early returns.

Also, try to add comments with intuition on what cases are covered.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:297
@@ +296,3 @@
+reportBug(*TrackedType, DestObjectPtrType, N, Sym, C);
+return;
+  }

This will not get caught by the check below?


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:326
@@ +325,3 @@
+static const Expr *stripImplicitIdCast(const Expr *E, ASTContext ASTCtxt) {
+  const ImplicitCastExpr *CE = dyn_castImplicitCastExpr(E);
+  if (CE  CE-getCastKind() == CK_BitCast 

Shouldn't we strip more than id casts?


Comment at: test/Analysis/generics.m:45
@@ +44,3 @@
+@interface MutableArrayT : NSArrayT
+- (int)addObject:(T)obj;
+@end

I'd prefer if these were copied 

Re: [PATCH] D11432: [Static Analyzer] Some tests do not turn on core checkers. Running the analyzers without the core checkers is no longer supported.

[Anna Zaks]

zaks.anna added a comment.

It has never been supported.

LGTM.


http://reviews.llvm.org/D11432




___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [anayzer] Basic checker option validation

[Anna Zaks]

Please,

Tighten the check so that we reject the prefixes of the full checker name that 
are not valid package names.

Also, add tests!


http://reviews.llvm.org/D8077

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Static Analyzer] Minor improvements to SATest

[Anna Zaks]

LGTM.



Comment at: utils/analyzer/SATestBuild.py:410
@@ -406,3 +409,3 @@
 # Compare the warnings produced by scan-build.
-def runCmpResults(Dir):   
+def runCmpResults(Dir, Strictness = 0):   
 TBegin = time.time() 

Let's document Strictness here.

http://reviews.llvm.org/D10812

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Prevent ccc/c++-analyzer from hanging on Windows.

[Anna Zaks]

Sylvestre,

Would you be interested in testing this before it lands?


http://reviews.llvm.org/D8774

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Prevent ccc/c++-analyzer from hanging on Windows.

[Anna Zaks]

Anton has commit access.


http://reviews.llvm.org/D8774

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Add scan-build python implementation

[Anna Zaks]

 I think will be nice to have possibility to control output content.

  When I run scan-build with all checkers enabled on my project, I got ~ 100 
 Gb of HTML mostly because of deadcode.DeadStores. My solution was to create 

  CC and CXX wrappers which run Clang-tidy with Analyzer only checkers 
 (producing messages with relevant line only).


Eugene,

These are very valid points and should be considered for future improvements to 
scan-build. However, these are outside of the scope of this initial patch, 
which is to match the existing functionality and allow for better build system 
interposition.

The bloat you get with the HTML output is a known limitation; for example, we 
produce one HTML file per bug report instead of sharing the same HTML between 
several bug reports that get reported on the same file. One thing in keep in 
mind is that the static analyzer contains many path-sensitive checks. When 
those are reported, you get a pull path through your code, where the problem 
occurs. You will not get the same experience when consuming reports through 
clang tidy.


http://reviews.llvm.org/D9600

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r240800 - [static analyzer] Analyzer is skipping forward declared C/C++ functions

[Anna Zaks]

Author: zaks
Date: Fri Jun 26 12:42:58 2015
New Revision: 240800

URL: http://llvm.org/viewvc/llvm-project?rev=240800view=rev
Log:
[static analyzer] Analyzer is skipping forward declared C/C++ functions

A patch by Karthik Bhat!

This patch fixes a regression introduced by r224398. Prior to r224398
we were able to analyze the following code in test-include.c and report
a null deref in this case. But post r224398 this analysis is being skipped.

E.g.
  // test-include.c
  #include test-include.h
  void test(int * data) {
data = 0;
*data = 1;
  }

   // test-include.h
  void test(int * data);

This patch uses the function body (instead of its declaration) as the location
of the function when deciding if the Decl should be analyzed with path-sensitive
analysis. (Prior to r224398, the call graph was guaranteed to have a definition
when available.)

Added:
cfe/trunk/test/Analysis/test-include-cpp.cpp
cfe/trunk/test/Analysis/test-include-cpp.h
cfe/trunk/test/Analysis/test-include.c
cfe/trunk/test/Analysis/test-include.h
Modified:
cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp?rev=240800r1=240799r2=240800view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp Fri Jun 26 
12:42:58 2015
@@ -588,7 +588,10 @@ AnalysisConsumer::getModeForDecl(Decl *D
   // - Header files: run non-path-sensitive checks only.
   // - System headers: don't run any checks.
   SourceManager SM = Ctx-getSourceManager();
-  SourceLocation SL = SM.getExpansionLoc(D-getLocation());
+  SourceLocation SL = D-hasBody() ? D-getBody()-getLocStart()
+ : D-getLocation();
+  SL = SM.getExpansionLoc(SL);
+
   if (!Opts-AnalyzeAll  !SM.isWrittenInMainFile(SL)) {
 if (SL.isInvalid() || SM.isInSystemHeader(SL))
   return AM_None;

Added: cfe/trunk/test/Analysis/test-include-cpp.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/test-include-cpp.cpp?rev=240800view=auto
==
--- cfe/trunk/test/Analysis/test-include-cpp.cpp (added)
+++ cfe/trunk/test/Analysis/test-include-cpp.cpp Fri Jun 26 12:42:58 2015
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+
+#include test-include-cpp.h
+
+int TestIncludeClass::test1(int *p) {
+  p = 0;
+  return *p; // expected-warning{{Dereference of null pointer}}
+}
+
+int TestIncludeClass::test2(int *p) {
+  p = 0;
+  return *p; // expected-warning{{Dereference of null pointer}}
+}

Added: cfe/trunk/test/Analysis/test-include-cpp.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/test-include-cpp.h?rev=240800view=auto
==
--- cfe/trunk/test/Analysis/test-include-cpp.h (added)
+++ cfe/trunk/test/Analysis/test-include-cpp.h Fri Jun 26 12:42:58 2015
@@ -0,0 +1,9 @@
+#ifndef TEST_INCLUDE_CPP_H
+#define TEST_INCLUDE_CPP_H
+
+class TestIncludeClass {
+  int test1(int *);
+  static int test2(int *);
+};
+
+#endif

Added: cfe/trunk/test/Analysis/test-include.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/test-include.c?rev=240800view=auto
==
--- cfe/trunk/test/Analysis/test-include.c (added)
+++ cfe/trunk/test/Analysis/test-include.c Fri Jun 26 12:42:58 2015
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+
+#include test-include.h
+#define DIVYX(X,Y) Y/X
+
+void test_01(int *data) {
+  data = 0;
+  *data = 1; // expected-warning{{Dereference of null pointer}}
+}
+
+int test_02() {
+  int res = DIVXY(1,0); // expected-warning{{Division by zero}}
+// expected-warning@-1{{division by zero is undefined}}
+  return res;
+}
+
+int test_03() {
+  int res = DIVYX(0,1); // expected-warning{{Division by zero}}
+// expected-warning@-1{{division by zero is undefined}}
+  return res;
+}
\ No newline at end of file

Added: cfe/trunk/test/Analysis/test-include.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/test-include.h?rev=240800view=auto
==
--- cfe/trunk/test/Analysis/test-include.h (added)
+++ cfe/trunk/test/Analysis/test-include.h Fri Jun 26 12:42:58 2015
@@ -0,0 +1,2 @@
+void test_01(int * data);
+#define DIVXY(X,Y) X/Y


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Static Analyzer] Fix Analysis being skipped for code with declarations in .h file

[Anna Zaks]

Committed in r240800.

Thank you VERY MUCH for sending in this patch. It fixes a truly bad regression!

Anna.


http://reviews.llvm.org/D10156

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r240725 - [docs] Several updates to the Address Sanitizer webpage.

[Anna Zaks]

Author: zaks
Date: Thu Jun 25 18:36:44 2015
New Revision: 240725

URL: http://llvm.org/viewvc/llvm-project?rev=240725view=rev
Log:
[docs] Several updates to the Address Sanitizer webpage.

 - Added the description of the interceptor suppression.
 - Re-organized a bit: grouped a few things under the Issue Suppression
   section, grouped IOC and leaks under a section, placed symbolication
   info into Symbolizing the Reports section..
 - In supported platforms: MacOS - OS X; added iOS Simulator
 - Added a paragraph to the Usage section describing when DYLD_INSERT_LIBRARIES
   might need to be used.
 - attribute((no_sanitize_address)) - 
__attribute__((no_sanitize(address)))
 - Updated Leak Sanitizer page with most up to date info.


http://reviews.llvm.org/D10559

Modified:
cfe/trunk/docs/AddressSanitizer.rst
cfe/trunk/docs/LeakSanitizer.rst

Modified: cfe/trunk/docs/AddressSanitizer.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/AddressSanitizer.rst?rev=240725r1=240724r2=240725view=diff
==
--- cfe/trunk/docs/AddressSanitizer.rst (original)
+++ cfe/trunk/docs/AddressSanitizer.rst Thu Jun 25 18:36:44 2015
@@ -60,7 +60,28 @@ or:
 % clang -g -fsanitize=address example_UseAfterFree.o
 
 If a bug is detected, the program will print an error message to stderr and
-exit with a non-zero exit code. To make AddressSanitizer symbolize its output
+exit with a non-zero exit code. AddressSanitizer exits on the first detected 
error.
+This is by design:
+
+* This approach allows AddressSanitizer to produce faster and smaller 
generated code
+  (both by ~5%).
+* Fixing bugs becomes unavoidable. AddressSanitizer does not produce
+  false alarms. Once a memory corruption occurs, the program is in an 
inconsistent
+  state, which could lead to confusing results and potentially misleading
+  subsequent reports.
+
+If your process is sandboxed and you are running on OS X 10.10 or earlier, you
+will need to set ``DYLD_INSERT_LIBRARIES`` environment variable and point it to
+the ASan library that is packaged with the compiler used to build the
+executable. (You can find the library by searching for dynamic libraries with
+``asan`` in their name.) If the environment variable is not set, the process 
will
+try to re-exec. Also keep in mind that when moving the executable to another 
machine,
+the ASan library will also need to be copied over.
+
+Symbolizing the Reports
+=
+
+To make AddressSanitizer symbolize its output
 you need to set the ``ASAN_SYMBOLIZER_PATH`` environment variable to point to
 the ``llvm-symbolizer`` binary (or make sure ``llvm-symbolizer`` is in your
 ``$PATH``):
@@ -100,14 +121,63 @@ force disabled by setting ``ASAN_OPTIONS
 Note that on OS X you may need to run ``dsymutil`` on your binary to have the
 file\:line info in the AddressSanitizer reports.
 
-AddressSanitizer exits on the first detected error. This is by design.
-One reason: it makes the generated code smaller and faster (both by
-~5%). Another reason: this makes fixing bugs unavoidable. With Valgrind,
-it is often the case that users treat Valgrind warnings as false
-positives (which they are not) and don't fix them.
+Additional Checks
+=
+
+Initialization order checking
+-
+
+AddressSanitizer can optionally detect dynamic initialization order problems,
+when initialization of globals defined in one translation unit uses
+globals defined in another translation unit. To enable this check at runtime,
+you should set environment variable
+``ASAN_OPTIONS=check_initialization_order=1``.
+
+Note that this option is not supported on OS X.
+
+Memory leak detection
+-
+
+For more information on leak detector in AddressSanitizer, see
+:doc:`LeakSanitizer`. The leak detection is turned on by default on Linux;
+however, it is not yet supported on other platforms.
+
+Issue Suppression
+=
+
+AddressSanitizer is not expected to produce false positives. If you see one,
+look again; most likely it is a true positive!
+
+Suppressing Reports in External Libraries
+-
+Runtime interposition allows AddressSanitizer to find bugs in code that is
+not being recompiled. If you run into an issue in external libraries, we
+recommend immediately reporting it to the library maintainer so that it
+gets addressed. However, you can use the following suppression mechanism
+to unblock yourself and continue on with the testing. This suppression
+mechanism should only be used for suppressing issues in external code; it
+does not work on code recompiled with AddressSanitizer. To suppress errors
+in external libraries, set the ``ASAN_OPTIONS`` environment variable to point
+to a suppression file. You can either specify the full path to the file or the
+path of the file relative to the location of your executable.
+
+.. code-block:: bash
+

Re: [PATCH] [Static Analyzer] Fix Analysis being skipped for code with declarations in .h file

[Anna Zaks]

I think this should be all that's needed:

 // - System headers: don't run any checks.
 SourceManager SM = Ctx-getSourceManager();
  -  SourceLocation SL = SM.getExpansionLoc(D-getLocation());
  +  
  +  SourceLocation SL = D-hasBody() ? D-getBody()-getLocStart() 
  +   : D-getLocation(); 
  +  SL = SM.getExpansionLoc(SL);
  +  
 if (!Opts-AnalyzeAll  !SM.isWrittenInMainFile(SL)) {

There are 2 differences from your patch:

1. I am not sure why the second if statement is added in your patch.
2. Getting the ExpansionLoc for the body. (In case it's a macro, it won't get 
analyzed.) Would be great if you add the macro test case as well.

Thanks!
Anna.



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:603
@@ -595,1 +602,3 @@
+if (SM.isInMainFile(SL))
+  return Mode;
 return Mode  ~AM_Path;

I don't think this if-statement is needed.

http://reviews.llvm.org/D10156

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] clarify ownership of BugReport objects

[Anna Zaks]

I have no additional comments. Looking forward to the updated patch.

Thank you,
Anna.
 On Jun 22, 2015, at 9:31 AM, Aaron Ballman aa...@aaronballman.com wrote:
 
 On Mon, Jun 22, 2015 at 12:23 PM, David Blaikie dblai...@gmail.com wrote:
 I'm assuming emitReport always takes ownership? (it doesn't look like it has
 any API surface area to express to the caller (in the current raw pointer
 API) that it didn't take ownership)
 
 Everywhere that I found was assuming ownership was taken, but I wanted
 to ask the analyzer folks in case they knew something I didn't.
 
 In that case, best to pass by value rather than (lvalue or rvalue) reference
 so caller's explicitly pass off ownership (std::move(R)) to emitReport.
 
 I was passing by reference on the chance that the caller did need the
 original pointer value, since it would be updated to something likely
 to crash quickly in the event they attempted to use the moved-from
 pointer value. My testing implies that no original caller uses the
 pointer after passing it to emitReport, so you're right, that probably
 should go back to being passed by value. Thank you!
 
 You'll have to use llvm::make_unique, rather than std::make_unique - since
 we're still compiling as C++11, not C++14.
 
 Oh shoot, that always messes me up! Easy enough to rectify.
 
 I probably wouldn't bother renaming R to UniqueR in emitReport - don't
 think it adds/changes much. Ah, I see - there already was a UniqueR and R.
 Probably just use R, though? (now that it's the only thing, so there's no
 need to distinguish it as the unique one)
 
 Yeah, I should go back to using R instead of UniqueR.
 
 Looks like a reformat of an unchanged/unrelated line:
 
 -  BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
 +  BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
 
 Yeah, drive-by. I can rectify that separately.
 
 And where does emitReport actually take ownership?
 
 Index: lib/StaticAnalyzer/Core/BugReporter.cpp
 ===
 --- lib/StaticAnalyzer/Core/BugReporter.cpp (revision 240274)
 +++ lib/StaticAnalyzer/Core/BugReporter.cpp (working copy)
 @@ -3224,11 +3224,8 @@
   BugTypes = F.add(BugTypes, BT);
 }
 
 -void BugReporter::emitReport(BugReport* R) {
 -  // To guarantee memory release.
 -  std::unique_ptrBugReport UniqueR(R);
 
 That's where it was doing it.
 
 Thank you for the review, David!
 
 ~Aaron
 
 
 On Mon, Jun 22, 2015 at 8:07 AM, Aaron Ballman aa...@aaronballman.com
 wrote:
 
 Currently, when the analyzer wants to emit a report, it takes a naked
 pointer, and the emitReport function eventually takes over ownership
 of that pointer. I think this is a dangerous API because it's not
 particularly clear that this ownership transfer will happen, or that
 the pointer must be unique.
 
 This patch makes the ownership semantics more clear by encoding it as
 part of the API. There should be no functional changes, and I do not
 think it caught any bugs, but I do think this is an improvement.
 
 Thoughts or opinions?
 
 ~Aaron
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
 
 


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Static Analyzer] Fix Analysis being skipped for code with declarations in .h file

[Anna Zaks]

Sorry about the breakage.

What about computing SL based on the location of the body before any of the 
checks? Specifically, changing this line:

  SourceLocation SL = SM.getExpansionLoc(D-getLocation());

Anna.


http://reviews.llvm.org/D10156

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] clarify ownership of BugReport objects

[Anna Zaks]

LGTM and LGTJordan!

Thank you,
Anna.

 On Jun 22, 2015, at 11:03 AM, Aaron Ballman aa...@aaronballman.com wrote:
 
 On Mon, Jun 22, 2015 at 1:59 PM, David Blaikie dblai...@gmail.com wrote:
 
 
 On Mon, Jun 22, 2015 at 10:48 AM, Aaron Ballman aa...@aaronballman.com
 wrote:
 
 Updated patch attached.
 
 On Mon, Jun 22, 2015 at 12:23 PM, David Blaikie dblai...@gmail.com
 wrote:
 I'm assuming emitReport always takes ownership? (it doesn't look like it
 has
 any API surface area to express to the caller (in the current raw
 pointer
 API) that it didn't take ownership)
 
 In that case, best to pass by value rather than (lvalue or rvalue)
 reference
 so caller's explicitly pass off ownership (std::move(R)) to emitReport.
 
 Fixed.
 
 
 You'll have to use llvm::make_unique, rather than std::make_unique -
 since
 we're still compiling as C++11, not C++14.
 
 Fixed.
 
 I probably wouldn't bother renaming R to UniqueR in emitReport -
 don't
 think it adds/changes much. Ah, I see - there already was a UniqueR and
 R.
 Probably just use R, though? (now that it's the only thing, so there's
 no
 need to distinguish it as the unique one)
 
 Fixed.
 
 Looks like a reformat of an unchanged/unrelated line:
 
 -  BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID,
 InsertPos);
 +  BugReportEquivClass *EQ = EQClasses.FindNodeOrInsertPos(ID,
 InsertPos);
 
 Removed (will commit separately).
 
 
 Will leave it to Anna to sign off on, but looks like an improvement to me.
 
 (my usual generic question on changes like this (should probably be treated
 separately from this patch, if you've interest): could we use this by value
 instead of by pointer? I see BugReport has virtual functions but not sure it
 has any derived classes (the Doxygen docs don't seem to indicate any?))
 
 There are some derived classes. See RetainCountChecker.cpp
 (CFRefLeakReport has an override).
Correct.
 
 ~Aaron


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] scan-build: Remove useless whitespace in File path

[Anna Zaks]

LGTM


REPOSITORY
  rL LLVM

http://reviews.llvm.org/D10354

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Ignore report when the argument to malloc is assigned known value

[Anna Zaks]

Comment at: test/Analysis/malloc-overflow.c:117
@@ +116,3 @@
+{
+  return malloc(n * 0 * sizeof(int));  // no-warning
+}

What is wrong with the current behavior of f15()?
ex.c:12:10: warning: Call to 'malloc' has an allocation size of 0 bytes
return malloc(n * 0 * sizeof(int));  // no-warning

http://reviews.llvm.org/D9924

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0).

[Anna Zaks]

Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:158
@@ +157,3 @@
+/// zero-allocated memory returned by 'realloc(ptr, 0)'.
+struct ReallocSizeZero {
+  void Profile(llvm::FoldingSetNodeID ID) const {

This struct does not contain any fields. What's its purpose?


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:524
@@ -511,2 +523,3 @@
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
+REGISTER_MAP_WITH_PROGRAMSTATE(ReallocSizeZeroFlag, SymbolRef, ReallocSizeZero)
 

Maybe you should use a set of SymbolRefs instead?

http://reviews.llvm.org/D9040

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Clang Static Analyzer] Bug identification

[Anna Zaks]

 This is not redundant, and I think we should include the file name in the 
 hash, otherwise bugs in copy pasted code-parts, but in different files will 
 have the same ID.


By redundant, I mean that this information is already encoded in the report; 
even if it's not part of the issue id. I can see this argument go either way. 
However, if we do decide to include the filename, we would need to change 
clang/utils/analyzer/CmpRuns.py and the current issue_hash so that it's all 
consistent.

  I believe these should be replaced with a bug identifier. The need for #6 
  highlights that the checker name is not sufficient here. A bug identifier 
  is the most 

 

   logical match for this info.

 

 

 This field is meant to differentiate between multiple reports by the exact 
 same checker at the same position (line, column). So in this case only the 
 checker

  writer can add additional information that can be a differentiator.


My point is that you would not need the extra field if you use bug type instead 
of checker id. We need an identifier that describes each type of a bug; instead 
of an identifier for a checker + a fuzzy extra field.

 Should I change to the current implementation to use only the name of the 
 template parameters? For example, T in the previous example.


What do you think? In this particular example, we would want both issues to be 
uniqued. I am sure we can construct an example where they should not be 
uniqued; not sure how rare that is. The best way to find out is to run this 
over a lot of C++ code and get the stats.


http://reviews.llvm.org/D10305

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Clang Static Analyzer] Bug identification

[Anna Zaks]

Thank you for working on this; this is a long needed improvement! Here are some 
high level comments.

 1. File name


Should we include redundancy in the hash? The file name is already part of the 
report. The current approach avoids redundancy but has a post processing step, 
where the true identifier is computed. See getIssueIdentifier of 
clang/utils/analyzer/CmpRuns.py.

 2. Content of the line where the bug is

 

 • So if anything changes in the close environment of the bug, it changes 
 the ID. We think that it is likely that the changes in the same line will 
 semantically affect the bug.


Close environment of the bug could potentially include other nodes on the 
diagnostic path. Have you considered including those as well? (This is food for 
thought, not a blocker for this patch.)

Have you seen PGOHash (./lib/CodeGen/CodeGenPGO.cpp), which creates a hash of 
the AST? Having a more semantic approach would be more robust. For example, it 
could protect from variable rename and other refactoring.

 4. Unique name of the checker

 5. Optional Extra field


I believe these should be replaced with a bug identifier. The need for #6 
highlights that the checker name is not sufficient here. A bug identifier is 
the most logical match for this info.

 Due to overloaded functions and copy pasted implementations, it is likely 
 that the same fault is found in two different overloaded functions.


What happens for bugs reported in template code?

 In the current code there exists a similar identifier generator to the one 
 suggested above. That implementation takes into consideration only

 • the name of the enclosing scope 

 • and the relative line number within the enclosing scope.

 This source of information is insufficient as the base of the hash for the 
 reasons described above.

 We included a version of the hash in the name of the key 
 (keybug_id_1/key) in the PLIST output to identify the hash generator 
 algorithm. This way it will be possible to introduce a new hash calculation 
 algorithm if needed.


I would be interested in either replacing issue_hash or adding 
issue_hash_bug_line_content (or something like it) instead of adding another 
completely differently named field with very similar information. I see no 
reason for having both. I am not sure if we have any users of issue_hash 
right now, who will suffer from the change. Maybe we could have issue_hash, 
issue_hash_1(offset based), and issue_hash_2(content of line) and add 
another field issue_hash_version that describes the version issue_hash is 
using?

This needs tests!!!


http://reviews.llvm.org/D10305

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Static Analyzer] Remove ObjCContainersChecker size information when a CFMutableArrayRef escapes

[Anna Zaks]

LGTM! Please, commit.


http://reviews.llvm.org/D10225

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0).

[Anna Zaks]

Here is a related enhancement request: 
https://llvm.org/bugs/show_bug.cgi?id=23695


http://reviews.llvm.org/D9040

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0).

[Anna Zaks]

I am not convinced that this is the right approach.

Adding warnings about use of zero allocated memory is fine. However, 
introducing the new leak warnings is not as this might be the behavior people 
expect.

Keep in mind that we try to minimize the number of false positives in the 
analyzer; even if that means we are reducing the number of true positives.


http://reviews.llvm.org/D9040

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Reject multiplication by zero cases in MallocOverflowSecurityChecker

[Anna Zaks]

Please, submit the patch with context as described on the llvm website.

Could you provide more motivation for this patch?


REPOSITORY
  rL LLVM


Comment at: lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp:61
@@ +60,3 @@
+// Return true for redundant computations.
+static bool RedundantComputation(APSInt Val, BinaryOperatorKind op) {
+  if (op == BO_Mul  Val == 0)

This is not a redundant computation, a redundant computation would be 1*x.

http://reviews.llvm.org/D9741

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Make realloc(ptr, 0) handling equivalent to malloc(0).

[Anna Zaks]

C89 says: If size is zero and ptr is not a null pointer, the object it points 
to is freed. I believe C89 and C99 disagree here.

I don't think we should add warnings for code that follows C89.


http://reviews.llvm.org/D9040

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: r236423 - [analyzer] scan-build: support spaces in compiler path and arguments.

[Anna Zaks]

Anton,

This has caused regressions on our internal ASan builedbot while analyzing 
openssl. (Please, revert until the issue is solved.)

/Users/buildslave/jenkins/workspace/Static_Analyzer_master/scan-build/ccc-analyzer
 -I. -I.. -I../include  -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN 
-DHAVE_DLFCN_H -arch i386 -O3 -fomit-frame-pointer -DL_ENDIAN 
-DOPENSSL_BN_ASM_PART_WORDS -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT 
-DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DRMD160_ASM -DAES_ASM 
-DWHIRLPOOL_ASM   -c -o cryptlib.o cryptlib.c
clang: error: no such file or directory: '-cc1'
clang: error: no such file or directory: '-triple'
clang: error: no such file or directory: 'i386-apple-macosx10.10.0'
clang: error: no such file or directory: '-analyze'
clang: error: no such file or directory: '-disable-free'
clang: error: no such file or directory: '-disable-llvm-verifier'
clang: error: no such file or directory: '-main-file-name'
clang: error: no such file or directory: 'cryptlib.c'
clang: error: no such file or directory: '-analyzer-store=region'
clang: error: no such file or directory: '-analyzer-opt-analyze-nested-blocks'
clang: error: no such file or directory: '-analyzer-eagerly-assume'
clang: error: no such file or directory: '-analyzer-checker=c

 On May 4, 2015, at 6:37 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 Author: ayartsev
 Date: Mon May  4 08:37:36 2015
 New Revision: 236423
 
 URL: http://llvm.org/viewvc/llvm-project?rev=236423view=rev
 Log:
 [analyzer] scan-build: support spaces in compiler path and arguments.
 
 This fixes errors that occur if a path to the default compiler has spaces or 
 if an argument with spaces is given to compiler (e.g. via -I). 
 (http://reviews.llvm.org/D9357)
 
 Modified:
cfe/trunk/tools/scan-build/ccc-analyzer
 
 Modified: cfe/trunk/tools/scan-build/ccc-analyzer
 URL: 
 http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/ccc-analyzer?rev=236423r1=236422r2=236423view=diff
 ==
 --- cfe/trunk/tools/scan-build/ccc-analyzer (original)
 +++ cfe/trunk/tools/scan-build/ccc-analyzer Mon May  4 08:37:36 2015
 @@ -145,7 +145,7 @@ sub ProcessClangFailure {
   print OUT @$Args\n;
   close OUT;
   `uname -a  $PPFile.info.txt 21`;
 -  `$Compiler -v  $PPFile.info.txt 21`;
 +  `$Compiler -v  $PPFile.info.txt 21`;
   rename($ofile, $PPFile.stderr.txt);
   return (basename $PPFile);
 }
 @@ -179,7 +179,7 @@ sub GetCCArgs {
   die could not find clang line\n if (!defined $line);
   # Strip leading and trailing whitespace characters.
   $line =~ s/^\s+|\s+$//g;
 -  my @items = quotewords('\s+', 0, $line);
 +  my @items = quotewords('\s+', 1, $line);
   my $cmd = shift @items;
   die cannot find 'clang' in 'clang' command\n if (!($cmd =~ /clang/));
   return \@items;
 
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker.

[Anna Zaks]

Yes, please, commit!

Thank you,
Anna.


http://reviews.llvm.org/D8273

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker.

[Anna Zaks]

Hi Anton,

Have you tested the patch on anything but the regression tests? If yes,
what are the results? Did this catch any issues? Are there any false
positives? Since this will be a turned on by default new warning, I'd like
make sure we test on real code before committing.

Other than testing, this looks good to me. Thank you!
Anna.

On Sat, Mar 21, 2015 at 7:19 AM, Антон Ярцев anton.yart...@gmail.com
wrote:

 .


 
 Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:889
 @@ +888,3 @@
 +const RefState *RS = State-getRegionState(Sym);
 +if (!RS || !RS-isAllocated())
 +  return State;
 
 ayartsev wrote:
  zaks.anna wrote:
   It should not be possible to have non allocated symbol here.. Is it?
 Maybe we should assert?
  Agree, done!
 Pardon, currently zero-allocated realloc do not attach a RefState so it is
 still early to assert for now.


Is there a test for this? If not, please add one.


 http://reviews.llvm.org/D8273

 EMAIL PREFERENCES
   http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker.

[Anna Zaks]

Hi Anton,

Have you tested the patch on anything but the regression tests? If yes,
what are the results? Did this catch any issues? Are there any false
positives? Since this will be a turned on by default new warning, I'd like
make sure we test on real code before committing.

Other than testing, this looks good to me. Thank you!
Anna.


http://reviews.llvm.org/D8273

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Improvements to scan-build.

[Anna Zaks]

Ok. Sounds good.

Sylvestre, 
Could you test this patch and the one labeled Prevent ccc/c++-analyzer from 
hanging on Windows.?

Anton, 
Does Sylvestre need an updated diff for the other patch?


http://reviews.llvm.org/D6551

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r233465 - [scan-build] Be friendly to in the argument list.

[Anna Zaks]

Author: zaks
Date: Fri Mar 27 21:17:21 2015
New Revision: 233465

URL: http://llvm.org/viewvc/llvm-project?rev=233465view=rev
Log:
[scan-build] Be friendly to  in the argument list.

Do not fail when  is one of the compilation arguments.

Modified:
cfe/trunk/tools/scan-build/ccc-analyzer

Modified: cfe/trunk/tools/scan-build/ccc-analyzer
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/ccc-analyzer?rev=233465r1=233464r2=233465view=diff
==
--- cfe/trunk/tools/scan-build/ccc-analyzer (original)
+++ cfe/trunk/tools/scan-build/ccc-analyzer Fri Mar 27 21:17:21 2015
@@ -492,6 +492,11 @@ foreach (my $i = 0; $i  scalar(@ARGV);
   my $Arg = $ARGV[$i];
   my ($ArgKey) = split /=/,$Arg,2;
 
+  # Be friendly to  in the argument list.
+  if (!defined($ArgKey)) {
+next;
+  }
+
   # Modes ccc-analyzer supports
   if ($Arg =~ /^-(E|MM?)$/) { $Action = 'preprocess'; }
   elsif ($Arg eq '-c') { $Action = 'compile'; }


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Improvements to scan-build.

[Anna Zaks]

Anton,
Is all of this just a refactoring? I've noticed a couple of places where new 
functionality is being added. If that is the case, could you split that into a 
separate patch, explaining why that is needed.

Sylvestre,

Thank you for the offer! It would be really great if you could help us with 
testing this. Which platforms are you using this on?

Anton has another patch for scan-build in review. See cfe-commits Prevent 
ccc/c++-analyzer from hanging on Windows. It might be best if you test all of 
them together. (Maybe Anton could send you the cumulative patch to save you 
time.)


http://reviews.llvm.org/D6551

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker.

[Anna Zaks]

Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:66
@@ -65,2 +65,3 @@
   unsigned K : 2; // Kind enum, but stored as a bitfield.
-  unsigned Family : 30; // Rest of 32-bit word, currently just an allocation 
+  unsigned ZeroAllocation : 1; // bool, true in case of a zero-size allocation.
+  unsigned Family : 29; // Rest of 32-bit word, currently just an allocation 

I think you could just fold it into the Kind, by adding AllocatedOfSizeZero or 
do we think that Relinquished or Escaped should be treated differently if they 
were zero allocated..?


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:844
@@ +843,3 @@
+// Performs a 0-sized allocations check.
+ProgramStateRef MallocChecker::ZeroAllocationCheck(CheckerContext C,
+   const Expr *E,

ProcessZeroAllocation ? We are not checking anything here.


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:889
@@ +888,3 @@
+const RefState *RS = State-getRegionState(Sym);
+if (!RS || !RS-isAllocated())
+  return State;

It should not be possible to have non allocated symbol here.. Is it? Maybe we 
should assert?


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1851
@@ +1850,3 @@
+  BT_UseZerroAllocated[*CheckKind].reset(new BugType(
+  CheckNames[*CheckKind], Use zero allocated, Memory Error));
+

I's call this Use of zero allocated or Zero allocation


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2302
@@ -2171,2 +2301,3 @@
   SymbolRef Sym = l.getLocSymbolInBase();
-  if (Sym)
+  const MemRegion *MR = l.getAsRegion()-StripCasts();
+

this seems unrelated to the patch. Can it be submitted separately with a 
testcase that it is trying to address?

http://reviews.llvm.org/D8273

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Implementation of potential undefbehavior.ZeroAllocDereference checker.

[Anna Zaks]

**As a rule of thumb, checkers should be stateless.

-

http://clang-analyzer.llvm.org/checker_dev_manual.html

When you introduce mutable members you are most likely making a mistake. The 
state should track properties of symbols; specifically to check with symbol 
corresponds to a '0' allocation.

The specific example that might break with your patch (depending on the order 
in which the states are being explored)  is something along these lines:
if (b)

  s= 10;

else

  s = 0;

p = malloc(s);
if (b)
 *p = 1;

When the checker explores malloc(s) along the s=0 path, the expression will 
be added to the set. If *p = 1 along the s=10 path is explored later on, we 
are going to produce a false positive.

Please, provide better testing so the cases like this one are exposed.


http://reviews.llvm.org/D8273

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.

[Anna Zaks]

 On Mar 7, 2015, at 12:43 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 On 07.03.2015 8:35, Anna Zaks wrote:
 
 On Mar 6, 2015, at 6:09 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Ok, I see, I wrongly used to think of MallockChecker as about more general 
 checker then it really is. If to consider CK_MismatchedDeallocatorChecker 
 an exception, then all other checks appear to be similar for a family and 
 there are always two types of checkers responsible for a family: a leaks 
 checker and a checker responsible for everything else. An additional bool 
 parameter to getCheckIfTracked() is sufficient in this case.
 Reverted an enhancement at r229593, additional cleanup at r231548
 
 Great! Thanks.
 
 Attach is the patch that adds an additional parameter to 
 getCheckIfTracked(), please review!
 
 +Optionalbool IsALeakCheck) const;
 Let’s replace this with a bool parameter with false as the default value. 
 This should simplify the patch. (We don’t need to differentiate between the 
 parameter not having a value and having a false value here.)
 Parameter not having a value means we do not now/care, if we want a leak 
 check, or not, like in MallocChecker::printState(). What to do in this case?
 

I think that in printState, we want to check both: if the symbol is tracked by 
a leak check and if the symbol is tracked by a non-leak check, so calling the 
method twice makes sense.

 
 Anna.
 Anton, 
 
 I am not convinced. Please, revert the patch until we agree on what is the 
 right thing to do here.
 
 More comments below.
 
 On Mar 6, 2015, at 7:03 AM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 On 06.03.2015 8:55, Anna Zaks wrote:
 
 On Mar 5, 2015, at 5:37 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 On 05.03.2015 21:39, Anna Zaks wrote:
 
 On Feb 17, 2015, at 4:39 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Author: ayartsev
 Date: Tue Feb 17 18:39:06 2015
 New Revision: 229593
 
 URL: http://llvm.org/viewvc/llvm-project?rev=229593view=rev 
 http://llvm.org/viewvc/llvm-project?rev=229593view=rev
 Log:
 [analyzer] Refactoring: clarified the way the proper check kind is 
 chosen.
 
 
 Anton, this doesn’t look like a simple refactoring. Also, the new API 
 looks more confusing and difficult to use. 
 
   auto CheckKind = getCheckIfTracked(C, DeallocExpr);
 vs
   auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,

 CK_MallocPessimistic,

 CK_NewDeleteChecker),
  C, DeallocExpr);
 
 Instead of checking if any of our checkers handle a specific family 
 and returning the one that does, we now have to pass in the list of 
 checkers we are interested in. Can you explain why this is needed? 
 
 I think this is a step in the wrong direction. My understanding is 
 that some of the methods only work for specific checkers (regardless 
 of the family being processed). Therefore, they returned early in case 
 they were called on checkers, where they are useless. Looks like you 
 are trying to fold that check into the API family check, which is 
 unrelated. Though, I might be missing something..
 Hi Anna!)
 
 Here is my very high level description on how this works:
 When reporting a bug, we call getCheckIfTracked(..) to find out which 
 check should be used to report it. (We might ocasionaly use the method 
 in some other context as well.) In most cases, we expect only one of the 
 checkers to track the symbol.
 
 The old getCheckIfTracked() has two drawbacks: first, it does not 
 considered CK_MismatchedDeallocatorChecker and CK_NewDeleteLeaksChecker 
 checkers. 
 
 I don’t think it should work with CK_MismatchedDeallocatorChecker as it 
 covers the case of mixed families. How is this API useful in that case? 
 In your implementation, you always return it back.
 The checker is returned back if the family of the given symbol fits the 
 checker, otherwise no checker is returned.
 
 I am talking about CK_MismatchedDeallocatorChecker here. This method does 
 not provide us with useful information when processing mismatched 
 deallocators. Don't try to overgeneralize and alter the API to fit in this 
 check. It does not fit by design.
 
 +  if (CK == CK_MismatchedDeallocatorChecker)
 +return CK;
 
 
 
 We can discuss the specifics of CK_NewDeleteLeaksChecker in more detail, 
 but my understanding is that the reason why it does not work is that we 
 want to be able to turn the DeleteLeaks off separately because it could 
 lead to false positives. Hopefully, that is a transitional limitation, 
 so we should not design the malloc checker around that.
 As you correctly described 'we call getCheckIfTracked(..) to find out 
 which check should be used to report the bug

Re: r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.

[Anna Zaks]

Anton, 

I am not convinced. Please, revert the patch until we agree on what is the 
right thing to do here.

More comments below.

 On Mar 6, 2015, at 7:03 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 On 06.03.2015 8:55, Anna Zaks wrote:
 
 On Mar 5, 2015, at 5:37 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 On 05.03.2015 21:39, Anna Zaks wrote:
 
 On Feb 17, 2015, at 4:39 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Author: ayartsev
 Date: Tue Feb 17 18:39:06 2015
 New Revision: 229593
 
 URL: http://llvm.org/viewvc/llvm-project?rev=229593view=rev 
 http://llvm.org/viewvc/llvm-project?rev=229593view=rev
 Log:
 [analyzer] Refactoring: clarified the way the proper check kind is chosen.
 
 
 Anton, this doesn’t look like a simple refactoring. Also, the new API 
 looks more confusing and difficult to use. 
 
   auto CheckKind = getCheckIfTracked(C, DeallocExpr);
 vs
   auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
CK_MallocPessimistic,
CK_NewDeleteChecker),
  C, DeallocExpr);
 
 Instead of checking if any of our checkers handle a specific family and 
 returning the one that does, we now have to pass in the list of checkers 
 we are interested in. Can you explain why this is needed? 
 
 I think this is a step in the wrong direction. My understanding is that 
 some of the methods only work for specific checkers (regardless of the 
 family being processed). Therefore, they returned early in case they were 
 called on checkers, where they are useless. Looks like you are trying to 
 fold that check into the API family check, which is unrelated. Though, I 
 might be missing something..
 Hi Anna!)
 
 Here is my very high level description on how this works:
 When reporting a bug, we call getCheckIfTracked(..) to find out which check 
 should be used to report it. (We might ocasionaly use the method in some 
 other context as well.) In most cases, we expect only one of the checkers to 
 track the symbol.
 
 The old getCheckIfTracked() has two drawbacks: first, it does not 
 considered CK_MismatchedDeallocatorChecker and CK_NewDeleteLeaksChecker 
 checkers. 
 
 I don’t think it should work with CK_MismatchedDeallocatorChecker as it 
 covers the case of mixed families. How is this API useful in that case? In 
 your implementation, you always return it back.
 The checker is returned back if the family of the given symbol fits the 
 checker, otherwise no checker is returned.

I am talking about CK_MismatchedDeallocatorChecker here. This method does not 
provide us with useful information when processing mismatched deallocators. 
Don't try to overgeneralize and alter the API to fit in this check. It does not 
fit by design.

 +  if (CK == CK_MismatchedDeallocatorChecker)
 +return CK;

 
 
 We can discuss the specifics of CK_NewDeleteLeaksChecker in more detail, but 
 my understanding is that the reason why it does not work is that we want to 
 be able to turn the DeleteLeaks off separately because it could lead to 
 false positives. Hopefully, that is a transitional limitation, so we should 
 not design the malloc checker around that.
 As you correctly described 'we call getCheckIfTracked(..) to find out which 
 check should be used to report the bug'. Old implementation just returned 
 CK_MallockChecker for AF_Malloc family and CK_NewDeleteChecker for 
 AF_CXXNew/AF_CXXNewArray families which is correct only in the case, when 
 CK_MallockChecker and CK_NewDeleteChecker are 'on' and all other are 'off'.

 I agree, most reports belong to CK_MallockChecker and CK_NewDeleteChecker 
 checkers, but why not to make getCheckIfTracked(..) return the proper check 
 in all cases?

What is the proper check? I believe this method should return a single 
matched check and not depend on the order of checks in the input array, which 
is confusing and error prone. 
For that we need to decide what to do in cases where there is no 1-to-1 
correspondence between families and checkers. There are 2 cases:
 - CK_MismatchedDeallocatorChecker // It is not useful to have the method cover 
this. I think mismatched deallocator checking should be special cased. (We 
don't have to call this method every time a bug is reported.)
 - Leaks // We may want to have leaks be reported by separate checks. For that, 
we can pass a boolean to the getCheckIfTracked to specify if we want leak check 
or a regular check. It would return MallocChecker for malloc family since the 
leaks check is not separate there.


 Consider the use of the new API, for example, in ReportFreeAlloca(). However 
 much new checks/checkers/families we add the new API will remain usable.
 Concerning the CK_NewDeleteLeaksChecker checker, currently 
 CK_NewDeleteLeaksChecker is considered a part of CK_NewDelete checker. 
 Technically

Re: r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.

[Anna Zaks]

 On Mar 6, 2015, at 6:09 PM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 Ok, I see, I wrongly used to think of MallockChecker as about more general 
 checker then it really is. If to consider CK_MismatchedDeallocatorChecker an 
 exception, then all other checks appear to be similar for a family and there 
 are always two types of checkers responsible for a family: a leaks checker 
 and a checker responsible for everything else. An additional bool parameter 
 to getCheckIfTracked() is sufficient in this case.
 Reverted an enhancement at r229593, additional cleanup at r231548
 
Great! Thanks.

 Attach is the patch that adds an additional parameter to getCheckIfTracked(), 
 please review!
 
+Optionalbool IsALeakCheck) const;
Let’s replace this with a bool parameter with false as the default value. This 
should simplify the patch. (We don’t need to differentiate between the 
parameter not having a value and having a false value here.)

Anna.
 Anton, 
 
 I am not convinced. Please, revert the patch until we agree on what is the 
 right thing to do here.
 
 More comments below.
 
 On Mar 6, 2015, at 7:03 AM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 On 06.03.2015 8:55, Anna Zaks wrote:
 
 On Mar 5, 2015, at 5:37 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 On 05.03.2015 21:39, Anna Zaks wrote:
 
 On Feb 17, 2015, at 4:39 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Author: ayartsev
 Date: Tue Feb 17 18:39:06 2015
 New Revision: 229593
 
 URL: http://llvm.org/viewvc/llvm-project?rev=229593view=rev 
 http://llvm.org/viewvc/llvm-project?rev=229593view=rev
 Log:
 [analyzer] Refactoring: clarified the way the proper check kind is 
 chosen.
 
 
 Anton, this doesn’t look like a simple refactoring. Also, the new API 
 looks more confusing and difficult to use. 
 
   auto CheckKind = getCheckIfTracked(C, DeallocExpr);
 vs
   auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
CK_MallocPessimistic,
CK_NewDeleteChecker),
  C, DeallocExpr);
 
 Instead of checking if any of our checkers handle a specific family and 
 returning the one that does, we now have to pass in the list of checkers 
 we are interested in. Can you explain why this is needed? 
 
 I think this is a step in the wrong direction. My understanding is that 
 some of the methods only work for specific checkers (regardless of the 
 family being processed). Therefore, they returned early in case they 
 were called on checkers, where they are useless. Looks like you are 
 trying to fold that check into the API family check, which is unrelated. 
 Though, I might be missing something..
 Hi Anna!)
 
 Here is my very high level description on how this works:
 When reporting a bug, we call getCheckIfTracked(..) to find out which 
 check should be used to report it. (We might ocasionaly use the method in 
 some other context as well.) In most cases, we expect only one of the 
 checkers to track the symbol.
 
 The old getCheckIfTracked() has two drawbacks: first, it does not 
 considered CK_MismatchedDeallocatorChecker and CK_NewDeleteLeaksChecker 
 checkers. 
 
 I don’t think it should work with CK_MismatchedDeallocatorChecker as it 
 covers the case of mixed families. How is this API useful in that case? In 
 your implementation, you always return it back.
 The checker is returned back if the family of the given symbol fits the 
 checker, otherwise no checker is returned.
 
 I am talking about CK_MismatchedDeallocatorChecker here. This method does 
 not provide us with useful information when processing mismatched 
 deallocators. Don't try to overgeneralize and alter the API to fit in this 
 check. It does not fit by design.
 
 +  if (CK == CK_MismatchedDeallocatorChecker)
 +return CK;
 
 
 
 We can discuss the specifics of CK_NewDeleteLeaksChecker in more detail, 
 but my understanding is that the reason why it does not work is that we 
 want to be able to turn the DeleteLeaks off separately because it could 
 lead to false positives. Hopefully, that is a transitional limitation, so 
 we should not design the malloc checker around that.
 As you correctly described 'we call getCheckIfTracked(..) to find out which 
 check should be used to report the bug'. Old implementation just returned 
 CK_MallockChecker for AF_Malloc family and CK_NewDeleteChecker for 
 AF_CXXNew/AF_CXXNewArray families which is correct only in the case, when 
 CK_MallockChecker and CK_NewDeleteChecker are 'on' and all other are 'off'. 
 
 I agree, most reports belong to CK_MallockChecker and CK_NewDeleteChecker 
 checkers, but why not to make getCheckIfTracked(..) return the proper check 
 in all cases? 
 
 What is the proper check? I believe this method should return

Re: r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.

[Anna Zaks]

 On Feb 17, 2015, at 4:39 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Author: ayartsev
 Date: Tue Feb 17 18:39:06 2015
 New Revision: 229593
 
 URL: http://llvm.org/viewvc/llvm-project?rev=229593view=rev 
 http://llvm.org/viewvc/llvm-project?rev=229593view=rev
 Log:
 [analyzer] Refactoring: clarified the way the proper check kind is chosen.
 

Anton, this doesn’t look like a simple refactoring. Also, the new API looks 
more confusing and difficult to use. 

  auto CheckKind = getCheckIfTracked(C, DeallocExpr);
vs
  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
   CK_MallocPessimistic,
   CK_NewDeleteChecker),
 C, DeallocExpr);

Instead of checking if any of our checkers handle a specific family and 
returning the one that does, we now have to pass in the list of checkers we are 
interested in. Can you explain why this is needed? 

I think this is a step in the wrong direction. My understanding is that some of 
the methods only work for specific checkers (regardless of the family being 
processed). Therefore, they returned early in case they were called on 
checkers, where they are useless. Looks like you are trying to fold that check 
into the API family check, which is unrelated. Though, I might be missing 
something..

 Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
 
 Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
 URL: 
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593r1=229592r2=229593view=diff
  
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593r1=229592r2=229593view=diff
 ==
 --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
 +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Tue Feb 17 
 18:39:06 2015
 @@ -184,6 +184,7 @@ public:
 
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckName CheckNames[CK_NumCheckKinds];
 +  typedef llvm::SmallVectorCheckKind, CK_NumCheckKinds CKVecTy;
 
   void checkPreCall(const CallEvent Call, CheckerContext C) const;
   void checkPostStmt(const CallExpr *CE, CheckerContext C) const;
 @@ -327,12 +328,16 @@ private:
 
   ///@{
   /// Tells if a given family/call/symbol is tracked by the current checker.
 -  /// Sets CheckKind to the kind of the checker responsible for this
 -  /// family/call/symbol.
 -  OptionalCheckKind getCheckIfTracked(AllocationFamily Family) const;
 -  OptionalCheckKind getCheckIfTracked(CheckerContext C,
 +  /// Looks through incoming CheckKind(s) and returns the kind of the 
 checker 
 +  /// responsible for this family/call/symbol.

Is it possible for more than one checker to be responsible for the same family? 
This returns the first checker that handles the family from the given list.

 +  OptionalCheckKind getCheckIfTracked(CheckKind CK,
 +AllocationFamily Family) const;

This always returns either the input checker or an empty one. Looks like it 
should just return a bool...

 +  OptionalCheckKind getCheckIfTracked(CKVecTy CKVec, 

Hard to tell what this argument is from documentation/name.

 +AllocationFamily Family) const;
 +  OptionalCheckKind getCheckIfTracked(CKVecTy CKVec, CheckerContext C,
 const Stmt *AllocDeallocStmt) const;
 -  OptionalCheckKind getCheckIfTracked(CheckerContext C, SymbolRef Sym) 
 const;
 +  OptionalCheckKind getCheckIfTracked(CKVecTy CKVec, CheckerContext C,
 +SymbolRef Sym) const;
   ///@}
   static bool SummarizeValue(raw_ostream os, SVal V);
   static bool SummarizeRegion(raw_ostream os, const MemRegion *MR);
 @@ -1310,21 +1315,32 @@ ProgramStateRef MallocChecker::FreeMemAu
 }
 
 OptionalMallocChecker::CheckKind
 -MallocChecker::getCheckIfTracked(AllocationFamily Family) const {
 +MallocChecker::getCheckIfTracked(MallocChecker::CheckKind CK,
 + AllocationFamily Family) const {
 +
 +  if (CK == CK_NumCheckKinds || !ChecksEnabled[CK])
 +return OptionalMallocChecker::CheckKind();
 +
 +  // C/C++ checkers.
 +  if (CK == CK_MismatchedDeallocatorChecker)
 +return CK;
 +
   switch (Family) {
   case AF_Malloc:
   case AF_IfNameIndex: {
 -if (ChecksEnabled[CK_MallocOptimistic]) {
 -  return CK_MallocOptimistic;
 -} else if (ChecksEnabled[CK_MallocPessimistic]) {
 -  return CK_MallocPessimistic;
 +// C checkers.
 +if (CK == CK_MallocOptimistic ||
 +CK == CK_MallocPessimistic) {
 +  return CK;
 }
 return OptionalMallocChecker::CheckKind();
   }
   case AF_CXXNew:
   case AF_CXXNewArray: {
 -if 

Re: r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.

[Anna Zaks]

 On Mar 5, 2015, at 5:37 PM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 On 05.03.2015 21:39, Anna Zaks wrote:
 
 On Feb 17, 2015, at 4:39 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Author: ayartsev
 Date: Tue Feb 17 18:39:06 2015
 New Revision: 229593
 
 URL: http://llvm.org/viewvc/llvm-project?rev=229593view=rev 
 http://llvm.org/viewvc/llvm-project?rev=229593view=rev
 Log:
 [analyzer] Refactoring: clarified the way the proper check kind is chosen.
 
 
 Anton, this doesn’t look like a simple refactoring. Also, the new API looks 
 more confusing and difficult to use. 
 
   auto CheckKind = getCheckIfTracked(C, DeallocExpr);
 vs
   auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
CK_MallocPessimistic,
CK_NewDeleteChecker),
  C, DeallocExpr);
 
 Instead of checking if any of our checkers handle a specific family and 
 returning the one that does, we now have to pass in the list of checkers we 
 are interested in. Can you explain why this is needed? 
 
 I think this is a step in the wrong direction. My understanding is that some 
 of the methods only work for specific checkers (regardless of the family 
 being processed). Therefore, they returned early in case they were called on 
 checkers, where they are useless. Looks like you are trying to fold that 
 check into the API family check, which is unrelated. Though, I might be 
 missing something..
 Hi Anna!)

Here is my very high level description on how this works:
When reporting a bug, we call getCheckIfTracked(..) to find out which check 
should be used to report it. (We might ocasionaly use the method in some other 
context as well.) In most cases, we expect only one of the checkers to track 
the symbol.

 The old getCheckIfTracked() has two drawbacks: first, it does not considered 
 CK_MismatchedDeallocatorChecker and CK_NewDeleteLeaksChecker checkers. 

I don’t think it should work with CK_MismatchedDeallocatorChecker as it covers 
the case of mixed families. How is this API useful in that case? In your 
implementation, you always return it back.

We can discuss the specifics of CK_NewDeleteLeaksChecker in more detail, but my 
understanding is that the reason why it does not work is that we want to be 
able to turn the DeleteLeaks off separately because it could lead to false 
positives. Hopefully, that is a transitional limitation, so we should not 
design the malloc checker around that.

On the other hand, we should design this to be easily extendable to handle more 
families, and this patch hampers that. You’d need to grow the list of checkers 
you send to each call to this function for every new family. Ex: KeychainAPI 
checker should be folded into this. 

 The second is that there is, in fact, unable to customize the set of checkers 
 getCheckIfTracked() chooses from. For each family there are several checkers 
 responsible for it. Without providing the set of checkers of interest 
 getCheckIfTracked() is ugly in use.

 Consider changes in MallocChecker::reportLeak() below - the removed block of 
 code (marked start and end of the code with - for you). This 
 piece was just added for situations (hard to guess looking at the code), 
 when, for example, CK_MallocPessimistic and CK_NewDelete are 'on' and 
 CK_NewDeleteLeaksChecker is 'off' and in this case getCheckIfTracked() 
 returns CK_NewDelete checker as the checker, responsible for the 
 AF_CXXNew/AF_CXXNewArray families. The code looks confusing in consideration 
 of the fact that we rejected all the checkers responsible for 
 AF_CXXNew/AF_CXXNewArray families, except CK_NewDeleteLeaksChecker, by 
 writing 'if (...  !ChecksEnabled[CK_NewDeleteLeaksChecker]) return;' at the 
 beginning of the method. In the current implementation getCheckIfTracked() 
 returns only the checkers it was restricted for.

I think it’s better to have one ugly spot that handles a corner case such as 
DeleteLeaks. (If we want all leak checks to be separate, we can design a 
solution for that as well. Maybe a boolean argument is passed in whenever we 
are processing a leak?)

 
 The second bonus of the current implementation is that it gets us rid of the 
 check for specific checkers at the beginning. 
 
 Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
 
 Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
 URL: 
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593r1=229592r2=229593view=diff
  
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593r1=229592r2=229593view=diff
 ==
 --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
 +++ cfe/trunk/lib/StaticAnalyzer/Checkers

Re: [PATCH] [Analyzer] Individual options for checkers #2

[Anna Zaks]

 Regarding the options validation on the frontend:

 Indeed, it would be nicer to use a clang method to check if a string is an 
 identifier however I did not found a nice way to do that. The only way I 
 came up with was to create a raw lexer and lex the option string. If you 
 consider that better than using a RegEx I will alter the patch accordingly.


It would also be good to check that the names of the checkers and packages are 
identifiers and that the option is a fully qualified checker name followed by 
the option name... Let's create a separate patch for validation and check if it 
can be done more elegantly.

The rest of the patch LGTM and can land. Thank you for working on this!


http://reviews.llvm.org/D7905

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Analyzer] Individual options for checkers #2

[Anna Zaks]

Could you include more context with the patch? See: 
http://llvm.org/docs/Phabricator.html

Looks like we don't have a public API for options with string values.

Thanks!
Anna.



Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:275
@@ +274,3 @@
+  /// ones.
+  / @retval CheckerOptionValue  An option for a checker if it was specified
+  /// @retval GroupOptionValue  An option for group if it was specified and no

Extra //.
Nit: Please, make sure all comments have proper punctuation. (A period is 
missing here and in a few other places.)


Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:288
@@ -257,3 +287,3 @@
   ///
   /// Accepts the strings true and false.
   /// If an option value is not provided, returns the given \p DefaultVal.

This is a bit confusing.


Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:311
@@ +310,3 @@
+  /// @param [in] C Optional parameter that may be used to retrieve
+  /// checker-related option for a given checker
+  /// @param [in] SearchInParents If set to true and the searched option was 
not

This might be more clear: The optional checker parameter that can be used to 
restrict the search to the options of this particular checker.


Comment at: lib/Frontend/CompilerInvocation.cpp:140
@@ -134,1 +139,3 @@
+}
+
 static bool ParseAnalyzerArgs(AnalyzerOptions Opts, ArgList Args,

Shouldn't we use an existing clang check to check if substrings are identifiers?



Comment at: lib/StaticAnalyzer/Core/AnalyzerOptions.cpp:110
@@ +109,3 @@
+  // Search for a category option if option for checker is not specified and
+  // inheritance is enabled.
+  ConfigTable::const_iterator E = Config.end();

The comment needs to move and be rephrased (there is no inheritance and 
checkers are organized in packages, not categories).



Comment at: unittests/Analysis/AnalyzerOptionsTest.cpp:47
@@ +46,2 @@
+} // end namespace ast_matchers
+} // end namespace clang

I think we need more tests. Ex: overridden options, options set only in a 
package.


Comment at: unittests/CMakeLists.txt:16
@@ -15,2 +15,3 @@
 if(CLANG_ENABLE_STATIC_ANALYZER)
+  add_subdirectory(Analysis)
   add_subdirectory(Frontend)

Analysis is too vague if we only plan to use this for the clang static 
analyzer.

http://reviews.llvm.org/D7905

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Analyzer] Individual options for checkers #2

[Anna Zaks]

 Right now there is no way for a checker to specify the package or checker 
 name when querying an option. The getCheckerOption function is private 

 (the config table is public tough). The checkers can use getBooleanOption 
 with a pointer to the checker, this means filling the package name and 
 checker name is automatic. With this API a checker can not query the options 
 of an unrelated package and there is no way to misspell the package or 

 the checker name.


The model I was going for was simpler. A checker would be allowed to query 
options that are set on itself or a given package. For that, we would want to 
expose the string based APIs to the checkers. However, this solution is good as 
well and does have a benefit of being less error prone. The only downside that 
I see with this solution is that a checker will not be able to search options 
of other checkers/packages. Maybe it's fine to disallow that. What do you think?

I would like to stay away from mentioning inheritance in these APIs. Here is 
a summary of the thread where this was discussed (on the Static Analyzer 
Checker Options Proposal thread):

Anna (initial proposal):
However, there will be no inheritance (i.e. the setting 'unix:Optimistic' is 
entirely distinct from the setting ‘unix.Malloc:Optimistic’).
Gabor Kozar:
I think inheritance like that could be useful in some situations.
Anna:
The main reason is that we currently do not have a need for it and allowing 
inheritance requires a design for it. For example, what happens when a package 
adds an option? How would the checkers access it? If we were to allow 
dynamically loaded checkers how would they inherit it? What happens if a 
checker overrides a package option?
Gabor Kozar:
I would expect such options to be inherited, i.e. if I set 'unix:Optimistic', 
then I expect this to be visible everywhere in the 'unix' package. Why are 
disallowing this?
Anna:
The package options will be visible to checkers; specifically, checkers could 
query the package options. For example, MallocChecker could call 
getOption(Optimistic, unix) to check if the option has been set on the 
package. 
Gabor Kozar:
How about the getOption() function getting a boolean parameter specifying 
whether to allow inherited options, with the default being true?

getOption(unix.stuff.moo.FooChecker, MyOption, true) would then be 
equivalent to trying to all of these:
unix.stuff.moo.FooChecker:MyOption
unix.stuff.moo:MyOption
unix.stuff:MyOption
unix:MyOption
I think this behavior would be clear and straight-forward.
Anna:
This looks good to me. I was mainly thinking/talking about implicit 
inheritance. Improving the way checkers can query options is a definite plus.

Given the above, the name OptionKind is confusing. (It implies that options 
have kinds, but we don't specify/enforce option kinds anywhere. We don't have a 
notion of an inherited option that will be automatically applied to all 
checkers in that package.) It's not an option kind but rather a search mode 
requested by the checker. It could be as easy as just a bool - search for the 
option in all parent packages or only search in this checker. In this patch, 
you allow many more ways of searching (OptionKind), which is fine if well 
documented and tested. However, if no one needs to all of these, implementing 
them could be an overkill at this point. It also complicated the API. We also 
need to document what happens when an option is overriden. The more kinds we 
have, the more complex the rule will be.

 I think the most appropriate way to test this feature would be to write some 
 unit tests for AnalyzerOptions class. However there are no 

  unittests for the static analyzer yet. Should I create some as part of this 
 patch?


If you volunteer to do this, that would be awesome! There is no good way of 
testing these now.


http://reviews.llvm.org/D7905

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Analyzer] Individual options for checkers #2

[Anna Zaks]

I'd also have Optimistic be an option of malloc checker, not unix package 
since there are no other optimistic/pessimistic checkers in unix. The only 
reason to do that would be testing, but maybe we could test in other ways.

Also, It would be good to have this extra checking, but can come as port of a 
later commit:

To avoid ambiguities with regular options, we should enforce the following:

1. Option Name should be an identifier
2. Checker names should be identifiers.
3. Package names should be identifiers joined with '.’.
4. Full Checker Name has the same form as package names.



Thank you!
Anna.



Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:191
@@ -179,1 +190,3 @@
+  };
+
 private:

We were trying to stay away from inheritance of options not to imply that the 
checkers will automatically inherit options. See the discussion on the proposal 
thread:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-October/039576.html


Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:291
@@ +290,3 @@
+ StringRef Default,
+ OptionKind Kind = OptionKind::Global);
+

I'd simplify this and have it take a book instead of the Kind.

http://reviews.llvm.org/D7905

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Proposal] [Analyzer] Individual options for checkers

[Anna Zaks]

Alexey,

There must have been some miscommunication. I've replied the following on the 
thread discussing the Static Analyzer Checker Options Proposal. As far as I 
can tell, your implementation differs to what was proposed. I have not received 
any reply to that. We are interested in this functionality, but do not agree 
with some aspects of this patch.


On Nov 14, 2014, at 11:24 AM, Anna Zaks ga...@apple.com wrote:

Hi Aleksei,

I believe that with your implementation, it's assumed that the checker always 
wants to inherit the options of the parent package (it’s not opt in).

Also, since you don’t support query by checker/package name, there is no 
ability for the checker to query the package options.

Anna.



http://reviews.llvm.org/D3967

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Include checker name in Static Analyzer PLIST output

[Anna Zaks]

Please, commit!
Anna.


http://reviews.llvm.org/D6841

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r228248 - [analyzer] Do not crash in the KeychainAPI checker on user defined 'free()'.

[Anna Zaks]

Author: zaks
Date: Wed Feb  4 19:02:56 2015
New Revision: 228248

URL: http://llvm.org/viewvc/llvm-project?rev=228248view=rev
Log:
[analyzer] Do not crash in the KeychainAPI checker on user defined 'free()'.

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
cfe/trunk/test/Analysis/redefined_system.c

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp?rev=228248r1=228247r2=228248view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp Wed Feb  
4 19:02:56 2015
@@ -292,7 +292,11 @@ void MacOSKeychainAPIChecker::checkPreSt
   // If it is a call to an allocator function, it could be a double allocation.
   idx = getTrackedFunctionIndex(funName, true);
   if (idx != InvalidIdx) {
-const Expr *ArgExpr = CE-getArg(FunctionsToTrack[idx].Param);
+unsigned paramIdx = FunctionsToTrack[idx].Param;
+if (CE-getNumArgs() = paramIdx)
+  return;
+
+const Expr *ArgExpr = CE-getArg(paramIdx);
 if (SymbolRef V = getAsPointeeSymbol(ArgExpr, C))
   if (const AllocationState *AS = State-getAllocatedData(V)) {
 if (!definitelyReturnedError(AS-Region, State, C.getSValBuilder())) {
@@ -325,8 +329,12 @@ void MacOSKeychainAPIChecker::checkPreSt
   if (idx == InvalidIdx)
 return;
 
+  unsigned paramIdx = FunctionsToTrack[idx].Param;
+  if (CE-getNumArgs() = paramIdx)
+return;
+
   // Check the argument to the deallocator.
-  const Expr *ArgExpr = CE-getArg(FunctionsToTrack[idx].Param);
+  const Expr *ArgExpr = CE-getArg(paramIdx);
   SVal ArgSVal = State-getSVal(ArgExpr, C.getLocationContext());
 
   // Undef is reported by another checker.

Modified: cfe/trunk/test/Analysis/redefined_system.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/redefined_system.c?rev=228248r1=228247r2=228248view=diff
==
--- cfe/trunk/test/Analysis/redefined_system.c (original)
+++ cfe/trunk/test/Analysis/redefined_system.c Wed Feb  4 19:02:56 2015
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=unix,core,alpha.security.taint 
-w -verify %s
+// RUN: %clang_cc1 -analyze 
-analyzer-checker=osx,unix,core,alpha.security.taint -w -verify %s
 // expected-no-diagnostics
 
 // Make sure we don't crash when someone redefines a system function we reason 
about.


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r228246 - [analyzer] Don't skip analyzing the functions in preprocessed files.

[Anna Zaks]

Author: zaks
Date: Wed Feb  4 19:02:47 2015
New Revision: 228246

URL: http://llvm.org/viewvc/llvm-project?rev=228246view=rev
Log:
[analyzer] Don't skip analyzing the functions in preprocessed files.

The change in main file detection ended up disabling the path-sensitive
analysis of functions within preprocessed files.

Modified:
cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp?rev=228246r1=228245r2=228246view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp Wed Feb  4 
19:02:47 2015
@@ -590,7 +590,7 @@ AnalysisConsumer::getModeForDecl(Decl *D
   // - System headers: don't run any checks.
   SourceManager SM = Ctx-getSourceManager();
   SourceLocation SL = SM.getExpansionLoc(D-getLocation());
-  if (!Opts-AnalyzeAll  !SM.isInMainFile(SL)) {
+  if (!Opts-AnalyzeAll  !SM.isWrittenInMainFile(SL)) {
 if (SL.isInvalid() || SM.isInSystemHeader(SL))
   return AM_None;
 return Mode  ~AM_Path;


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r228247 - [analyzer] Look for allocation site in the parent frames as well as the current one.

[Anna Zaks]

Author: zaks
Date: Wed Feb  4 19:02:53 2015
New Revision: 228247

URL: http://llvm.org/viewvc/llvm-project?rev=228247view=rev
Log:
[analyzer] Look for allocation site in the parent frames as well as the current 
one.

Instead of handling edge cases (mostly involving blocks), where we have 
difficulty finding
an allocation statement, allow the allocation site to be in a parent node.

Previously we assumed that the allocation site can always be found in the same 
frame
as allocation, but there are scenarios in which an element is leaked in a child
frame but is allocated in the parent.

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
cfe/trunk/test/Analysis/objc-radar17039661.m

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp?rev=228247r1=228246r2=228247view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp Wed Feb  
4 19:02:53 2015
@@ -499,9 +499,11 @@ MacOSKeychainAPIChecker::getAllocationNo
   while (N) {
 if (!N-getState()-getAllocatedData(Sym))
   break;
-// Allocation node, is the last node in the current context in which the
-// symbol was tracked.
-if (N-getLocationContext() == LeakContext)
+// Allocation node, is the last node in the current or parent context in
+// which the symbol was tracked.
+const LocationContext *NContext = N-getLocationContext();
+if (NContext == LeakContext ||
+NContext-isParentOf(LeakContext))
   AllocNode = N;
 N = N-pred_empty() ? nullptr : *(N-pred_begin());
   }

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=228247r1=228246r2=228247view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Wed Feb  4 19:02:53 
2015
@@ -1801,9 +1801,11 @@ MallocChecker::getAllocationSite(const E
 }
   }
 
-// Allocation node, is the last node in the current context in which the
-// symbol was tracked.
-if (N-getLocationContext() == LeakContext)
+// Allocation node, is the last node in the current or parent context in
+// which the symbol was tracked.
+const LocationContext *NContext = N-getLocationContext();
+if (NContext == LeakContext ||
+NContext-isParentOf(LeakContext))
   AllocNode = N;
 N = N-pred_empty() ? nullptr : *(N-pred_begin());
   }

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp?rev=228247r1=228246r2=228247view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Wed Feb  4 
19:02:53 2015
@@ -2190,7 +2190,7 @@ static AllocationInfo
 GetAllocationSite(ProgramStateManager StateMgr, const ExplodedNode *N,
   SymbolRef Sym) {
   const ExplodedNode *AllocationNode = N;
-  const ExplodedNode *AllocationNodeInCurrentContext = N;
+  const ExplodedNode *AllocationNodeInCurrentOrParentContext = N;
   const MemRegion *FirstBinding = nullptr;
   const LocationContext *LeakContext = N-getLocationContext();
 
@@ -2220,10 +2220,15 @@ GetAllocationSite(ProgramStateManager S
 // AllocationNode is the last node in which the symbol was tracked.
 AllocationNode = N;
 
-// AllocationNodeInCurrentContext, is the last node in the current context
-// in which the symbol was tracked.
-if (NContext == LeakContext)
-  AllocationNodeInCurrentContext = N;
+// AllocationNodeInCurrentContext, is the last node in the current or
+// parent context in which the symbol was tracked.
+//
+// Note that the allocation site might be in the parent conext. For 
example,
+// the case where an allocation happens in a block that captures a 
reference
+// to it and that reference is overwritten/dropped by another call to
+// the block.
+if (NContext == LeakContext || NContext-isParentOf(LeakContext))
+  AllocationNodeInCurrentOrParentContext = N;
 
 // Find the last init that was called on the given symbol and store the
 // init method's location context.
@@ -2261,7 +2266,7 @@ GetAllocationSite(ProgramStateManager S
 FirstBinding = nullptr;
   }
 
-  return 

r228249 - [analyzer] Relax an assertion in VisitLvalArraySubscriptExpr

[Anna Zaks]

Author: zaks
Date: Wed Feb  4 19:02:59 2015
New Revision: 228249

URL: http://llvm.org/viewvc/llvm-project?rev=228249view=rev
Log:
[analyzer] Relax an assertion in VisitLvalArraySubscriptExpr

The analyzer thinks that ArraySubscriptExpr cannot be an r-value (ever).
However, it can be in some corner cases. Specifically, C forbids expressions
of unqualified void type from being l-values.

Note, the analyzer will keep modeling the subscript expr as an l-value. The
analyzer should be treating void* as a char array
(https://gcc.gnu.org/onlinedocs/gcc-4.3.0/gcc/Pointer-Arith.html).

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
cfe/trunk/test/Analysis/array-struct.c

Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=228249r1=228248r2=228249view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Wed Feb  4 19:02:59 2015
@@ -1901,6 +1901,9 @@ void ExprEngine::VisitLvalArraySubscript
   getCheckerManager().runCheckersForPreStmt(checkerPreStmt, Pred, A, *this);
 
   StmtNodeBuilder Bldr(checkerPreStmt, Dst, *currBldrCtx);
+  assert(A-isGLValue() ||
+  (!AMgr.getLangOpts().CPlusPlus 
+   A-getType().isCForbiddenLValueType()));
 
   for (ExplodedNodeSet::iterator it = checkerPreStmt.begin(),
  ei = checkerPreStmt.end(); it != ei; ++it) {
@@ -1909,7 +1912,6 @@ void ExprEngine::VisitLvalArraySubscript
 SVal V = state-getLValue(A-getType(),
   state-getSVal(Idx, LCtx),
   state-getSVal(Base, LCtx));
-assert(A-isGLValue());
 Bldr.generateNode(A, *it, state-BindExpr(A, LCtx, V), nullptr,
   ProgramPoint::PostLValueKind);
   }

Modified: cfe/trunk/test/Analysis/array-struct.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/array-struct.c?rev=228249r1=228248r2=228249view=diff
==
--- cfe/trunk/test/Analysis/array-struct.c (original)
+++ cfe/trunk/test/Analysis/array-struct.c Wed Feb  4 19:02:59 2015
@@ -183,3 +183,19 @@ int offset_of_data_array(void)
   return ((char *)(((struct s*)0)-data_array)) - ((char *)0); // no-warning
 }
 
+int testPointerArithmeticOnVoid(void *bytes) {
+  int p = 0;
+  if (bytes[0] == bytes[1])
+return 6/p; // no-warning
+  return 0;
+}
+
+int testRValueArraySubscriptExpr(void *bytes) {
+  int *p = (int*)bytes[0];
+  *p = 0;
+  if (*(int*)bytes[0] == 0)
+return 0;
+  return 5/(*p); // no-warning
+}
+
+


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r228119 - Register parameters have local storage.

[Anna Zaks]

Author: zaks
Date: Wed Feb  4 01:15:12 2015
New Revision: 228119

URL: http://llvm.org/viewvc/llvm-project?rev=228119view=rev
Log:
Register parameters have local storage.

Fixes a regression introduced in r209149.

Modified:
cfe/trunk/include/clang/AST/Decl.h
cfe/trunk/test/Analysis/stack-addr-ps.c

Modified: cfe/trunk/include/clang/AST/Decl.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=228119r1=228118r2=228119view=diff
==
--- cfe/trunk/include/clang/AST/Decl.h (original)
+++ cfe/trunk/include/clang/AST/Decl.h Wed Feb  4 01:15:12 2015
@@ -840,7 +840,7 @@ public:
   return !isFileVarDecl()  getTSCSpec() == TSCS_unspecified;
 
 // Global Named Register (GNU extension)
-if (getStorageClass() == SC_Register  !isLocalVarDecl())
+if (getStorageClass() == SC_Register  !isLocalVarDeclOrParm())
   return false;
 
 // Return true for:  Auto, Register.

Modified: cfe/trunk/test/Analysis/stack-addr-ps.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/stack-addr-ps.c?rev=228119r1=228118r2=228119view=diff
==
--- cfe/trunk/test/Analysis/stack-addr-ps.c (original)
+++ cfe/trunk/test/Analysis/stack-addr-ps.c Wed Feb  4 01:15:12 2015
@@ -90,3 +90,10 @@ RDar10348049 test_rdar10348049(void) {
   return b; // no-warning
 }
 
+void testRegister(register const char *reg) {
+if (reg) (void)reg[0];
+}
+void callTestRegister() {
+char buf[20];
+testRegister(buf); // no-warning
+}


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Include checker name in Static Analyzer PLIST output

[Anna Zaks]

Hi Gabor,

Jordan has pointed out that a similar discussion has occurred when the 
getCheckName method has been added to the diagnostic. There are several 
write-ups from Ted and Jordan about issue tracking there as well. (See the 
Future directions for the analyzer thread on cfe-dev as well as the review 
comments for the patch that added the getCheckName method. It's about a year 
old.)

Note that we do not guarantee that CheckName will never change. (At some point, 
we will want to clean up our naming of the checks.) We also do not guarantee 
that the CheckName will be the same as the checker name (these should be the 
names of bug types, not of the implementation module).  On the other hand, we 
do not expect to continuously change these.

Since the getCheckName is already part of the diagnostic, it's fine to add it 
to the plist output. The name of the field does need to change to check_name. 
(This highlights that these are not guaranteed to be checker names.)

What do you think?

If you are ok with this, could you rename the field and I'll commit the patch.

Thanks!
Anna.


http://reviews.llvm.org/D6841

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Include checker name in Static Analyzer PLIST output

[Anna Zaks]

It seems wrong to me to associate the bug descriptions with checker names. I 
think BugType is more appropriate. Also, I don't think the checker name should 
be part of identifying the issue.

Said that, we do not have a very good design for issue tracking and we do not 
guarantee locking either category, bug type, the message, or the checker name. 
I'll talk to Jordan and Ted to see what they think about this.

Looks like our issue identification is only referring to the issue location 
(see utils/analyzer/CmpRuns.py).


http://reviews.llvm.org/D6841

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Review request][analyzer] Duplicate '0 size allocation' check from unix.API in unix.Malloc.

[Anna Zaks]

Anton, Can you add the quote from the standard somewhere in the comments 
explaining the zero allocation warning?



Comment at: test/Analysis/malloc-annotations.c:51
@@ -47,1 +50,3 @@
+  int *p = malloc(12);
+  realloc(p,0); // no-warning
 }

ayartsev wrote:
 Quuxplusone wrote:
  FWIW, this maybe should give the same warning as `realloc(p,1);` on a line 
  by itself: namely, because the returned pointer is never freed, this code 
  has a memory leak on implementations where `realloc(p,0)` returns a 
  non-null pointer.
  
  If the returned pointer is captured (`q = realloc(p,0);`) and later freed 
  (`free(q);`), there is no bug. It's not unsafe to use the return value of 
  `realloc`; it's perfectly safe and well-defined. The only 
  implementation-defined aspect of `realloc` is whether the returned pointer 
  is null or not. Either way, *using* the returned pointer is fine — it's not 
  like using a freed pointer, which is indeed undefined behavior.
 //FWIW, this maybe should give the same warning as realloc(p,1); on a line by 
 itself: namely, because the returned pointer is never freed, this code has a 
 memory leak on implementations where realloc(p,0) returns a non-null 
 pointer.//
 Agree.
 
 //It's not unsafe to use the return value of realloc; it's perfectly safe and 
 well-defined. *using* the returned pointer is fine — it's not like using a 
 freed pointer, which is indeed undefined behavior.//
 It's not a safety check, just a warning that there may be an error in the 
 code. The usage of a pointer returned from realloc(p,0) is suspicious, any 
 bytes in the new object have indeterminate values also modifying the contents 
 the returned memory is an error.
 Zero size may be requested by reason of an error when the second parameter to 
 realloc() is a variable.
I don't think we should be warning when free is called on the returned value 
from realloc(p,0). We should try and stay away from reporting issues that would 
lead to known false positives. 

We could whitelist various ways the pointer could be dereferenced and warn on 
those or just not warn on realloc(p,0). (I think passing '0' to other 
allocation functions would not likely trigger false positives.)

Anton, what do you think?

http://reviews.llvm.org/D6178

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Include checker name in Static Analyzer PLIST output

[Anna Zaks]

I don't believe the checker name should be used for bug identification. The 
checker names are implementation detail. The bug message/name and category are 
better for this. If we think that we might be changing the names of categories, 
we might come up with some kind of a stable ID.

For example, this patch, which is currently in review 
(http://reviews.llvm.org/D6178), will move the implementation of a set of 
warnings from one place/checker to another.

I'd suggest to only use the bug message and it's location. (We should already 
do that in  the CmpRuns script.)

What do you think?


http://reviews.llvm.org/D6841

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Include checker name in Static Analyzer PLIST output

[Anna Zaks]

Could you provide more information on how these would be used? The idea is that 
the category and type should provide sufficient information to categorize 
the bugs.


http://reviews.llvm.org/D6841

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Review request][analyzer] Duplicate '0 size allocation' check from unix.API in unix.Malloc.

[Anna Zaks]

Hi Anton,

Thanks for the updated patch!

Yes, but only with 'suppress-inlined-defensive-checks=false' given to 
analyzer.
Have you added a test to make sure that is the case? (If yes, what's the 
function name. I could not easily find it.) It would be similar to the 
path-sensitive check you've added but the check against '0' would be done in an 
inlined function.

Looks like you've removed the checks from the unix checker along with the 
tests. Can you add the tests back? I suggest adding your checker to the test 
file that contains the existing tests and check that it produces the same exact 
output. (If there are differences, I'd like to see what they are.) We can later 
remove the redundant tests as a separate commit.

/*
// TODO: Will be uncommented when 'realloc' is handled properly (see 
http://reviews.llvm.org/D6178 for 
// details).
void testReallocZero(char *ptr) {

  char *foo = realloc(ptr, 0); // TODO:expected-warning{{Call to 'realloc()' 
has an allocation size of 0 bytes}}
  free(foo);

}
*/

1. We should not use /* for comments.
2. Why should we warn in this case? It is possible that someone is trying to 
free the memory by calling realloc(ptr, 0).. Or are you saying that if 
literal '0' is passed we should warn? This could potentially introduce false 
positives..


http://reviews.llvm.org/D6178

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r224398 - [CallGraph] Make sure the edges are not missed due to re-declarations

[Anna Zaks]

Author: zaks
Date: Tue Dec 16 18:34:07 2014
New Revision: 224398

URL: http://llvm.org/viewvc/llvm-project?rev=224398view=rev
Log:
[CallGraph] Make sure the edges are not missed due to re-declarations

A patch by Daniel DeFreez!

We were previously dropping edges on re-declarations. Store the
canonical declarations in the graph to ensure that different
references to the same function end up reflected with the same call graph
node.

(Note, this might lead to performance fluctuation because call graph
is used to determine the function analysis order.)

Modified:
cfe/trunk/lib/Analysis/CallGraph.cpp
cfe/trunk/test/Analysis/debug-CallGraph.c

Modified: cfe/trunk/lib/Analysis/CallGraph.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CallGraph.cpp?rev=224398r1=224397r2=224398view=diff
==
--- cfe/trunk/lib/Analysis/CallGraph.cpp (original)
+++ cfe/trunk/lib/Analysis/CallGraph.cpp Tue Dec 16 18:34:07 2014
@@ -110,14 +110,13 @@ CallGraph::~CallGraph() {
 
 bool CallGraph::includeInGraph(const Decl *D) {
   assert(D);
-  if (!D-getBody())
+  if (!D-hasBody())
 return false;
 
   if (const FunctionDecl *FD = dyn_castFunctionDecl(D)) {
 // We skip function template definitions, as their semantics is
 // only determined when they are instantiated.
-if (!FD-isThisDeclarationADefinition() ||
-FD-isDependentContext())
+if (FD-isDependentContext())
   return false;
 
 IdentifierInfo *II = FD-getIdentifier();
@@ -125,11 +124,6 @@ bool CallGraph::includeInGraph(const Dec
   return false;
   }
 
-  if (const ObjCMethodDecl *ID = dyn_castObjCMethodDecl(D)) {
-if (!ID-isThisDeclarationADefinition())
-  return false;
-  }
-
   return true;
 }
 
@@ -152,6 +146,9 @@ CallGraphNode *CallGraph::getNode(const
 }
 
 CallGraphNode *CallGraph::getOrInsertNode(Decl *F) {
+  if (F  !isaObjCMethodDecl(F))
+F = F-getCanonicalDecl();
+
   CallGraphNode *Node = FunctionMap[F];
   if (Node)
 return Node;

Modified: cfe/trunk/test/Analysis/debug-CallGraph.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/debug-CallGraph.c?rev=224398r1=224397r2=224398view=diff
==
--- cfe/trunk/test/Analysis/debug-CallGraph.c (original)
+++ cfe/trunk/test/Analysis/debug-CallGraph.c Tue Dec 16 18:34:07 2014
@@ -23,11 +23,22 @@ void bbb(int y) {
   foo(x, y);
   }();
 }
+void ccc();
+void ddd() { ccc(); }
+void ccc() {}
+
+void eee();
+void eee() {}
+void fff() { eee(); }
 
 // CHECK:--- Call graph Dump ---
-// CHECK: Function:  root  calls: mmm foo aaa   bbb
-// CHECK: Function: bbb calls:  
-// CHECK: Function:   calls: foo
-// CHECK: Function: aaa calls: foo
-// CHECK: Function: foo calls: mmm
-// CHECK: Function: mmm calls:
+// CHECK-NEXT: {{Function:  root  calls: mmm foo aaa   bbb ccc ddd eee fff 
$}}
+// CHECK-NEXT: {{Function: fff calls: eee $}}
+// CHECK-NEXT: {{Function: eee calls: $}}
+// CHECK-NEXT: {{Function: ddd calls: ccc $}}
+// CHECK-NEXT: {{Function: ccc calls: $}}
+// CHECK-NEXT: {{Function: bbb calls:   $}}
+// CHECK-NEXT: {{Function:   calls: foo $}}
+// CHECK-NEXT: {{Function: aaa calls: foo $}}
+// CHECK-NEXT: {{Function: foo calls: mmm $}}
+// CHECK-NEXT: {{Function: mmm calls: $}}


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Proposal] [Analyzer] Individual options for checkers

[Anna Zaks]

Thanks Alexei! I've replied.

http://reviews.llvm.org/D3967



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Proposal] [Analyzer] Individual options for checkers

[Anna Zaks]

Hi Aleksei,

We have discussed the patch and summarized the comments (from Jordan, Ted, and 
me) on this thread: [cfe-dev] Static Analyzer Checker Options Proposal. This 
patch is very close, but does not address some of the points raised in the 
proposal. Would you be interested in discussing the proposal further?

http://reviews.llvm.org/D3967



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Path-sensitive different.IntegerOverflow checker

[Anna Zaks]

I suspect, this code below explains why you are getting the false positives.

The issue you highlight in the example is that sometimes the analyzer doesn't 
know what the value of a variable is. The existing checkers minimize false 
positives by issuing a warning only when it's known that a value is bad. For 
example, we would only warn if StateOverflow  !StateNotOverflow. This will 
flag much less issues, but should not produce a lot of false positives.

Are the false positives you are getting being flagged by the first if clause? 

 if (StateOverflow  StateNotOverflow) {
if (Pack.LValueIsTainted) {
  Msg.assign(Possible integer overflow while  + Pack.Operation +
 . Left operand is tainted:  + Pack.LValue +  AND  +
 Pack.RValue);
  reportBug(Msg, C, SL);
} else if (Pack.RValueIsTainted) {
  Msg.assign(Possible integer overflow while  + Pack.Operation +
 . Right operand is tainted:  + Pack.LValue +  AND  +
 Pack.RValue);
  reportBug(Msg, C, SL);
}
return;
  }

  if (StateOverflow) {
Msg.assign(Integer overflow while  + Pack.Operation + .  + Pack.LValue +
AND  + Pack.RValue);
reportBug(Msg, C, SL);
  }


Comment at: lib/StaticAnalyzer/Checkers/IntegerOverflowChecker.cpp:35
@@ +34,3 @@
+  mutable std::unique_ptrBuiltinBug BT;
+
+  mutable std::setSourceLocation OverflowLoc;

j.trofimovich wrote:
 zaks.anna wrote:
  Are you getting multiple reports on the same location? I don't think that 
  should be happening - the bug reporting infrastructure should unique 
  reports.
 In what way should bug reporting infrastructure unique reports? scan-build 
 prevents existence of fully identical reports by computing digest 
 (Digest::MD5-new-addfile(*FILE)-hexdigest; scan-build, line 247) but cases 
 when alerts differs by message only aren't caught.
Identical issues should have the same message. Do you have identical issues 
with different messages?

http://reviews.llvm.org/D4066



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Review request][analyzer] Duplicate '0 size allocation' check from unix.API in unix.Malloc.

[Anna Zaks]

Hi Anton,

Looks like the right direction. See a few comments throughout the code.

How do you plan on staging this? The current patch cannot be committed as is 
for the following 2 reasons:
 - Some warnings will be duplicated since we warn here and in the unix.api 
check. 
 - We should not warn on realloc(p,0).

The proposed treatment of realloc seems fine to me. Maybe you could just start 
with skipping the warning on realloc to stage the commits. What do you think?


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:222
@@ -220,4 +221,3 @@
   mutable std::unique_ptrBugType BT_OffsetFree[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc,
- *II_valloc, *II_reallocf, *II_strndup, *II_strdup,
- *II_kmalloc, *II_if_nameindex, *II_if_freenameindex;
+  mutable std::unique_ptrBugType BT_MallocZero[CK_NumCheckKinds];
+  mutable IdentifierInfo *II_alloca, *II_alloca_builtin, *II_malloc, *II_free,

Ho about BT_MallocZero - BT_ZerroAllocation ?


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:259
@@ +258,3 @@
+
+  ProgramStateRef BasicAllocationCheck(CheckerContext C, const CallExpr *CE,
+   const unsigned NumArgs, 

Please, add oxygen comment for this and CheckCallocZero.


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:261
@@ +260,3 @@
+   const unsigned NumArgs, 
+   const unsigned SizeArg,
+   ProgramStateRef State) const;

I'd rename SizeArg to make it more clear it's the allocation size; for 
example, something like AllocationSizeArg.


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:817
@@ -792,1 +816,3 @@
 
+//===--===//
+// calloc, malloc, realloc, reallocf, alloca and valloc

This comment is probably copied from the unix.api checker, where it tells which 
APIs are being processed below. It feels out of place here.


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:823
@@ +822,3 @@
+// Returns true if we try to do a zero byte allocation, false otherwise.
+// Fills in TrueState and TalseState.
+static bool IsZeroByteAllocation(ProgramStateRef State,

TalseState - FalseState

Are the True and False states backwards? I would expect TrueState to mean zero 
allocation; but by looking at the return it seems that it's the opposite..


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:834
@@ +833,3 @@
+
+// Does a basic check for 0-sized allocations suitable for most of the below
+// functions (modulo calloc)

Does - Performs

most of the bellow? where below?

Missing .


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:845
@@ +844,3 @@
+  // Sanity check for the correct number of arguments
+  if (CE-getNumArgs() != NumArgs)
+return nullptr;

Does it make sense to pass an extra argument just for sanity check?


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:866
@@ +865,3 @@
+
+ProgramStateRef MallocChecker::CheckCallocZero(CheckerContext C,
+   const CallExpr *CE,

Why is CheckCallocZero different from BasicAllocationCheck? (I feel like I am 
missing something..)


Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1548
@@ +1547,3 @@
+void MallocChecker::ReportZeroByteAllocation(CheckerContext C,
+ ProgramStateRef FalseState,
+ const Expr *Arg,

Please, use more descriptive state name.


Comment at: test/Analysis/malloc.c:957
@@ -951,1 +956,3 @@
 // 
+// Test zero-sized allocations.
+void testMallocZero() {

Could you add a couple of more interesting zero allocation tests (where we are 
not dealing with zero constant).

Theoretically, the inlined defensive checks could be a problem here, right?
(This is where the value is assumed to be zero by an inlined function, but 
known not be be zero in a caller.)

http://reviews.llvm.org/D6178



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

r220976 - [analyzer] Rename NewDeleteLeaks checker in the test script.

[Anna Zaks]

Author: zaks
Date: Fri Oct 31 12:40:14 2014
New Revision: 220976

URL: http://llvm.org/viewvc/llvm-project?rev=220976view=rev
Log:
[analyzer] Rename NewDeleteLeaks checker in the test script.

Fixup to r220289.

Modified:
cfe/trunk/utils/analyzer/SATestBuild.py

Modified: cfe/trunk/utils/analyzer/SATestBuild.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/analyzer/SATestBuild.py?rev=220976r1=220975r2=220976view=diff
==
--- cfe/trunk/utils/analyzer/SATestBuild.py (original)
+++ cfe/trunk/utils/analyzer/SATestBuild.py Fri Oct 31 12:40:14 2014
@@ -170,7 +170,7 @@ SBOutputDirReferencePrefix = Ref
 # The list of checkers used during analyzes.
 # Currently, consists of all the non-experimental checkers, plus a few alpha
 # checkers we don't want to regress on.
-Checkers=alpha.unix.SimpleStream,alpha.security.taint,alpha.cplusplus.NewDeleteLeaks,core,cplusplus,deadcode,security,unix,osx
+Checkers=alpha.unix.SimpleStream,alpha.security.taint,cplusplus.NewDeleteLeaks,core,cplusplus,deadcode,security,unix,osx
 
 Verbose = 1
 


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH][analyzer] Pass original command line arguments to compilers.

[Anna Zaks]

Let’s use tt instead of i for error messages.
Also, the quote character after % should use standard encoding.

Anna.
 On Oct 31, 2014, at 2:26 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 scan-build.htmlv_03.patch


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH][analyzer] Pass original command line arguments to compilers.

[Anna Zaks]

Yes. Thank you!
Anna.
 On Oct 31, 2014, at 1:58 PM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 Corrected. OK to commit?
 Let’s use tt instead of i for error messages.
 Also, the quote character after % should use standard encoding.
 
 Anna.
 On Oct 31, 2014, at 2:26 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 scan-build.htmlv_03.patch
 
 
 -- 
 Anton
 
 scan-build.htmlv_04.patch


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH][analyzer] Pass original command line arguments to compilers.

[Anna Zaks]

Anton,

I think the bulleted list might benefit from restructuring a bit to make it 
sound more like a recommendation on what needs to be done under different 
circumstances.

My understanding is that the most reliable way is to use MinGW instead of make. 
(This is addressed by the first bullet.)

However, some projects might get away with using make. In that case, the other 
recommendations (2d and 3d bullet) would apply.

Is this correct?

Thanks,
Anna.

 On Oct 20, 2014, at 1:49 PM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 Updated the For Windows Users section with helpful hints, OK to commit?
 
 Anton, 
 
 Thanks for the investigation.
 
 Please, send the proposed wording as a patch. (Not sure if it would be 
 possible to describe the symptoms of the problem.)
 
 Anna.
 On Oct 18, 2014, at 1:56 AM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 As I was explained in the MSYS community the MSYS utils are dependent on 
 the MSYS runtime and their usage from cmd.exe is unsupported. You are 
 welcome to try it, but if you observe odd behaviour, such as here, then you 
 are out of luck.
 
 I performed several tests and found out that proper processing is performed 
 with either running scan-build with MSYS make in the following way:
 scan-build ... sh -c make
 or with using mingw32-make and removal of MSYS from PATH (otherwise 
 mingw32-make tries to use MSYS utils).
 
 from the MinGW FAQ:
 What's the difference between make and mingw32-make?
 The native (i.e.: MSVCRT dependent) port of make is lacking in some 
 functionality and has modified functionality due to the lack of POSIX on 
 Win32. There also exists a version of make in the MSYS distribution that is 
 dependent on the MSYS runtime. This port operates more as make was intended 
 to operate and gives less headaches during execution. Based on this, the 
 MinGW developers/maintainers/packagers decided it would be best to rename 
 the native version so that both the native version and the MSYS version 
 could be present at the same time without file name collision.
 
 Is it OK to add the recommendations to the scan-build: running the analyzer 
 from the command line 
 http://clang-analyzer.llvm.org/scan-build.html#scanbuild_forwindowsusers, 
 For Windows Users section?
 
 Sorry, that's not a solution. 
 
 The goal of the patch is to pass unmodified arguments to compilers as 
 they were written in the makefile. Arguments taken from @ARGV may be 
 modified by the system and Perl, at least quotes and backslash sequences 
 are processed. Using this arguments may cause compiler errors. Sometimes 
 system+Perl corrupt arguments completely, for example, using perl from 
 MSYS 1.0 on Windows I got: 
 Line from makefile: 
$(CXX) -DMACRO=\string\ file.cpp asd dff ghh -o file.exe 
 
 arguments red from @ARGV by c++-analyzer: 
   -DMACRO=\string\ file.cpp -o file.exe 
 
 Please review! 
 
 
 -- 
 Anton
 
 -- 
 Anton
 scan-build.html.patch

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH][analyzer] Pass original command line arguments to compilers.

[Anna Zaks]

Hi Anton,

Here are some suggestions.

-All you need to be able to invoke scan-build from an arbitrary location is to 
add the path to scan-build to your PATH environment variable.
+To invoke scan-build from an arbitrary location, add the path to the folder 
containing scan-build.bat to your PATH environment variable.

New section uses a lot of bold, which makes it stand out from the rest of the 
page. How about something like this:

If getting unexpected fatal error: no input files while building with MSYS 
make from the Windows cmd, try one of the these solutions:
Use MinGW mingw32-make instead of MSYS make and exclude the path to MSYS from 
PATH to prevent mingw32-make from using MSYS utils. MSYS utils are dependent on 
the MSYS runtime and they are not intended for being run from the Windows cmd. 
Specifically, makefile commands with backslashed quotes may be heavily 
corrupted when passed for execution. 
Run make from the sh shell:

 $scan-build [options] sh -c make [options]”  // Use the 
proper formatting for this. like other invocations of scan build.

If getting Error : *** target pattern contains no `%’” while using GNU Make 
3.81, try to use another version of make.

Please, don’t use tt instead of italics for the error messages.

I’ve left out some links and explanations of why the errors happen. I think 
these are not essential to the user and we want to keep the section brief since 
the page contains simple scan-build use instructions.

Thanks!
Anna.

On Oct 30, 2014, at 11:03 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 Attached is an updated patch.
 
 Anton,
 
 I think the bulleted list might benefit from restructuring a bit to make it 
 sound more like a recommendation on what needs to be done under different 
 circumstances.
 Restructured the bulleted list, now it looks much more better.
 
 
 My understanding is that the most reliable way is to use MinGW instead of 
 make. (This is addressed by the first bullet.)
 
 However, some projects might get away with using make. In that case, the 
 other recommendations (2d and 3d bullet) would apply.
 There are two reliable ways - either use pure MinGW or run make from the sh 
 shell. The 2d bullet is really a recipe for healing a problem from the 1st 
 bullet, eliminated it.
 Pleas look at the updated list.
 
 Thank you for looking at this!
 
 Is this correct?
 
 Thanks,
 Anna.
 
 On Oct 20, 2014, at 1:49 PM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 Updated the For Windows Users section with helpful hints, OK to commit?
 
 Anton, 
 
 Thanks for the investigation.
 
 Please, send the proposed wording as a patch. (Not sure if it would be 
 possible to describe the symptoms of the problem.)
 
 Anna.
 On Oct 18, 2014, at 1:56 AM, Anton Yartsev anton.yart...@gmail.com 
 mailto:anton.yart...@gmail.com wrote:
 
 As I was explained in the MSYS community the MSYS utils are dependent on 
 the MSYS runtime and their usage from cmd.exe is unsupported. You are 
 welcome to try it, but if you observe odd behaviour, such as here, then 
 you are out of luck.
 
 I performed several tests and found out that proper processing is 
 performed with either running scan-build with MSYS make in the following 
 way:
 scan-build ... sh -c make
 or with using mingw32-make and removal of MSYS from PATH (otherwise 
 mingw32-make tries to use MSYS utils).
 
 from the MinGW FAQ:
 What's the difference between make and mingw32-make?
 The native (i.e.: MSVCRT dependent) port of make is lacking in some 
 functionality and has modified functionality due to the lack of POSIX on 
 Win32. There also exists a version of make in the MSYS distribution that 
 is dependent on the MSYS runtime. This port operates more as make was 
 intended to operate and gives less headaches during execution. Based on 
 this, the MinGW developers/maintainers/packagers decided it would be best 
 to rename the native version so that both the native version and the 
 MSYS version could be present at the same time without file name 
 collision.
 
 Is it OK to add the recommendations to the scan-build: running the 
 analyzer from the command line 
 http://clang-analyzer.llvm.org/scan-build.html#scanbuild_forwindowsusers,
  For Windows Users section?
 
 Sorry, that's not a solution. 
 
 The goal of the patch is to pass unmodified arguments to compilers as 
 they were written in the makefile. Arguments taken from @ARGV may be 
 modified by the system and Perl, at least quotes and backslash 
 sequences are processed. Using this arguments may cause compiler 
 errors. Sometimes system+Perl corrupt arguments completely, for 
 example, using perl from MSYS 1.0 on Windows I got: 
 Line from makefile: 
$(CXX) -DMACRO=\string\ file.cpp asd dff ghh -o file.exe 
 
 arguments red from @ARGV by c++-analyzer: 
   -DMACRO=\string\ file.cpp -o file.exe 
 
 Please review! 
 
 
 -- 
 Anton
 
 -- 
 Anton
 scan-build.html.patch
 
 -- 
 Anton
 

Re: [PATCH][analyzer] Pass original command line arguments to compilers.

[Anna Zaks]

Anton, 

Thanks for the investigation.

Please, send the proposed wording as a patch. (Not sure if it would be possible 
to describe the symptoms of the problem.)

Anna.
 On Oct 18, 2014, at 1:56 AM, Anton Yartsev anton.yart...@gmail.com wrote:
 
 As I was explained in the MSYS community the MSYS utils are dependent on the 
 MSYS runtime and their usage from cmd.exe is unsupported. You are welcome to 
 try it, but if you observe odd behaviour, such as here, then you are out of 
 luck.
 
 I performed several tests and found out that proper processing is performed 
 with either running scan-build with MSYS make in the following way:
 scan-build ... sh -c make
 or with using mingw32-make and removal of MSYS from PATH (otherwise 
 mingw32-make tries to use MSYS utils).
 
 from the MinGW FAQ:
 What's the difference between make and mingw32-make?
 The native (i.e.: MSVCRT dependent) port of make is lacking in some 
 functionality and has modified functionality due to the lack of POSIX on 
 Win32. There also exists a version of make in the MSYS distribution that is 
 dependent on the MSYS runtime. This port operates more as make was intended 
 to operate and gives less headaches during execution. Based on this, the 
 MinGW developers/maintainers/packagers decided it would be best to rename the 
 native version so that both the native version and the MSYS version could 
 be present at the same time without file name collision.
 
 Is it OK to add the recommendations to the scan-build: running the analyzer 
 from the command line 
 http://clang-analyzer.llvm.org/scan-build.html#scanbuild_forwindowsusers, 
 For Windows Users section?
 
 Sorry, that's not a solution. 
 
 The goal of the patch is to pass unmodified arguments to compilers as they 
 were written in the makefile. Arguments taken from @ARGV may be modified by 
 the system and Perl, at least quotes and backslash sequences are processed. 
 Using this arguments may cause compiler errors. Sometimes system+Perl 
 corrupt arguments completely, for example, using perl from MSYS 1.0 on 
 Windows I got: 
 Line from makefile: 
$(CXX) -DMACRO=\string\ file.cpp asd dff ghh -o file.exe 
 
 arguments red from @ARGV by c++-analyzer: 
   -DMACRO=\string\ file.cpp -o file.exe 
 
 Please review! 
 
 
 -- 
 Anton

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] move NewDeleteLeaks checker from alpha.cplusplus to cplusplus group.

[Anna Zaks]

Anton, 

Thank you for running the extra tests! Please, go ahead and turn on the checker 
when you are ready.

Thank you,
Anna.

http://reviews.llvm.org/D5313



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Anna Zaks]

Artyom,

Can you please measure the performance impact of your patch and provide the 
timing information along with the patch? Testing with the preprocessed files 
we've sent you is a must.

Below are the results comparing the baseline with the revert of r218296 + the 
other 2 patches you sent to be applied on top of it. I see an 84% slowdown with 
the patches on gram.c, see below.

Anna.


--- baseline ---

Zakss-Mac-Pro:build-clang zaks$ time ./bin/clang --analyze ~/tmp/gram.c 
gram.c:18574:200: warning: Value stored to 'yyptr' is never read
  ...* sizeof (*yyls) + (sizeof (union yyalloc) - 1); yyptr += yynewbytes / 
sizeof (*yyptr); } while (0);
  ^

gram.y:11400:18: warning: Assigned value is garbage or undefined
 c-location = yylsp[-4];
 ^ ~
gram.y:11417:18: warning: Assigned value is garbage or undefined
 w-location = yylsp[-3];
 ^ ~
gram.y:11715:18: warning: Assigned value is garbage or undefined
 t-location = yylsp[-4];
 ^ ~
gram.y:11734:287: warning: Function call argument is an uninitialized value
  ... 24))), errmsg(interval precision specified twice), 
scanner_errposition(yylsp[-5], yyscanner))) : (voi...
 
^
gram.y:11738:43: warning: Function call argument is an uninitialized value
  t-typmods = lappend(yyvsp[0].list, makeIntConst(yyvsp[-3].ival, 
yylsp[-3]));
  
^~~
gram.y:11741:60: warning: Function call argument is an uninitialized value
  t-typmods = lcons(makeIntConst((0x7FFF), -1), 
lcons(makeIntConst(yyvsp[-3].ival, yylsp[-3]), ((Lis...
   
^~~
7 warnings generated.

real0m17.958s
user0m17.020s
sys 0m0.302s


--- latest patches ---

Zakss-Mac-Pro:build-clang zaks$ time ./bin/clang --analyze ~/tmp/gram.c 
gram.c:18574:200: warning: Value stored to 'yyptr' is never read
  ...* sizeof (*yyls) + (sizeof (union yyalloc) - 1); yyptr += yynewbytes / 
sizeof (*yyptr); } while (0);
  ^

gram.y:11400:18: warning: Assigned value is garbage or undefined
 c-location = yylsp[-4];
 ^ ~
gram.y:11417:18: warning: Assigned value is garbage or undefined
 w-location = yylsp[-3];
 ^ ~
gram.y:11715:18: warning: Assigned value is garbage or undefined
 t-location = yylsp[-4];
 ^ ~
gram.y:11734:287: warning: Function call argument is an uninitialized value
  ... 24))), errmsg(interval precision specified twice), 
scanner_errposition(yylsp[-5], yyscanner))) : (voi...
 
^
gram.y:11738:43: warning: Function call argument is an uninitialized value
  t-typmods = lappend(yyvsp[0].list, makeIntConst(yyvsp[-3].ival, 
yylsp[-3]));
  
^~~
gram.y:11741:60: warning: Function call argument is an uninitialized value
  t-typmods = lcons(makeIntConst((0x7FFF), -1), 
lcons(makeIntConst(yyvsp[-3].ival, yylsp[-3]), ((Lis...
   
^~~
7 warnings generated.

real1m45.005s
user1m43.871s
sys 0m0.418s

 On Oct 6, 2014, at 2:27 AM, Artyom Skrobov artyom.skro...@arm.com wrote:
 
 Anna, sorry, now I see what you mean (I needed to pass --fblocks on the 
 command line).
  
 The following simple patch fixes the performance in this case, as well as 
 brings the implementation in line with the comment just above it (dequeue 
 rather than unstack).
  
 Index: lib/Analysis/DataflowWorklist.cpp
 ===
 --- lib/Analysis/DataflowWorklist.cpp (revision 218295)
 +++ lib/Analysis/DataflowWorklist.cpp (working copy)
 @@ -58,8 +101,10 @@
// First dequeue from the worklist.  This can represent
// updates along backedges that we want propagated as quickly as possible.
 -  if (!worklist.empty())
 -B = worklist.pop_back_val();
 +  if (!worklist.empty()) {
 + B = worklist.front();
 + worklist.erase(worklist.begin());
 +  }
// Next dequeue from the initial graph traversal (either post order or
// reverse post order).  This is the theoretical ideal in the presence
  
 The two patches are independent one from the other.
  
  
 From: Anna Zaks [mailto:ga...@apple.com] 
 Sent: 03 October 2014 19:56
 To: Artyom Skrobov
 Cc: Alexander Kornienko; Ted Kremenek; cfe-commits@cs.uiuc.edu
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, 
 to recover

Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Anna Zaks]

The analyzer is still timing out for me on postgresql.

I am going to send a preprocessed file off line.

Maybe you will find some answers by looking at other performance related 
patches for LiveVariables that have been committed over years. 

Anna.
 On Oct 2, 2014, at 4:45 AM, Artyom Skrobov artyom.skro...@arm.com wrote:
 
 Hello,
  
 I see now that the CFG iteration orders, so painstakingly defined during the 
 previous round of reviews in Aug, didn’t entirely match the pre-r214064 
 behaviour.
  
 Attached is a patch that simplifies PostOrderCFGView significantly, making 
 the two possible iteration orders really obvious.
  
 I have tested it with both Alexander’s testcases (analyzer-regression1.c from 
 Sep, and options.c from Aug), and it shows good performance on both.
  
 OK to un-revert r218296, and to commit this patch on top of that?
  
  
 From: Alexander Kornienko [mailto:ale...@google.com] 
 Sent: 21 September 2014 01:16
 To: Artyom Skrobov
 Cc: Anna Zaks; cfe-commits@cs.uiuc.edu Commits
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, 
 to recover the performance after r214064
  
 I'm attaching one of the files that regressed after r214064 and never got 
 fixed (preprocessed ceval.c from Python 3.3).
  
 On Sat, Sep 20, 2014 at 5:38 PM, Alexander Kornienko ale...@google.com 
 mailto:ale...@google.com wrote:
 I actually still think, that I have some code that started taking large time 
 to be analyzed after r214064 and didn't recover after r215650. But I didn't 
 get to creating a reasonable repro for you. And the number of files left 
 affected after r215650 is so small, that I didn't prioritize this high 
 enough. I'll still try to provide a repro soon.
 
 On 20 Sep 2014 17:10, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 Anna, do you mean the performance had been acceptable after r214064, but 
 degraded after r215650, which fixed the performance regression introduced in 
 r214064?
  
 Do you have any specific example of code that takes longer to compile after 
 r215650?
  
 Not hearing back from Alexander since August, I assumed the performance 
 regression he observed after r215650 was not in fact related to that commit.
  
  
 From: Anna Zaks [mailto:ga...@apple.com mailto:ga...@apple.com] 
 Sent: 20 September 2014 01:19
 To: Artyom Skrobov
 Cc: cfe-commits@cs.uiuc.edu mailto:cfe-commits@cs.uiuc.edu Commits; Ted 
 Kremenek; Jordan Rose; Alexander Kornienko
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, 
 to recover the performance after r214064
  
 Hi Artyom,
  
 Unfortunately, this commit (r215650) causes major performance regressions on 
 our buildbots. In particular, building postgresql-9.1 times out.
  
 Please, revert as soon as possible.
  
 Thank you,
 Anna.
 On Aug 20, 2014, at 3:13 AM, Alexander Kornienko ale...@google.com 
 mailto:ale...@google.com wrote:
  
 On Fri, Aug 15, 2014 at 10:38 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 Many thanks -- committed as r215650
 
 Alexander, can you confirm that the analyzer performance is now acceptable
 for your use cases?
  
 Artyom, sorry for the long delay. These files now work fine, but I still see 
 up to 8-10 hours analysis time on a couple of other files. I'm sure I didn't 
 see this before your first patch, but I can't yet tell in which revision it 
 was introduced. I'll post more details and a repro later today.
  
 
 
 -Original Message-
 From: Ted kremenek [mailto:kreme...@apple.com mailto:kreme...@apple.com]
 Sent: 14 August 2014 16:36
 To: Artyom Skrobov
 Cc: Alexander Kornienko; cfe-commits@cs.uiuc.edu 
 mailto:cfe-commits@cs.uiuc.edu
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables
 analysis, to recover the performance after r214064
 
 Looks great to me.
 
  On Aug 14, 2014, at 3:08 AM, Artyom Skrobov artyom.skro...@arm.com 
  mailto:artyom.skro...@arm.com
 wrote:
 
  Thank you Ted!
 
  Attaching the updated patch for a final review.
 
  Summary of changes:
 
  * Comments updated to reflect the two possible CFG traversal orders
  * PostOrderCFGView::po_iterator taken out of the header file
  * Iteration order for PostOrderCFGView changed to reverse inverse
  post-order, the one required for a backward analysis
  * ReversePostOrderCFGView created, with the same iteration order that
  PostOrderCFGView used to have, the one required for a forward analysis
  * The two previous consumers of PostOrderCFGView, ThreadSafetyCommon.h and
  Consumed.cpp, switched to use ReversePostOrderCFGView
  * DataflowWorklistBase renamed to DataflowWorklist, and the two
  specializations named BackwardDataflowWorklist and ForwardDataflowWorklist
 
  I believe this naming scheme matches the accepted terminology best.
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu mailto:cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman

r219026 - [analyzer] Refactor and cleanup IsCompleteType

[Anna Zaks]

Author: zaks
Date: Fri Oct  3 16:49:03 2014
New Revision: 219026

URL: http://llvm.org/viewvc/llvm-project?rev=219026view=rev
Log:
[analyzer] Refactor and cleanup IsCompleteType
There are three copies of IsCompleteType(...) functions in CSA and all
of them are incomplete (I experienced  crashes in some CSA's test cases).
I have replaced these function calls with Type::isIncompleteType() calls.

A patch by Aleksei Sidorin!

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp?rev=219026r1=219025r2=219026view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp Fri Oct  3 
16:49:03 2014
@@ -218,17 +218,6 @@ void RegionRawOffsetV2::dumpToStream(raw
   os  raw_offset_v2{  getRegion()  ','  getByteOffset()  '}';
 }
 
-// FIXME: Merge with the implementation of the same method in Store.cpp
-static bool IsCompleteType(ASTContext Ctx, QualType Ty) {
-  if (const RecordType *RT = Ty-getAsRecordType()) {
-const RecordDecl *D = RT-getDecl();
-if (!D-getDefinition())
-  return false;
-  }
-
-  return true;
-}
-
 
 // Lazily computes a value to be used by 'computeOffset'.  If 'val'
 // is unknown or undefined, we lazily substitute '0'.  Otherwise,
@@ -288,7 +277,7 @@ RegionRawOffsetV2 RegionRawOffsetV2::com
 QualType elemType = elemReg-getElementType();
 // If the element is an incomplete type, go no further.
 ASTContext astContext = svalBuilder.getContext();
-if (!IsCompleteType(astContext, elemType))
+if (elemType-isIncompleteType())
   return RegionRawOffsetV2();
 
 // Update the offset.

Modified: cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp?rev=219026r1=219025r2=219026view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp Fri Oct  3 16:49:03 2014
@@ -1116,17 +1116,6 @@ const SymbolicRegion *MemRegion::getSymb
   return nullptr;
 }
 
-// FIXME: Merge with the implementation of the same method in Store.cpp
-static bool IsCompleteType(ASTContext Ctx, QualType Ty) {
-  if (const RecordType *RT = Ty-getAsRecordType()) {
-const RecordDecl *D = RT-getDecl();
-if (!D-getDefinition())
-  return false;
-  }
-
-  return true;
-}
-
 RegionRawOffset ElementRegion::getAsArrayOffset() const {
   CharUnits offset = CharUnits::Zero();
   const ElementRegion *ER = this;
@@ -1148,7 +1137,7 @@ RegionRawOffset ElementRegion::getAsArra
 QualType elemType = ER-getElementType();
 
 // If we are pointing to an incomplete type, go no further.
-if (!IsCompleteType(C, elemType)) {
+if (elemType-isIncompleteType()) {
   superR = ER;
   break;
 }
@@ -1288,7 +1277,7 @@ RegionOffset MemRegion::getAsOffset() co
   R = ER-getSuperRegion();
 
   QualType EleTy = ER-getValueType();
-  if (!IsCompleteType(getContext(), EleTy)) {
+  if (EleTy-isIncompleteType()) {
 // We cannot compute the offset of the base class.
 SymbolicOffsetBase = R;
 continue;

Modified: cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp?rev=219026r1=219025r2=219026view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/Store.cpp Fri Oct  3 16:49:03 2014
@@ -48,17 +48,6 @@ const MemRegion *StoreManager::MakeEleme
   return MRMgr.getElementRegion(EleTy, idx, Base, svalBuilder.getContext());
 }
 
-// FIXME: Merge with the implementation of the same method in MemRegion.cpp
-static bool IsCompleteType(ASTContext Ctx, QualType Ty) {
-  if (const RecordType *RT = Ty-getAsRecordType()) {
-const RecordDecl *D = RT-getDecl();
-if (!D-getDefinition())
-  return false;
-  }
-
-  return true;
-}
-
 StoreRef StoreManager::BindDefault(Store store, const MemRegion *R, SVal V) {
   return StoreRef(store, *this);
 }
@@ -196,7 +185,7 @@ const MemRegion *StoreManager::castRegio
   const MemRegion *newSuperR = nullptr;
 
   // We can only compute sizeof(PointeeTy) if it is a complete type.
-  if (IsCompleteType(Ctx, PointeeTy)) {
+  if (!PointeeTy-isIncompleteType()) {
 // Compute the size in **bytes**.
 CharUnits pointeeTySize = Ctx.getTypeSizeInChars(PointeeTy);
  

r219025 - [analyzer] Make Malloc Checker track memory allocated by if_nameindex

[Anna Zaks]

Author: zaks
Date: Fri Oct  3 16:48:59 2014
New Revision: 219025

URL: http://llvm.org/viewvc/llvm-project?rev=219025view=rev
Log:
[analyzer] Make Malloc Checker track memory allocated by if_nameindex

The MallocChecker does currently not track the memory allocated by
if_nameindex. That memory is dynamically allocated and should be freed
by calling if_freenameindex. The attached patch teaches the checker
about these functions.

Memory allocated by if_nameindex is treated as a separate allocation
family. That way the checker can verify it is freed by the correct
function.

A patch by Daniel Fahlgren!

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=219025r1=219024r2=219025view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Fri Oct  3 16:48:59 
2014
@@ -42,7 +42,8 @@ enum AllocationFamily {
   AF_None,
   AF_Malloc,
   AF_CXXNew,
-  AF_CXXNewArray
+  AF_CXXNewArray,
+  AF_IfNameIndex
 };
 
 class RefState {
@@ -161,7 +162,8 @@ public:
   MallocChecker()
   : II_malloc(nullptr), II_free(nullptr), II_realloc(nullptr),
 II_calloc(nullptr), II_valloc(nullptr), II_reallocf(nullptr),
-II_strndup(nullptr), II_strdup(nullptr), II_kmalloc(nullptr) {}
+II_strndup(nullptr), II_strdup(nullptr), II_kmalloc(nullptr),
+II_if_nameindex(nullptr), II_if_freenameindex(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
@@ -174,6 +176,12 @@ public:
 CK_NumCheckKinds
   };
 
+  enum class MemoryOperationKind { 
+MOK_Allocate,
+MOK_Free,
+MOK_Any
+  };
+
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckName CheckNames[CK_NumCheckKinds];
 
@@ -212,7 +220,7 @@ private:
   mutable std::unique_ptrBugType BT_OffsetFree[CK_NumCheckKinds];
   mutable IdentifierInfo *II_malloc, *II_free, *II_realloc, *II_calloc,
  *II_valloc, *II_reallocf, *II_strndup, *II_strdup,
- *II_kmalloc;
+ *II_kmalloc, *II_if_nameindex, *II_if_freenameindex;
   mutable Optionaluint64_t KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext C) const;
@@ -238,8 +246,10 @@ private:
   /// Check if this is one of the functions which can allocate/reallocate 
memory 
   /// pointed to by one of its arguments.
   bool isMemFunction(const FunctionDecl *FD, ASTContext C) const;
-  bool isFreeFunction(const FunctionDecl *FD, ASTContext C) const;
-  bool isAllocationFunction(const FunctionDecl *FD, ASTContext C) const;
+  bool isCMemFunction(const FunctionDecl *FD,
+  ASTContext C,
+  AllocationFamily Family,
+  enum MemoryOperationKind) const;
   bool isStandardNewDelete(const FunctionDecl *FD, ASTContext C) const;
   ///@}
   ProgramStateRef MallocMemReturnsAttr(CheckerContext C,
@@ -496,13 +506,15 @@ void MallocChecker::initIdentifierInfo(A
   II_strdup = Ctx.Idents.get(strdup);
   II_strndup = Ctx.Idents.get(strndup);
   II_kmalloc = Ctx.Idents.get(kmalloc);
+  II_if_nameindex = Ctx.Idents.get(if_nameindex);
+  II_if_freenameindex = Ctx.Idents.get(if_freenameindex);
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext C) const 
{
-  if (isFreeFunction(FD, C))
+  if (isCMemFunction(FD, C, AF_Malloc, MemoryOperationKind::MOK_Any))
 return true;
 
-  if (isAllocationFunction(FD, C))
+  if (isCMemFunction(FD, C, AF_IfNameIndex, MemoryOperationKind::MOK_Any))
 return true;
 
   if (isStandardNewDelete(FD, C))
@@ -511,45 +523,61 @@ bool MallocChecker::isMemFunction(const
   return false;
 }
 
-bool MallocChecker::isAllocationFunction(const FunctionDecl *FD,
- ASTContext C) const {
+bool MallocChecker::isCMemFunction(const FunctionDecl *FD,
+   ASTContext C,
+   AllocationFamily Family,
+   enum MemoryOperationKind MemKind) const {
   if (!FD)
 return false;
 
+  bool CheckFree = (MemKind == MemoryOperationKind::MOK_Any ||
+MemKind == MemoryOperationKind::MOK_Free);
+  bool CheckAlloc = (MemKind == MemoryOperationKind::MOK_Any ||
+ MemKind == MemoryOperationKind::MOK_Allocate);
+
   if (FD-getKind() == Decl::Function) {
-IdentifierInfo *FunI = FD-getIdentifier();
+const IdentifierInfo *FunI = FD-getIdentifier();
 initIdentifierInfo(C);
 
-if (FunI == II_malloc || FunI == II_realloc ||
-FunI == II_reallocf || FunI == II_calloc || FunI == II_valloc ||
-FunI == II_strdup || FunI == II_strndup || FunI == II_kmalloc)
-  

r219024 - [analyzer] Make CStringChecker correctly calculate return value of mempcpy

[Anna Zaks]

Author: zaks
Date: Fri Oct  3 16:48:54 2014
New Revision: 219024

URL: http://llvm.org/viewvc/llvm-project?rev=219024view=rev
Log:
[analyzer] Make CStringChecker correctly calculate return value of mempcpy

The return value of mempcpy is only correct when the destination type is
one byte in size. This patch casts the argument to a char* so the
calculation is also correct for structs, ints etc.

A patch by Daniel Fahlgren!

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
cfe/trunk/test/Analysis/bstring.c

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp?rev=219024r1=219023r2=219024view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp Fri Oct  3 
16:48:54 2014
@@ -969,8 +969,13 @@ void CStringChecker::evalCopyCommon(Chec
   // Get the length to copy.
   if (OptionalNonLoc lenValNonLoc = sizeVal.getAsNonLoc()) {
 // Get the byte after the last byte copied.
+SValBuilder SvalBuilder = C.getSValBuilder();
+ASTContext Ctx = SvalBuilder.getContext();
+QualType CharPtrTy = Ctx.getPointerType(Ctx.CharTy);
+loc::MemRegionVal DestRegCharVal = SvalBuilder.evalCast(destRegVal,
+  CharPtrTy, Dest-getType()).castAsloc::MemRegionVal();
 SVal lastElement = C.getSValBuilder().evalBinOpLN(state, BO_Add, 
-  destRegVal,
+  DestRegCharVal,
   *lenValNonLoc, 
   Dest-getType());
   

Modified: cfe/trunk/test/Analysis/bstring.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/bstring.c?rev=219024r1=219023r2=219024view=diff
==
--- cfe/trunk/test/Analysis/bstring.c (original)
+++ cfe/trunk/test/Analysis/bstring.c Fri Oct  3 16:48:54 2014
@@ -257,6 +257,45 @@ void mempcpy13() {
   mempcpy(a, 0, 0); // no-warning
 }
 
+void mempcpy14() {
+  int src[] = {1, 2, 3, 4};
+  int dst[5] = {0};
+  int *p;
+
+  p = mempcpy(dst, src, 4 * sizeof(int));
+
+  clang_analyzer_eval(p == dst[4]); // expected-warning{{TRUE}}
+}
+
+struct st {
+  int i;
+  int j;
+};
+
+void mempcpy15() {
+  struct st s1 = {0};
+  struct st s2;
+  struct st *p1;
+  struct st *p2;
+
+  p1 = (s2) + 1;
+  p2 = mempcpy(s2, s1, sizeof(struct st));
+
+  clang_analyzer_eval(p1 == p2); // expected-warning{{TRUE}}
+}
+
+void mempcpy16() {
+  struct st s1[10] = {{0}};
+  struct st s2[10];
+  struct st *p1;
+  struct st *p2;
+
+  p1 = (s2[0]) + 5;
+  p2 = mempcpy(s2[0], s1[0], 5 * sizeof(struct st));
+
+  clang_analyzer_eval(p1 == p2); // expected-warning{{TRUE}}
+}
+
 void mempcpy_unknown_size_warn (size_t n) {
   char a[4];
   void *result = mempcpy(a, 0, n); // expected-warning{{Null pointer argument 
in call to memory copy function}}


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] [Refactoring+bugfix] Replace copy-paste function with a correctly implemented one

[Anna Zaks]

Committed in r219026. 
Thanks Aleksei!

http://reviews.llvm.org/D4973



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] make analyzer track memory allocated by if_nameindex

[Anna Zaks]

Committed in r219025.

Thanks Daniel!
 On Oct 3, 2014, at 7:18 AM, Daniel Fahlgren dan...@fahlgren.se wrote:
 
 Hi,
 
 On Thu, 2014-10-02 at 10:26 -0700, Anna Zaks wrote:
 No I do not, but perhaps it is time that I apply for it?
 
 I think your patches still need to go through pre-commit review, so we
 can just continue applying them.
 
 No problem. I wouldn't even dream of committing patches that wasn't
 reviewed and approved :)
 
 Please, send me the updated patch (with MOK_None removed) and I'll
 commit it.
 
 Updated patch attached. Thanks for reviewing this.
 
 Cheers,
 Daniel
 ifnameindex.patch

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Make analyzer correctly calculate return value of mempcpy

[Anna Zaks]

Committed in r219024.

Thanks Daniel!
 On Oct 2, 2014, at 6:18 PM, Anna Zaks ga...@apple.com wrote:
 
 
 On Oct 2, 2014, at 2:15 AM, Daniel Fahlgren dan...@fahlgren.se wrote:
 
 On Fri, 2014-09-12 at 01:01 +0200, Daniel Fahlgren wrote:
 Hi,
 
 the return value of mempcpy is only correct when the destination type is
 one byte in size. This patch casts the argument to a char* so the
 calculation is also correct for structs, ints etc. Comments?
 
 Ping?
 
 Cheers,
 Daniel Fahlgren
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
 

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] make analyzer track memory allocated by if_nameindex

[Anna Zaks]

 On Oct 2, 2014, at 2:13 AM, Daniel Fahlgren dan...@fahlgren.se wrote:
 
 Hi,
 
 On Wed, 2014-10-01 at 18:00 -0700, Anna Zaks wrote:
 +MOK_None,
 Do we need this one?
 
 No, that is a left over after some old code structure. Sorry.
 
 +if (Family == AF_Malloc  CheckFree) {
 +if (Family == AF_Malloc  CheckAlloc) {
 
 A possible micro optimization would be to check the family once for the
 common two cases. Also, note that ChecksEnabled[CK_MallocOptimistic]
 most commonly evaluates to false.
 
 if (Family == AF_Malloc) {
if (CheckFree) {
  if ()   ...
} else {
  assert(CheckAlloc); 
  if ()   ...
}
if (!ChecksEnabled[CK_MallocOptimistic])
  return false;
 }
 
 I've tried that but the amount of nested if-statements and conditions
 looked too cluttered. Also that example is slightly wrong since in some
 cases we wish do check for both Free and Allocate functions, not just
 one kind.
 
 Do you have commit access?
 
 No I do not, but perhaps it is time that I apply for it?
 

I think your patches still need to go through pre-commit review, so we can just 
continue applying them.

Please, send me the updated patch (with MOK_None removed) and I'll commit it.

Thanks!
Anna.

 Cheers,
 Daniel

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Detect duplicate [super dealloc] calls

[Anna Zaks]

David, 

I've listed some small concerns throughout and have not looked at the tests in 
a lot of detail yet.

However, the main problem is that you store too much state in 
SuperDeallocState. (We try to keep the states as lean as possible since they 
are one of the major bottlenecks and have considerable impact on performance.) 
Most of what you store is only consumed by BugReporterVisitor. You might be 
able to work around storing this info by lazily reconstructing it at bug 
reporting time.

When bug is reported, the path is visited bottom up; this is when your visitor 
will get called. You can teach the visitor to detect the first node in which 
the SuperDeallocState for the given SymRef has been added (as the path is 
visited bottom up). At that point, you look up the statement associated with 
the ProgramPoint and if it's a message call, you found the node to which the 
diagnostic should be attached. (Take a look at other checkers such as 
MallocChecker; they are all based on this lazy approach.)

You might also want to play with Stack Hints in error reporting since your 
reports are likely to span multiple functions. These are the notes that get 
attached to the call site of the functions that contain the diagnostic. You 
should use -analyzer-output=text (and -analyzer-output=plist) when testing the 
full path experience. (Ex: See ./test/Analysis/keychainAPI-diagnostic-visitor.m)


Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:156
@@ +155,3 @@
+
+  // FIXME: A callback should disable checkers at the start of functions.
+  if (!shouldRunOnFunctionOrMethod(

Is this done for performance purposes? Can [super dealloc] be called from 
functions? isSuperDeallocMessage check seems faster.


Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:161
@@ +160,3 @@
+
+  // Check for [super dealloc] method call.
+  if (isSuperDeallocMessage(M)) {

It would be better to do an early return here as well, unless you plan to 
expand this for handling other calls.
if (!isSuperDeallocMessage(M))
  return;


Comment at: lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp:182
@@ +181,3 @@
+  SymbolRef SelfSymbol = M.getSelfSVal().getAsSymbol();
+  const SuperDeallocState *SDState = 
State-getCalledSuperDealloc(SelfSymbol);
+  if (!SDState)

These should be guarded on isSuperDeallocMessage. Otherwise, we would do a 
lookup every time we see a method call.

http://reviews.llvm.org/D5238



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Make analyzer correctly calculate return value of mempcpy

[Anna Zaks]

Looks good to me. 

Anna.
 On Oct 2, 2014, at 2:15 AM, Daniel Fahlgren dan...@fahlgren.se wrote:
 
 On Fri, 2014-09-12 at 01:01 +0200, Daniel Fahlgren wrote:
 Hi,
 
 the return value of mempcpy is only correct when the destination type is
 one byte in size. This patch casts the argument to a char* so the
 calculation is also correct for structs, ints etc. Comments?
 
 Ping?
 
 Cheers,
 Daniel Fahlgren
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] make analyzer track memory allocated by if_nameindex

[Anna Zaks]

+MOK_None,
Do we need this one?

+if (Family == AF_Malloc  CheckFree) {
+if (Family == AF_Malloc  CheckAlloc) {

A possible micro optimization would be to check the family once for the common 
two cases. Also, note that ChecksEnabled[CK_MallocOptimistic] most commonly 
evaluates to false.
if (Family == AF_Malloc) {
if (CheckFree) {
  if ()   ...
} else {
  assert(CheckAlloc); 
  if ()   ...
}
if (!ChecksEnabled[CK_MallocOptimistic])
  return false;
}

Do you have commit access?

Thanks!
Anna.

 On Oct 1, 2014, at 3:09 PM, Daniel Fahlgren dan...@fahlgren.se wrote:
 
 Hi Anna,
 
 On ons, 2014-09-24 at 10:02 -0700, Anna Zaks wrote:
 How about the similar functions from the malloc family:
 isAllocationFunction and isFreeFunction?
 
 You could either introduce a helper function which checks if the
 FunctionDecl declares a function from the given list of identifiers or
 introduce a function that takes FunctionDecl, ASTContext, familyKind,
 and MemoryOperationKind (enum class MemoryOperationKind { MOK_Allocate,
 MOK_Free };) and checks if the FunctionDecl belongs to that family and
 memory operation. (The second approach is probably better.)
 
 Yes, the second approach does sound better. Combining the four functions
 into one required a bit if thinking in order to maintain readability.
 Attached is a patch that refactors isAllocationFunction and
 isFreeFunction, as well as adding support for if_*nameindex functions.
 
 Any comments?
 
 Cheers,
 Daniel
 ifnameindex.patch

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Dereference then check

[Anna Zaks]

Anders,

Could you describe the high-level algorithm you are using?

Here is one example I would expect to work.
int test (int *x, int cond) {
  if (cond)
*x = 1; // expected-warning
  else 
*x = 2;  // expected-warning
  if (!x)
return 0;
 // *x = 0;// no-warning

  return 1;
}

I have a bunch of other minor comments about the other parts of the patch that 
I will send separately.

Anna.

 On Sep 25, 2014, at 7:30 AM, Anders Rönnholm anders.ronnh...@evidente.se 
 wrote:
 
 Hi,
 
 I have made a new checker similar to the divison by zero i previously 
 submitted.
 
 This one checks for dereferences that are later compared against zero.
 
 As requested i have made it CFG-based like how DeadStores is implemented. I 
 hope this is how you meant.
 
 //Andersderefthencheck.diff


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Dereference then check

[Anna Zaks]

Anders, 

Here are some comments about the patch. However, I am still to understand and 
review the core algorithm you are using (asked in another email).

Thanks!
Anna.

Index: include/clang/Analysis/Analyses/TestAfter.h
===
--- include/clang/Analysis/Analyses/TestAfter.h (revision 0)
+++ include/clang/Analysis/Analyses/TestAfter.h (working copy)
@@ -0,0 +1,71 @@
+//===- TestAfter.h - Test after usage analysis for Source CFGs *- C++ 
--*-//
Please, describe the algorithm used and the specifics - test after usage is 
very generic. (Ex: What is considered a use? What is considered a test?)

+class TestAfter : public ManagedAnalysis {
+  class InvalidatedValues {
The names are off and/or not explained. We have a TestAfter analysis that 
collects InvalidatedValues.

+bool isDereferenced(const VarDecl *D) const;
Please, add explanation what this returns. Is this true for the pointer values 
that have been dereferenced on all paths leading to this block or something 
else?

--- lib/StaticAnalyzer/Checkers/DereferenceThenCheck.cpp(revision 0)
+++ lib/StaticAnalyzer/Checkers/DereferenceThenCheck.cpp(working copy)
@@ -0,0 +1,85 @@
+BR.EmitBasicReport(AC-getDecl(), Checker, DerefBug, DerefBug,
+   Possible null pointer dereference, L, R);

Is there a reason why the warning message is different than what the 
DereferenceChecker produces?


Index: lib/Analysis/TestAfter.cpp
===
--- lib/Analysis/TestAfter.cpp  (revision 0)
+++ lib/Analysis/TestAfter.cpp  (working copy)
@@ -0,0 +1,165 @@

+  TestAfter::InvalidatedValues runOnBlock(const CFGBlock *Block,
+   TestAfter::InvalidatedValues 
DerefVal,
+   TestAfter::Observer *Obs = nullptr);
Please, fix 80 col rule and alignment.

+DSetFact(false) // This is a *major* performance win.
Is the comment true?


--- test/Analysis/dereference-then-check.c  (revision 0)
+++ test/Analysis/dereference-then-check.c  (working copy)
@@ -0,0 +1,86 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core -verify %s
+// RUN: %clang_cc1 -x c++ -analyze -analyzer-checker=core,alpha.core -verify %s

I've noticed that none of the checks, have if (p) as a check. It's worth 
adding it since this is the most common pattern for checking if a pointer is 
null.

+void foo55(char *p) {
+  if (1)
+if (*p == 0) {
+}
+  if (!p) {
+  }
+}

Strange indentation. If possible, please use more descriptive names in false 
negative tests (hinting on the underlying cause) .

+void f(int **p);
+void foo6() {
+  int *p;
+  f(p);
+  if (!p) {
+  }
+}
+
+int *w;
+int **ff() { return w; }
+
+void foo7() {
+  int **p = ff();
+  if (!p) {
+  }
+}
Are these supposed to be false negatives? Looks like normal tests got mixed 
with false negatives (below as well)..

+int x;
+void foo8(int *p) {
+  if (x)
+p = 0;
+  else
+*p = 0; // no-warning
+  if (!p) {
+  }
+}
For the tests where you test for absence of warnings, please, add // 
no-warning. This is the case for the test above. 
Also, this test can be renamed into something like 
warn_only_if_all_path_dereference.

Let's add this test for completeness:
int inlined_defensive_check(int *p) {
  if (p)
return 5;
  return 0;
}
int use_inlined_defensive_check(int *p) {
  int x = *p;  // no-warning
  return x + inlined_defensive_check(p);
}

Also, the should produce 2 warnings in this case:
void test_two_dereferences(int *p, char cond) {
  if (cond) {
*p = 3;  // expected-warning
  } else {
*p = 1; // expected-warning
  }
  if (p) {
cond = 1;
  }
}

 On Sep 25, 2014, at 7:30 AM, Anders Rönnholm anders.ronnh...@evidente.se 
 wrote:
 
 Hi,
 
 I have made a new checker similar to the divison by zero i previously 
 submitted.
 
 This one checks for dereferences that are later compared against zero.
 
 As requested i have made it CFG-based like how DeadStores is implemented. I 
 hope this is how you meant.
 
 //Andersderefthencheck.diff


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [Proposal] [Analyzer] Individual options for checkers

[Anna Zaks]

Aleksei,

Thanks for working on this! 

I've added a small comment above. 

The main problem with this patch is that it is lacking test cases. Is it 
possible to use it for the cases in CString checker and/or MallocChecker you've 
mentioned above?

Anna.


Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:274
@@ -249,3 +273,3 @@
   /// Interprets an option's string value as a boolean.
   ///
   /// Accepts the strings true and false.

Please, add a comment about CheckerBase parameter in all the changed public 
methods.

http://reviews.llvm.org/D3967



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] make analyzer track memory allocated by if_nameindex

[Anna Zaks]

How about the similar functions from the malloc family: isAllocationFunction 
and isFreeFunction?

You could either introduce a helper function which checks if the FunctionDecl 
declares a function from the given list of identifiers or introduce a function 
that takes FunctionDecl, ASTContext, familyKind, and MemoryOperationKind (enum 
class MemoryOperationKind { MOK_Allocate, MOK_Free };) and checks if the 
FunctionDecl belongs to that family and memory operation. (The second approach 
is probably better.)

Cheers,
Anna.

 On Sep 24, 2014, at 3:06 AM, Daniel Fahlgren dan...@fahlgren.se wrote:
 
 Hi Anna,
 
 On Mon, 2014-09-22 at 22:54 -0700, Anna Zaks wrote:
 The bodies of these functions look very similar. Please, try to factor
 out the copy and paste.
 +bool MallocChecker::isIfNameFunction(const FunctionDecl *FD,
 +bool MallocChecker::isIfFreeNameFunction(const FunctionDecl *FD,
 ASTContext C) const {
 
 Otherwise, looks good to me.
 
 Thanks for reviewing this. Attached is an updated patch where the two
 functions are refactored to a single one.
 
 Best regards,
 Daniel Fahlgren
 ifnameindex.patch

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Division by zero

[Anna Zaks]

 On Sep 15, 2014, at 1:59 AM, Anders Rönnholm anders.ronnh...@evidente.se 
 wrote:
 
 Hi,
 
 I feel that to change this checker and the null dereference check would take 
 a large amount of time compared to what is gained, time which could be used 
 more efficiently on other checkers.
 The null dereference check is already completed as path sensitive and works 
 well.
 
 We are talking about converting only the check after division/dereference 
 (not regular div by zero or dereference checks) because these checks require 
 all paths reasoning (See the [cfe-dev] [RFC] Creating base class for 'Test 
 after X' checkers thread). The main win is speed (flow sensitive analyzes 
 are algorithmically much simpler than the path sensitive ones), which also 
 opens a possibility of converting this into a compiler warning.
 
 I agree that it would not be a very easy task, but this is the right way to 
 approach the problem.
 
 I agree with Anna. Doing this because it's convenient is really just 
 technical debt and isn't something we'd necessarily be comfortable moving 
 out of the alpha package, meaning that plenty of users won't even know it 
 exists. I can see us very easily never coming back to do the right thing 
 here.
 
 Jordan
 
 We still need to know more of how to do the right thing. Can you help us 
 more on how to do it cfg-based? Do we need to create our own LiveVariables 
 class for our checkers and then observe it like DeadStoresChecker observes 
 LiveVariables? 
 

Consuming the result of data flow analysis in a checker is a good approach for 
producing static analyzer warnings. I can think of one reason that might need 
to change; specifically, if the analysis is cheap enough to turn it into a 
warning. It is hard to say if that will be the case ahead of time.

Anna.

 //Anders


___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] move NewDeleteLeaks checker from alpha.cplusplus to cplusplus group.

[Anna Zaks]

Anton,

Have you tested this on any C++ codebase other than LLVM? It would be really 
great to confirm the results by testing this on a different project.

http://reviews.llvm.org/D5313



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Anna Zaks]

 On Sep 22, 2014, at 10:03 AM, Artyom Skrobov artyom.skro...@arm.com wrote:
 
 Alexander, I’m now looking at your example, thank you!
  
 Anna, my question about the revision was to make sure I understand which 
 patch you want reverted.
 There were two patches, with a few weeks in between the two commits.
 The first (r215650, Jul 28) introduced a performance regression, the second 
 (r215650, Aug 14) fixed it for most but evidently not all cases.
  
 When did PostgreSQL start timing out?
  

It started timing out after r215650. I'll look into the performance 
implications of 214064 on our side.

Anna.
  
 From: Anna Zaks [mailto:ga...@apple.com] 
 Sent: 20 September 2014 18:58
 To: Artyom Skrobov
 Cc: cfe-commits@cs.uiuc.edu; Alexander Kornienko
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, 
 to recover the performance after r214064
  
 Artyom,
  
 PostgreSQL started timing out or taking a VERY long time. We have a Bulidbot 
 that builds several projects and none of them were timing out before this 
 commit. I don't know the specific revision; but it is PostgreSQL 9.1.
  
 I suggest reverting this commit and investigating why it causes the 
 regression. Generally, we should come up with a solution that does not take 
 hours on any of the benchmarks.
  
 Anna.
 
 Sent from my iPhone
 
 On Sep 20, 2014, at 8:10 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 
 Anna, do you mean the performance had been acceptable after r214064, but 
 degraded after r215650, which fixed the performance regression introduced in 
 r214064?
  
 Do you have any specific example of code that takes longer to compile after 
 r215650?
  
 Not hearing back from Alexander since August, I assumed the performance 
 regression he observed after r215650 was not in fact related to that commit.
  
  
 I suspect it is related.
 
 From: Anna Zaks [mailto:ga...@apple.com mailto:ga...@apple.com] 
 Sent: 20 September 2014 01:19
 To: Artyom Skrobov
 Cc: cfe-commits@cs.uiuc.edu mailto:cfe-commits@cs.uiuc.edu Commits; Ted 
 Kremenek; Jordan Rose; Alexander Kornienko
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, 
 to recover the performance after r214064
  
 Hi Artyom,
  
 Unfortunately, this commit (r215650) causes major performance regressions on 
 our buildbots. In particular, building postgresql-9.1 times out.
  
 Please, revert as soon as possible.
  
 Thank you,
 Anna.
 On Aug 20, 2014, at 3:13 AM, Alexander Kornienko ale...@google.com 
 mailto:ale...@google.com wrote:
  
 On Fri, Aug 15, 2014 at 10:38 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 Many thanks -- committed as r215650
 
 Alexander, can you confirm that the analyzer performance is now acceptable
 for your use cases?
  
 Artyom, sorry for the long delay. These files now work fine, but I still see 
 up to 8-10 hours analysis time on a couple of other files. I'm sure I didn't 
 see this before your first patch, but I can't yet tell in which revision it 
 was introduced. I'll post more details and a repro later today.
  
 
 
 -Original Message-
 From: Ted kremenek [mailto:kreme...@apple.com mailto:kreme...@apple.com]
 Sent: 14 August 2014 16:36
 To: Artyom Skrobov
 Cc: Alexander Kornienko; cfe-commits@cs.uiuc.edu 
 mailto:cfe-commits@cs.uiuc.edu
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables
 analysis, to recover the performance after r214064
 
 Looks great to me.
 
  On Aug 14, 2014, at 3:08 AM, Artyom Skrobov artyom.skro...@arm.com 
  mailto:artyom.skro...@arm.com
 wrote:
 
  Thank you Ted!
 
  Attaching the updated patch for a final review.
 
  Summary of changes:
 
  * Comments updated to reflect the two possible CFG traversal orders
  * PostOrderCFGView::po_iterator taken out of the header file
  * Iteration order for PostOrderCFGView changed to reverse inverse
  post-order, the one required for a backward analysis
  * ReversePostOrderCFGView created, with the same iteration order that
  PostOrderCFGView used to have, the one required for a forward analysis
  * The two previous consumers of PostOrderCFGView, ThreadSafetyCommon.h and
  Consumed.cpp, switched to use ReversePostOrderCFGView
  * DataflowWorklistBase renamed to DataflowWorklist, and the two
  specializations named BackwardDataflowWorklist and ForwardDataflowWorklist
 
  I believe this naming scheme matches the accepted terminology best.
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu mailto:cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits 
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Anna Zaks]

On our builedbot that analyzes several projects (such as openssl, adium, 
postgresql), we see a 17% slowdown after r214064 and time out after r215650.

The commit message for r214064 states that it's Factoring DataflowWorklist out 
of LiveVariables and UninitializedValues analyses. A simple factoring should 
not cause the 17% slowdown. 

Let's revert both patches while we are investigating what's going on.

Anna.
 On Sep 22, 2014, at 11:06 AM, Anna Zaks ga...@apple.com wrote:
 
 
 On Sep 22, 2014, at 10:03 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 
 Alexander, I’m now looking at your example, thank you!
  
 Anna, my question about the revision was to make sure I understand which 
 patch you want reverted.
 There were two patches, with a few weeks in between the two commits.
 The first (r215650, Jul 28) introduced a performance regression, the second 
 (r215650, Aug 14) fixed it for most but evidently not all cases.
  
 When did PostgreSQL start timing out?
  
 
 It started timing out after r215650. I'll look into the performance 
 implications of 214064 on our side.
 
 Anna.
  
 From: Anna Zaks [mailto:ga...@apple.com mailto:ga...@apple.com] 
 Sent: 20 September 2014 18:58
 To: Artyom Skrobov
 Cc: cfe-commits@cs.uiuc.edu mailto:cfe-commits@cs.uiuc.edu; Alexander 
 Kornienko
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables 
 analysis, to recover the performance after r214064
  
 Artyom,
  
 PostgreSQL started timing out or taking a VERY long time. We have a Bulidbot 
 that builds several projects and none of them were timing out before this 
 commit. I don't know the specific revision; but it is PostgreSQL 9.1.
  
 I suggest reverting this commit and investigating why it causes the 
 regression. Generally, we should come up with a solution that does not take 
 hours on any of the benchmarks.
  
 Anna.
 
 Sent from my iPhone
 
 On Sep 20, 2014, at 8:10 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 
 Anna, do you mean the performance had been acceptable after r214064, but 
 degraded after r215650, which fixed the performance regression introduced in 
 r214064?
  
 Do you have any specific example of code that takes longer to compile after 
 r215650?
  
 Not hearing back from Alexander since August, I assumed the performance 
 regression he observed after r215650 was not in fact related to that commit.
  
  
 I suspect it is related.
 
 From: Anna Zaks [mailto:ga...@apple.com mailto:ga...@apple.com] 
 Sent: 20 September 2014 01:19
 To: Artyom Skrobov
 Cc: cfe-commits@cs.uiuc.edu mailto:cfe-commits@cs.uiuc.edu Commits; Ted 
 Kremenek; Jordan Rose; Alexander Kornienko
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables 
 analysis, to recover the performance after r214064
  
 Hi Artyom,
  
 Unfortunately, this commit (r215650) causes major performance regressions on 
 our buildbots. In particular, building postgresql-9.1 times out.
  
 Please, revert as soon as possible.
  
 Thank you,
 Anna.
 On Aug 20, 2014, at 3:13 AM, Alexander Kornienko ale...@google.com 
 mailto:ale...@google.com wrote:
  
 On Fri, Aug 15, 2014 at 10:38 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 Many thanks -- committed as r215650
 
 Alexander, can you confirm that the analyzer performance is now acceptable
 for your use cases?
  
 Artyom, sorry for the long delay. These files now work fine, but I still see 
 up to 8-10 hours analysis time on a couple of other files. I'm sure I didn't 
 see this before your first patch, but I can't yet tell in which revision it 
 was introduced. I'll post more details and a repro later today.
  
 
 
 -Original Message-
 From: Ted kremenek [mailto:kreme...@apple.com mailto:kreme...@apple.com]
 Sent: 14 August 2014 16:36
 To: Artyom Skrobov
 Cc: Alexander Kornienko; cfe-commits@cs.uiuc.edu 
 mailto:cfe-commits@cs.uiuc.edu
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables
 analysis, to recover the performance after r214064
 
 Looks great to me.
 
  On Aug 14, 2014, at 3:08 AM, Artyom Skrobov artyom.skro...@arm.com 
  mailto:artyom.skro...@arm.com
 wrote:
 
  Thank you Ted!
 
  Attaching the updated patch for a final review.
 
  Summary of changes:
 
  * Comments updated to reflect the two possible CFG traversal orders
  * PostOrderCFGView::po_iterator taken out of the header file
  * Iteration order for PostOrderCFGView changed to reverse inverse
  post-order, the one required for a backward analysis
  * ReversePostOrderCFGView created, with the same iteration order that
  PostOrderCFGView used to have, the one required for a forward analysis
  * The two previous consumers of PostOrderCFGView, ThreadSafetyCommon.h and
  Consumed.cpp, switched to use ReversePostOrderCFGView
  * DataflowWorklistBase renamed to DataflowWorklist, and the two
  specializations named BackwardDataflowWorklist and ForwardDataflowWorklist
 
  I believe

Re: [PATCH] make analyzer track memory allocated by if_nameindex

[Anna Zaks]

The bodies of these functions look very similar. Please, try to factor out the 
copy and paste.
+bool MallocChecker::isIfNameFunction(const FunctionDecl *FD,
+bool MallocChecker::isIfFreeNameFunction(const FunctionDecl *FD, ASTContext 
C) const {

Otherwise, looks good to me.

Thank you!
Anna.

 On Sep 22, 2014, at 9:20 AM, Daniel Fahlgren dan...@fahlgren.se 
 mailto:dan...@fahlgren.se wrote:
 
 Hi,
 
 On Wed, 2014-09-03 at 19:27 -0700, Jordan Rose wrote:
 [+Anna, Anton] This does seem very much like a new allocation family.
 Do we have a policy on how we're going to handle these in general,
 though? The MacOSKeychainAPIChecker also handles allocation-like
 tracking, as does SimpleStreamChecker. What does everyone think we
 should do?
 
 My personal opinion (though without thinking too long) is that
 aggregating new allocators under MallocChecker is the right thing to
 do for now—i.e. we should take this patch. We may even want to come up
 with a way to make this nicely extensible/configurable in the future.
 But there are a lot of APIs that work this way, so...
 
 (We can keep SimpleStreamChecker distinct even if we fold fopen/fclose
 under MallocChecker, since it's still a good example of how the
 analyzer works.)
 
 Jordan
 
 Ping. What is the next step for this patch, is more work needed? Is
 there something that I should do?
 
 
 On Aug 26, 2014, at 8:45 , Daniel Fahlgren dan...@fahlgren.se 
 mailto:dan...@fahlgren.se wrote:
 
 Hi,
 
 The MallocChecker does currently not track the memory allocated by
 if_nameindex. That memory is dynamically allocated and should be
 freed
 by calling if_freenameindex. The attached patch teaches the checker
 about these functions.
 
 Memory allocated by if_nameindex is treated as a separate allocation
 family. That way the checker can verify it is freed by the correct
 function.
 
 Any comments / feedback?
 
 Cheers,
 Daniel Fahlgren

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Anna Zaks]

Artyom,

PostgreSQL started timing out or taking a VERY long time. We have a Bulidbot 
that builds several projects and none of them were timing out before this 
commit. I don't know the specific revision; but it is PostgreSQL 9.1.

I suggest reverting this commit and investigating why it causes the regression. 
Generally, we should come up with a solution that does not take hours on any of 
the benchmarks.

Anna.

Sent from my iPhone

 On Sep 20, 2014, at 8:10 AM, Artyom Skrobov artyom.skro...@arm.com wrote:
 
 Anna, do you mean the performance had been acceptable after r214064, but 
 degraded after r215650, which fixed the performance regression introduced in 
 r214064?
  
 Do you have any specific example of code that takes longer to compile after 
 r215650?
  
 Not hearing back from Alexander since August, I assumed the performance 
 regression he observed after r215650 was not in fact related to that commit.
  
  
I suspect it is related.
 From: Anna Zaks [mailto:ga...@apple.com] 
 Sent: 20 September 2014 01:19
 To: Artyom Skrobov
 Cc: cfe-commits@cs.uiuc.edu Commits; Ted Kremenek; Jordan Rose; Alexander 
 Kornienko
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, 
 to recover the performance after r214064
  
 Hi Artyom,
  
 Unfortunately, this commit (r215650) causes major performance regressions on 
 our buildbots. In particular, building postgresql-9.1 times out.
  
 Please, revert as soon as possible.
  
 Thank you,
 Anna.
 On Aug 20, 2014, at 3:13 AM, Alexander Kornienko ale...@google.com wrote:
  
 On Fri, Aug 15, 2014 at 10:38 AM, Artyom Skrobov artyom.skro...@arm.com 
 wrote:
 Many thanks -- committed as r215650
 
 Alexander, can you confirm that the analyzer performance is now acceptable
 for your use cases?
  
 Artyom, sorry for the long delay. These files now work fine, but I still see 
 up to 8-10 hours analysis time on a couple of other files. I'm sure I didn't 
 see this before your first patch, but I can't yet tell in which revision it 
 was introduced. I'll post more details and a repro later today.
  
 
 
 -Original Message-
 From: Ted kremenek [mailto:kreme...@apple.com]
 Sent: 14 August 2014 16:36
 To: Artyom Skrobov
 Cc: Alexander Kornienko; cfe-commits@cs.uiuc.edu
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables
 analysis, to recover the performance after r214064
 
 Looks great to me.
 
  On Aug 14, 2014, at 3:08 AM, Artyom Skrobov artyom.skro...@arm.com
 wrote:
 
  Thank you Ted!
 
  Attaching the updated patch for a final review.
 
  Summary of changes:
 
  * Comments updated to reflect the two possible CFG traversal orders
  * PostOrderCFGView::po_iterator taken out of the header file
  * Iteration order for PostOrderCFGView changed to reverse inverse
  post-order, the one required for a backward analysis
  * ReversePostOrderCFGView created, with the same iteration order that
  PostOrderCFGView used to have, the one required for a forward analysis
  * The two previous consumers of PostOrderCFGView, ThreadSafetyCommon.h and
  Consumed.cpp, switched to use ReversePostOrderCFGView
  * DataflowWorklistBase renamed to DataflowWorklist, and the two
  specializations named BackwardDataflowWorklist and ForwardDataflowWorklist
 
  I believe this naming scheme matches the accepted terminology best.
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
  
___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Anna Zaks]

Hi Artyom,

Unfortunately, this commit (r215650) causes major performance regressions on 
our buildbots. In particular, building postgresql-9.1 times out.

Please, revert as soon as possible.

Thank you,
Anna.
 On Aug 20, 2014, at 3:13 AM, Alexander Kornienko ale...@google.com wrote:
 
 On Fri, Aug 15, 2014 at 10:38 AM, Artyom Skrobov artyom.skro...@arm.com 
 mailto:artyom.skro...@arm.com wrote:
 Many thanks -- committed as r215650
 
 Alexander, can you confirm that the analyzer performance is now acceptable
 for your use cases?
 
 Artyom, sorry for the long delay. These files now work fine, but I still see 
 up to 8-10 hours analysis time on a couple of other files. I'm sure I didn't 
 see this before your first patch, but I can't yet tell in which revision it 
 was introduced. I'll post more details and a repro later today.
  
 
 
 -Original Message-
 From: Ted kremenek [mailto:kreme...@apple.com mailto:kreme...@apple.com]
 Sent: 14 August 2014 16:36
 To: Artyom Skrobov
 Cc: Alexander Kornienko; cfe-commits@cs.uiuc.edu 
 mailto:cfe-commits@cs.uiuc.edu
 Subject: Re: [PATCH] Inverse post-order traversal for LiveVariables
 analysis, to recover the performance after r214064
 
 Looks great to me.
 
  On Aug 14, 2014, at 3:08 AM, Artyom Skrobov artyom.skro...@arm.com 
  mailto:artyom.skro...@arm.com
 wrote:
 
  Thank you Ted!
 
  Attaching the updated patch for a final review.
 
  Summary of changes:
 
  * Comments updated to reflect the two possible CFG traversal orders
  * PostOrderCFGView::po_iterator taken out of the header file
  * Iteration order for PostOrderCFGView changed to reverse inverse
  post-order, the one required for a backward analysis
  * ReversePostOrderCFGView created, with the same iteration order that
  PostOrderCFGView used to have, the one required for a forward analysis
  * The two previous consumers of PostOrderCFGView, ThreadSafetyCommon.h and
  Consumed.cpp, switched to use ReversePostOrderCFGView
  * DataflowWorklistBase renamed to DataflowWorklist, and the two
  specializations named BackwardDataflowWorklist and ForwardDataflowWorklist
 
  I believe this naming scheme matches the accepted terminology best.
 
 ___
 cfe-commits mailing list
 cfe-commits@cs.uiuc.edu
 http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] Division by zero

[Anna Zaks]

 On Sep 5, 2014, at 5:24 AM, Anders Rönnholm anders.ronnh...@evidente.se 
 wrote:
 
 Hi,
 
 I feel that to change this checker and the null dereference check would take 
 a large amount of time compared to what is gained, time which could be used 
 more efficiently on other checkers.
 The null dereference check is already completed as path sensitive and works 
 well.

We are talking about converting only the check after division/dereference 
(not regular div by zero or dereference checks) because these checks require 
all paths reasoning (See the [cfe-dev] [RFC] Creating base class for 'Test 
after X' checkers thread). The main win is speed (flow sensitive analyzes are 
algorithmically much simpler than the path sensitive ones), which also opens a 
possibility of converting this into a compiler warning.

I agree that it would not be a very easy task, but this is the right way to 
approach the problem.

 I don't know when we can deliver this as CFG-based (definitely not this 
 year),  wouldn't it be better to have it like this now?
 
 For a possible remake of this checker to a CFG-based one in the future, we 
 would need more help from you on how to make them CFG-based. What parts of 
 LiveVariables and DeadStoresChecker are related to our check? I guess just 
 parts of the LiveVariables framework is needed.

DeadStoresChecker is an example of other flow sensitive checks. You would need 
to create a similar one for div by zero / dereference. 

 
 So, Anna brought up that the check as implemented is very nearly 
 path-independent, i.e. it only depends on flow-sensitive properties of the 
 CFG. The path-sensitivity is buying us very little; it catches this case:
 
 int y = x;
 int div = z / y;
 if (x) { ...}
 
 But also warns here, which doesn't necessarily make sense:
 
 int foo(int x, int y, int z) {
   int div = z / y;
   if (x) return div;
   return 0;
 }
 
 foo(a, a, b); // only coincidentally the same symbol
 
 What would you think about turning this (and/or the null dereference check) 
 into a CFG-based check instead? We lose the first example (and cases where 
 inlining would help), but fix the second, and very possibly speed up 
 analysis. CFG analysis is also more capable of proving that something 
 happens on all paths rather than just some, since that's just propagating 
 information along the graph.
 
 I agree, this can in theory be a false positive but we believe it is an 
 unlikely one. Other existing checkers in the Clang static analyser have the 
 same problem. I don't have any proposal, but maybe a generic solution for 
 such FP would be good. In the long run I think it would be nice to have full 
 flow analysis in this checker so we can find deeper bugs that are not limited 
 to a single basic block.
 

Again, the main issue is the algorithmic performance win, not false positives.

 //Anders

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Detect use-after-free scenarios in -dealloc after calling [super dealloc]

[Anna Zaks]

I agree with Jordan. We would probably want to create a path sensitive check 
here instead of using the matchers. Another advantage would be that you would 
get inter procedural analysis (within the same translation unit), so if the 
dealloc delegates the deallocation to another method whose implementation is 
within the same TU, you would get the checking as if the callee has been 
inlined.

(I am not sure if you watched our presentation about writing path sensitive 
checkers. If not, I highly recommend it. It's called Building a Checker in 24 
hours http://www.llvm.org/devmtg/2012-11/)

http://reviews.llvm.org/D5042



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] make analyzer track memory allocated by if_nameindex

[Anna Zaks]

 On Sep 3, 2014, at 7:27 PM, Jordan Rose jordan_r...@apple.com wrote:
 
 [+Anna, Anton] This does seem very much like a new allocation family. Do we 
 have a policy on how we're going to handle these in general, though? The 
 MacOSKeychainAPIChecker also handles allocation-like tracking, as does 
 SimpleStreamChecker. What does everyone think we should do?
 
 My personal opinion (though without thinking too long) is that aggregating 
 new allocators under MallocChecker is the right thing to do for now—i.e. we 
 should take this patch. We may even want to come up with a way to make this 
 nicely extensible/configurable in the future.

This particular patch - a different kind of allocator seems like a perfect fit 
for the MallocChecker the way it is.

 But there are a lot of APIs that work this way, so…

I agree that MacOSKeychainAPI has a lot in common with MallocChecker and it 
would be beneficial to unify them.

 

 (We can keep SimpleStreamChecker distinct even if we fold fopen/fclose under 
 MallocChecker, since it's still a good example of how the analyzer works.)
 
 Jordan
 
 
 On Aug 26, 2014, at 8:45 , Daniel Fahlgren dan...@fahlgren.se 
 mailto:dan...@fahlgren.se wrote:
 
 Hi,
 
 The MallocChecker does currently not track the memory allocated by
 if_nameindex. That memory is dynamically allocated and should be freed
 by calling if_freenameindex. The attached patch teaches the checker
 about these functions.
 
 Memory allocated by if_nameindex is treated as a separate allocation
 family. That way the checker can verify it is freed by the correct
 function.
 
 Any comments / feedback?
 
 Best regards,
 Daniel Fahlgren
 ifnameindex.patch

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Re: [PATCH] [analyzer] Fix ObjC Dealloc Checker to operate only on classes with retained properties

[Anna Zaks]

Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:115
@@ -91,1 +114,3 @@
+  // writable pointers (or id...)?  If not, skip the check entirely.
+  // NOTE: This is motivated by PR 2517 and rdar://problem/6074390:
   //http://llvm.org/bugs/show_bug.cgi?id=2517

We try to keep the codebase clear of radar (and PR) numbers. These are very 
welcome in the commit messages and test cases.


Comment at: lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp:206
@@ -194,3 +205,3 @@
 bool requiresRelease = PD-getSetterKind() != ObjCPropertyDecl::Assign;
 if (scan_ivar_release(MD-getBody(), ID, PD, RS, SelfII, Ctx)
!= requiresRelease) {

Would it be possible to move the checking of the setter (to be not 'assign') 
into the body of isSynthesizedWritablePointerProperty() ?


Comment at: test/Analysis/DeallocMissingRelease.m:142
@@ +141,3 @@
+
+// RUN: not %clang_cc1 -analyze -analyzer-checker=alpha.osx.cocoa.Dealloc 
-fblocks -triple x86_64-apple-darwin10 -fobjc-arc %s 21 | FileCheck 
-check-prefix=CHECK-ARC %s
+// CHECK-ARC: DeallocMissingRelease.m:32:17: error: ARC forbids explicit 
message send of 'retain'

All of the run lines are usually placed in the front of the test file.


Comment at: test/Analysis/MissingDealloc.m:139
@@ +138,3 @@
+
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.osx.cocoa.Dealloc -fblocks 
%s 21 | FileCheck -check-prefix=CHECK %s
+// CHECK: MissingDealloc.m:58:1: warning: Objective-C class 
'MissingDeallocWithCopyProperty' lacks a 'dealloc' instance method

Run commands need to go into the beginning of the file.



Comment at: test/Analysis/PR2978.m:36
@@ -35,3 +35,3 @@
 @synthesize X = _X;
-@synthesize Y = _Y; // expected-warning{{The '_Y' instance variable was 
retained by a synthesized property but wasn't released in 'dealloc'}}
-@synthesize Z = _Z; // expected-warning{{The '_Z' instance variable was not 
retained by a synthesized property but was released in 'dealloc'}}
+@synthesize Y = _Y; // expected-warning{{The '_Y' instance variable in 
Objective-C class 'MyClass' was retained by a synthesized property but was not 
released in 'dealloc'}}
+@synthesize Z = _Z; // expected-warning{{The '_Z' instance variable in 
Objective-C class 'MyClass' was not retained by a synthesized property but was 
released in 'dealloc'}}

We prefer to keep the warning messages as short as possible. (One reason is 
that the IDE real estate is at a premium.) So here, I would drop Objective-C 
class.

http://reviews.llvm.org/D5023



___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

[PATCH] Add a flag to disable all analyzer warnings

[Anna Zaks]

Please, review:Add an option to silence all analyzer warnings.People have been incorrectly using "-analyzer-disable-checker" tosilence analyzer warnings on a file, when analyzing a project. Addthe "-analyzer-disable-all-checks" option, which would allow to dothis and suggest it as part of the error message for"-analyzer-disable-checker".

disable-all-checks.diff
Description: Binary data

___
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits