[PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Nevermind, the order is correct!


https://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Bump?


https://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In https://reviews.llvm.org/D18073#504216, @dcoughlin wrote:

> No. The identifier info will be null for C++ operators.


I assume you mean `operator new/new[]/delete/delete[]

> > Thus, when`! isWindowsMSVCEnvironment`, I leave the Windows-only memory 
> > allocating functions initialized to `nullptr`, which will never equal a 
> > non-null `IdentifierInfo*`, and never trigger on a non-Windows platform.

> 

> 

> It is probably better to be explicit about this.





https://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio marked an inline comment as done.
ariccio added a comment.

As said in comment, I disagree with the no need for `const` here.



Comment at: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1396
@@ -1395,3 +1395,3 @@
   const auto &ReferencedBlockVars = AC->getReferencedBlockVars(BC->getDecl());
-  auto NumBlockVars =
+  const auto NumBlockVars =
   std::distance(ReferencedBlockVars.begin(), ReferencedBlockVars.end());

aaron.ballman wrote:
> No need for a const here; the correct change is to not use auto (here and the 
> line above), but instead spell out the type explicitly.
The const is partly to make sure that the `if (NumBlockVars == 0) {` line never 
accidentally becomes `if (NumBlockVars = 0) {` like it did in CPython:

http://bugs.python.org/issue25844


Comment at: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1396
@@ -1395,3 +1395,3 @@
   const auto &ReferencedBlockVars = AC->getReferencedBlockVars(BC->getDecl());
-  auto NumBlockVars =
+  const auto NumBlockVars =
   std::distance(ReferencedBlockVars.begin(), ReferencedBlockVars.end());

ariccio wrote:
> aaron.ballman wrote:
> > No need for a const here; the correct change is to not use auto (here and 
> > the line above), but instead spell out the type explicitly.
> The const is partly to make sure that the `if (NumBlockVars == 0) {` line 
> never accidentally becomes `if (NumBlockVars = 0) {` like it did in CPython:
> 
> http://bugs.python.org/issue25844
For archival reasons, I'll copy & paste the relevant diff here (I hate dead 
links):


```
--- a/PC/launcher.c
+++ b/PC/launcher.c
@@ -114,7 +114,7 @@ static wchar_t * get_env(wchar_t * key)
 if (result >= BUFSIZE) {
 /* Large environment variable. Accept some leakage */
 wchar_t *buf2 = (wchar_t*)malloc(sizeof(wchar_t) * (result+1));
-if (buf2 = NULL) {
+if (buf2 == NULL) {
 error(RC_NO_MEMORY, L"Could not allocate environment buffer");
 }
 GetEnvironmentVariableW(key, buf2, result);
```


http://reviews.llvm.org/D16748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio marked 3 inline comments as done.
ariccio added a comment.

I will remove that `const` later tonight.



Comment at: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1396
@@ -1395,3 +1395,3 @@
   const auto &ReferencedBlockVars = AC->getReferencedBlockVars(BC->getDecl());
-  auto NumBlockVars =
+  const auto NumBlockVars =
   std::distance(ReferencedBlockVars.begin(), ReferencedBlockVars.end());

aaron.ballman wrote:
> ariccio wrote:
> > ariccio wrote:
> > > aaron.ballman wrote:
> > > > No need for a const here; the correct change is to not use auto (here 
> > > > and the line above), but instead spell out the type explicitly.
> > > The const is partly to make sure that the `if (NumBlockVars == 0) {` line 
> > > never accidentally becomes `if (NumBlockVars = 0) {` like it did in 
> > > CPython:
> > > 
> > > http://bugs.python.org/issue25844
> > For archival reasons, I'll copy & paste the relevant diff here (I hate dead 
> > links):
> > 
> > 
> > ```
> > --- a/PC/launcher.c
> > +++ b/PC/launcher.c
> > @@ -114,7 +114,7 @@ static wchar_t * get_env(wchar_t * key)
> >  if (result >= BUFSIZE) {
> >  /* Large environment variable. Accept some leakage */
> >  wchar_t *buf2 = (wchar_t*)malloc(sizeof(wchar_t) * (result+1));
> > -if (buf2 = NULL) {
> > +if (buf2 == NULL) {
> >  error(RC_NO_MEMORY, L"Could not allocate environment buffer");
> >  }
> >  GetEnvironmentVariableW(key, buf2, result);
> > ```
> While this form of bug can certainly crop up, it's still a bridge-too-far for 
> this project, as I understand it our de facto guidelines on this. I am not 
> certain that we want to start sprinkling const onto value types (as opposed 
> to reference and pointer types) at this point. If we do, it should certainly 
> be something handled a bit more consistently and actively than a general 
> clean-up related to unnecessary type casting.
> If we do, it should certainly be something handled a bit more consistently 
> and actively than a general clean-up related to unnecessary type casting.

You've got a good point there. I'll upload a final revision without that 
`const` a bit later tonight.



http://reviews.llvm.org/D16748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio added a comment.

All set.


http://reviews.llvm.org/D16748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 46737.
ariccio marked 2 inline comments as done.
ariccio added a comment.

Removed single `const`.


http://reviews.llvm.org/D16748

Files:
  
C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===
--- C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1319,8 +1319,8 @@
 
   void setTrait(SymbolRef Sym, InvalidationKinds IK);
   void setTrait(const MemRegion *MR, InvalidationKinds IK);
-  bool hasTrait(SymbolRef Sym, InvalidationKinds IK);
-  bool hasTrait(const MemRegion *MR, InvalidationKinds IK);
+  bool hasTrait(SymbolRef Sym, InvalidationKinds IK) const;
+  bool hasTrait(const MemRegion *MR, InvalidationKinds IK) const;
 };
   
 } // end GR namespace
Index: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -46,7 +46,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -64,7 +64,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -85,7 +85,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, a2, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -104,7 +104,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, a2, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -123,7 +123,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, a2, a3, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -246,39 +246,39 @@
 //===--===//
 
 void MemSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast(getKind()));
 }
 
 void StackSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast(getKind()));
   ID.AddPointer(getStackFrame());
 }
 
 void StaticGlobalSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast(getKind()));
   ID.AddPointer(getCodeRegion());
 }
 
 void StringRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
  const StringLiteral* Str,
  const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) StringRegionKind);
+  ID.AddInteger(static_cast(StringRegionKind));
   ID.AddPointer(Str);
   ID.AddPointer(superRegion);
 }
 
 void ObjCStringRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
  const ObjCStringLiteral* Str,
  const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) ObjCStringRegionKind);
+  ID.AddInteger(static_cast(ObjCStringRegionKind));
   ID.AddPointer(Str);
   ID.AddPointer(superRegion);
 }
 
 void AllocaRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
  const Expr *Ex, unsigned cnt,
  const MemRegion *superRegion) {
-  ID.AddInteger((unsigned) AllocaRegionKind);
+  ID.AddInteger(static_cast(AllocaRegionKind));
   ID.AddPointer(Ex);
   ID.AddInteger(cnt);
   ID.AddPointer(superRegion);
@@ -295,15 +295,15 @@
 void CompoundLiteralRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
   const CompoundLiteralExpr *CL,
   const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) CompoundLiteralRegionKind);
+  ID.AddInteger(static_cast(CompoundLiteralRegionKind));
   ID.AddPointer(CL);
   ID.AddPointer(superRegion);
 }
 
 void CXXThisRegion::ProfileRegion(llvm::FoldingSetNodeID &ID,
   const PointerType *PT,
   const MemRegion *sRegion) {
-  ID.AddInteger((unsigned)

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D16748#343011, @aaron.ballman wrote:

> LGTM, but the patch does not apply cleanly because it was created using 
> absolute paths. In the future, please generate the patch with relative paths. 
> ;-)
>
> Commit in r259652


Oops.


http://reviews.llvm.org/D16748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13893: Roll-back r250822.

[Alexander Riccio via cfe-commits]

ariccio added a subscriber: ariccio.
ariccio added a comment.

Whatever happened to this? What in `ASTMatchers` did it break? It seems like a 
good change, and nobody followed up to fix it?


Repository:
  rL LLVM

http://reviews.llvm.org/D13893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13893: Roll-back r250822.

[Alexander Riccio via cfe-commits]

ariccio added a comment.

See original diff here: http://reviews.llvm.org/D13890


Repository:
  rL LLVM

http://reviews.llvm.org/D13893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio marked an inline comment as done.
ariccio added a comment.

Grr! missed a single note.


http://reviews.llvm.org/D16748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13893: Roll-back r250822.

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D13893#343436, @angelgarcia wrote:

> The compiler complained about creating constant instances of classes
>  without a user provided constructor (which is the case for the ASTMatchers).
>
> I gave up this change because it broke the build for a huge amount of
>  people and I didn't want that to happen again.


Ahh, ok. I saw one of these in MemRegion.cpp, and noticed this.


Repository:
  rL LLVM

http://reviews.llvm.org/D13893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio created this revision.
ariccio added reviewers: aaron.ballman, dcoughlin.
ariccio added a subscriber: cfe-commits.

Instead of one long function, `MemRegionManager::getVarRegion` now calls 
`getMemRegionGloballyStored` for global, non-static variables, and 
`getMemRegionStaticLocal` for static locals. The behavior of 
`MemRegionManager::getVarRegion` is thus clearer, and easier to reason about.



http://reviews.llvm.org/D16873

Files:
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===
--- llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1180,6 +1180,16 @@
   ///  a specified VarDecl and LocationContext.
   const VarRegion* getVarRegion(const VarDecl *D, const LocationContext *LC);
 
+private:
+  /// getMemRegionGloballyStored - Retrieve or create the memory region
+  ///  associated with a specified globally stored, non-static local, VarDecl.
+  const MemRegion *getMemRegionGloballyStored(const VarDecl *D);
+
+  /// getMemRegionStaticLocal - Retrieve or create the memory region
+  ///  associated with a specified VarDecl and LocationContext.
+  const MemRegion *getMemRegionStaticLocal(const VarDecl *D, const LocationContext *LC);
+
+public:
   /// getVarRegion - Retrieve or create the memory region associated with
   ///  a specified VarDecl and super region.
   const VarRegion* getVarRegion(const VarDecl *D, const MemRegion *superR);
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -766,87 +766,89 @@
   return (const StackFrameContext *)nullptr;
 }
 
-const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
-const LocationContext *LC) {
-  const MemRegion *sReg = nullptr;
+const MemRegion* MemRegionManager::getMemRegionGloballyStored(const VarDecl *D) {
+  assert(D->hasGlobalStorage());
+  assert(!D->isStaticLocal());
+  // First handle the globals defined in system headers.
+  if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
+// Whitelist the system globals which often DO GET modified, assume the
+// rest are immutable.
+if (D->getName().find("errno") != StringRef::npos)
+  return getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
 
-  if (D->hasGlobalStorage() && !D->isStaticLocal()) {
+return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+  }
+  // Treat other globals as GlobalInternal unless they are constants.
+  QualType GQT = D->getType();
+  const Type *GT = GQT.getTypePtrOrNull();
+  // TODO: We could walk the complex types here and see if everything is
+  // constified.
+  if (GT && GQT.isConstQualified() && GT->isArithmeticType())
+return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
 
-// First handle the globals defined in system headers.
-if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
-  // Whitelist the system globals which often DO GET modified, assume the
-  // rest are immutable.
-  if (D->getName().find("errno") != StringRef::npos)
-sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
-  else
-sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+  return getGlobalsRegion();
+}
 
-// Treat other globals as GlobalInternal unless they are constants.
-} else {
-  QualType GQT = D->getType();
-  const Type *GT = GQT.getTypePtrOrNull();
-  // TODO: We could walk the complex types here and see if everything is
-  // constified.
-  if (GT && GQT.isConstQualified() && GT->isArithmeticType())
-sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-  else
-sReg = getGlobalsRegion();
-}
+const MemRegion* MemRegionManager::getMemRegionStaticLocal(const VarDecl *D, const LocationContext *LC) {
+  // FIXME: Once we implement scope handling, we will need to properly lookup
+  // 'D' to the proper LocationContext.
+  const DeclContext *DC = D->getDeclContext();
+  llvm::PointerUnion V =
+getStackOrCaptureRegionForDeclContext(LC, DC, D);
 
-  // Finally handle static locals.
-  } else {
-// FIXME: Once we implement scope handling, we will need to properly lookup
-// 'D' to the proper LocationContext.
-const DeclContext *DC = D->getDeclContext();
-llvm::PointerUnion V =
-  getStackOrCaptureRegionForDeclContext(LC, DC, D);
+  if (V.is())
+return V.get();
 
-if (V.is())
-  return V.get();
+  const StackFrameContext *STC = V.get();
 
-const 

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Responded to comments.

Will happily make changes when questions are answered.



Comment at: 
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1186
@@ +1185,3 @@
+  ///  associated with a specified globally stored, non-static local, VarDecl.
+  const MemRegion *getMemRegionGloballyStored(const VarDecl *D);
+

a.sidorin wrote:
> How about make these helper functions return `const MemSpaceRegion *` to make 
> their signatures more meaningful?
Would that change their behavior functionally?


Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:769
@@ -768,4 +768,3 @@
 
-const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
-const LocationContext *LC) {
-  const MemRegion *sReg = nullptr;
+const MemRegion* MemRegionManager::getMemRegionGloballyStored(const VarDecl 
*D) {
+  assert(D->hasGlobalStorage());

a.sidorin wrote:
> `get[Global/StaticLocal]MemSpaceForVariable`?
Ahh, that might make more sense. I did this refactoring without any sense of 
context, as the unnecessary complexity was a hindrance thereto.



Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:769
@@ -768,4 +768,3 @@
 
-const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
-const LocationContext *LC) {
-  const MemRegion *sReg = nullptr;
+const MemRegion* MemRegionManager::getMemRegionGloballyStored(const VarDecl 
*D) {
+  assert(D->hasGlobalStorage());

ariccio wrote:
> a.sidorin wrote:
> > `get[Global/StaticLocal]MemSpaceForVariable`?
> Ahh, that might make more sense. I did this refactoring without any sense of 
> context, as the unnecessary complexity was a hindrance thereto.
> 
How about `getGlobalMemSpaceForGlobalVariable`? I'm hesitant to drop the second 
global, to be clear about the scope of the variable that's stored globally. Is 
that a reasonable concern, or is that redundant?


Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:843
@@ +842,3 @@
+const LocationContext *LC) {
+  const MemRegion *sReg = nullptr;
+

a.sidorin wrote:
> `const MemSpaceRegion *StorageSpace?`
Same question as above: Would that change their behavior functionally?


(if not, then I'll happily change it)


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio marked 9 inline comments as done.
ariccio added a comment.

Whoops, I didn't //submit// my comments.



Comment at: 
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1185
@@ +1184,3 @@
+  /// getMemRegionGloballyStored - Retrieve or create the memory region
+  ///  associated with a specified globally stored, non-static local, VarDecl.
+  const MemRegion *getMemRegionGloballyStored(const VarDecl *D);

dcoughlin wrote:
> I think describing this as taking a "globally stored, non-static local, 
> VarDecl" is ambiguous. This method operates on global variables. It does not 
> act on local variables (static or otherwise). How about "Retrieve or create 
> the memory region associated with a VarDecl for a global variable."
> 
> Also, the recommended style these days is to not prefix the doc comment with 
> the name of the member, so I would remove "getMemRegionGloballyStored - " 
> even though getVarRegion() has the same thing. The doc comment style in this 
> file is sufficiently mismatched that I think it is better to do the 
> now-recommend thing rather than try to match its surrounding context.
I like that. Done.


Comment at: 
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1186
@@ +1185,3 @@
+  ///  associated with a specified globally stored, non-static local, VarDecl.
+  const MemRegion *getMemRegionGloballyStored(const VarDecl *D);
+

a.sidorin wrote:
> ariccio wrote:
> > a.sidorin wrote:
> > > How about make these helper functions return `const MemSpaceRegion *` to 
> > > make their signatures more meaningful?
> > Would that change their behavior functionally?
> No, it will not change it. You will need to change `static_cast` type to 
> `const MemSpaceRegion *`, however (lines 809-810). There is `const 
> MemSpaceRegion *` and its children classes everywhere in return statements 
> already.
What're the `static_cast`s for anyways? Shouldn't the `const 
StackArgumentsSpaceRegion*`s convert to `const MemRegion*` automatically?

`StackArgumentsSpaceRegion` derives from `StackSpaceRegion`:

`class StackArgumentsSpaceRegion : public StackSpaceRegion`

...`StackSpaceRegion` derives from `MemSpaceRegion`:

`class StackSpaceRegion : public MemSpaceRegion`

...and `MemSpaceRegion` derives from `MemRegion`:

`class MemSpaceRegion : public MemRegion`

...which I think should work, because of the kooky rules of covariant return 
types (or something)?


Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:769
@@ -768,4 +768,3 @@
 
-const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
-const LocationContext *LC) {
-  const MemRegion *sReg = nullptr;
+const MemRegion* MemRegionManager::getMemRegionGloballyStored(const VarDecl 
*D) {
+  assert(D->hasGlobalStorage());

ariccio wrote:
> ariccio wrote:
> > dcoughlin wrote:
> > > a.sidorin wrote:
> > > > `get[Global/StaticLocal]MemSpaceForVariable`?
> > > The rest of the method in class follow a pattern of getAdjectiveNoun, so 
> > > I would suggest something like getGlobalVarRegion() and 
> > > getLocalVarRegion()
> > Ahh, that might make more sense. I did this refactoring without any sense 
> > of context, as the unnecessary complexity was a hindrance thereto.
> > 
> How about `getGlobalMemSpaceForGlobalVariable`? I'm hesitant to drop the 
> second global, to be clear about the scope of the variable that's stored 
> globally. Is that a reasonable concern, or is that redundant?
Whatever. I've changed the global one to `getGlobalVarRegion`.


Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ -783,12 +791,3 @@
 
-// Treat other globals as GlobalInternal unless they are constants.
-} else {
-  QualType GQT = D->getType();
-  const Type *GT = GQT.getTypePtrOrNull();
-  // TODO: We could walk the complex types here and see if everything is
-  // constified.
-  if (GT && GQT.isConstQualified() && GT->isArithmeticType())
-sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-  else
-sReg = getGlobalsRegion();
-}
+const MemRegion* MemRegionManager::getMemRegionStaticLocal(const VarDecl *D, 
const LocationContext *LC) {
+  // FIXME: Once we implement scope handling, we will need to properly lookup

dcoughlin wrote:
> It looks to me like this function operates on both locals *and* static 
> locals. I would change the name to reflect that.
So, `getMemRegionStaticLocalOrLocal`?


Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:800
@@ -803,1 +799,3 @@
+  if (V.is())
+return V.get();
 

a.sidorin wrote:
> Oops.
> Lines above should stay in the caller function or be refactored. Otherwise, 
> we'll get the subregion of `V.get directly. This looks like a behaviour change. (And this i

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio marked 11 inline comments as done.
ariccio added a comment.

Marked some inline comments as done.


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Bump?



Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ -783,12 +791,3 @@
 
-// Treat other globals as GlobalInternal unless they are constants.
-} else {
-  QualType GQT = D->getType();
-  const Type *GT = GQT.getTypePtrOrNull();
-  // TODO: We could walk the complex types here and see if everything is
-  // constified.
-  if (GT && GQT.isConstQualified() && GT->isArithmeticType())
-sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-  else
-sReg = getGlobalsRegion();
-}
+const MemRegion* MemRegionManager::getMemRegionStaticLocal(const VarDecl *D, 
const LocationContext *LC) {
+  // FIXME: Once we implement scope handling, we will need to properly lookup

ariccio wrote:
> dcoughlin wrote:
> > It looks to me like this function operates on both locals *and* static 
> > locals. I would change the name to reflect that.
> So, `getMemRegionStaticLocalOrLocal`?
Bump?


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio marked 3 inline comments as done.
ariccio added a comment.

Alrighty, will update the diff momentarily.


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 47161.
ariccio added a comment.

Responded to comments (fixed the bug noticed in review), and changed names.


http://reviews.llvm.org/D16873

Files:
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===
--- llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1180,6 +1180,16 @@
   ///  a specified VarDecl and LocationContext.
   const VarRegion* getVarRegion(const VarDecl *D, const LocationContext *LC);
 
+private:
+  /// Retrieve or create the memory region
+  ///  associated with a VarDecl for a global variable.
+  const MemRegion *getMemSpaceForGlobalVariable(const VarDecl *D);
+
+  /// Retrieve or create the memory region
+  ///  associated with a specified VarDecl and LocationContext.
+  const MemRegion *getMemSpaceForLocalVariable(const VarDecl *D, llvm::PointerUnion &V);
+
+public:
   /// getVarRegion - Retrieve or create the memory region associated with
   ///  a specified VarDecl and super region.
   const VarRegion* getVarRegion(const VarDecl *D, const MemRegion *superR);
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -766,34 +766,76 @@
   return (const StackFrameContext *)nullptr;
 }
 
+const MemRegion* MemRegionManager::getMemSpaceForGlobalVariable(const VarDecl *D) {
+  assert(D->hasGlobalStorage());
+  assert(!D->isStaticLocal());
+  // First handle the globals defined in system headers.
+  if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
+// Whitelist the system globals which often DO GET modified, assume the
+// rest are immutable.
+if (D->getName().find("errno") != StringRef::npos)
+  return getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
+
+return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+  }
+  // Treat other globals as GlobalInternal unless they are constants.
+  QualType GQT = D->getType();
+  const Type *GT = GQT.getTypePtrOrNull();
+  // TODO: We could walk the complex types here and see if everything is
+  // constified.
+  if (GT && GQT.isConstQualified() && GT->isArithmeticType())
+return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+
+  return getGlobalsRegion();
+}
+
+const MemRegion* MemRegionManager::getMemSpaceForLocalVariable(const VarDecl *D, llvm::PointerUnion &V) {
+  const StackFrameContext *STC = V.get();
+
+  if (!STC)
+return getUnknownRegion();
+
+  if (D->hasLocalStorage()) {
+return (isa(D) || isa(D)
+   ? static_cast(getStackArgumentsRegion(STC))
+   : static_cast(getStackLocalsRegion(STC)));
+  }
+  assert(D->isStaticLocal());
+  const Decl *STCD = STC->getDecl();
+  if (isa(STCD) || isa(STCD))
+return getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
+getFunctionCodeRegion(cast(STCD)));
+
+  else if (const BlockDecl *BD = dyn_cast(STCD)) {
+// FIXME: The fallback type here is totally bogus -- though it should
+// never be queried, it will prevent uniquing with the real
+// BlockCodeRegion. Ideally we'd fix the AST so that we always had a
+// signature.
+QualType T;
+if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
+  T = TSI->getType();
+if (T.isNull())
+  T = getContext().VoidTy;
+if (!T->getAs())
+  T = getContext().getFunctionNoProtoType(T);
+T = getContext().getBlockPointerType(T);
+
+const BlockCodeRegion *BTR =
+  getBlockCodeRegion(BD, C.getCanonicalType(T),
+ STC->getAnalysisDeclContext());
+return getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
+BTR);
+  }
+  return getGlobalsRegion();
+}
+
 const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
 const LocationContext *LC) {
   const MemRegion *sReg = nullptr;
 
   if (D->hasGlobalStorage() && !D->isStaticLocal()) {
+sReg = getMemSpaceForGlobalVariable(D);
 
-// First handle the globals defined in system headers.
-if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
-  // Whitelist the system globals which often DO GET modified, assume the
-  // rest are immutable.
-  if (D->getName().find("errno") != StringRef::npos)
-sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
-  else
-sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-
-// Treat other globals as GlobalInternal unless they are constants.
-} el

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio added a comment.

All clear. Ready for landing?


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio added a comment.

One of two comments addressed, one question asked.



Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ +791,3 @@
+
+const MemRegion* MemRegionManager::getMemSpaceForLocalVariable(const VarDecl 
*D, llvm::PointerUnion &V) {
+  const StackFrameContext *STC = V.get();

a.sidorin wrote:
> That needs to be formatted. Also, you may pass `const StackFrameContext *` 
> instead of `PointerUnion` (as Devin pointed) because it is only used once to 
> get SFC.
> And I still think that making these function return `const MemSpaceRegion *` 
> is a good idea.
If I change it to a `MemSpaceRegion*`, do I need to change the `static_cast`s, 
from:
```
   ? static_cast(getStackArgumentsRegion(STC))
   : static_cast(getStackLocalsRegion(STC)));

```

to:
```
   ? static_cast(getStackArgumentsRegion(STC))
   : static_cast(getStackLocalsRegion(STC)));

```
...?

I'm still not quite sure what they're for in the first place?


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 47289.

http://reviews.llvm.org/D16873

Files:
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===
--- llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1180,6 +1180,17 @@
   ///  a specified VarDecl and LocationContext.
   const VarRegion* getVarRegion(const VarDecl *D, const LocationContext *LC);
 
+private:
+  /// Retrieve or create the memory region
+  ///  associated with a VarDecl for a global variable.
+  const MemRegion *getMemSpaceForGlobalVariable(const VarDecl *D);
+
+  /// Retrieve or create the memory region
+  ///  associated with a specified VarDecl and LocationContext.
+  const MemRegion *getMemSpaceForLocalVariable(const VarDecl *D,
+   const StackFrameContext *STC);
+
+public:
   /// getVarRegion - Retrieve or create the memory region associated with
   ///  a specified VarDecl and super region.
   const VarRegion* getVarRegion(const VarDecl *D, const MemRegion *superR);
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -766,34 +766,77 @@
   return (const StackFrameContext *)nullptr;
 }
 
+const MemRegion* MemRegionManager::getMemSpaceForGlobalVariable(const VarDecl *D) {
+  assert(D->hasGlobalStorage());
+  assert(!D->isStaticLocal());
+  // First handle the globals defined in system headers.
+  if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
+// Whitelist the system globals which often DO GET modified, assume the
+// rest are immutable.
+if (D->getName().find("errno") != StringRef::npos)
+  return getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
+
+return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+  }
+  // Treat other globals as GlobalInternal unless they are constants.
+  QualType GQT = D->getType();
+  const Type *GT = GQT.getTypePtrOrNull();
+  // TODO: We could walk the complex types here and see if everything is
+  // constified.
+  if (GT && GQT.isConstQualified() && GT->isArithmeticType())
+return getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
+
+  return getGlobalsRegion();
+}
+
+const MemRegion*
+MemRegionManager::getMemSpaceForLocalVariable(const VarDecl *D,
+  const StackFrameContext *STC) {
+
+  if (!STC)
+return getUnknownRegion();
+
+  if (D->hasLocalStorage()) {
+return (isa(D) || isa(D)
+   ? static_cast(getStackArgumentsRegion(STC))
+   : static_cast(getStackLocalsRegion(STC)));
+  }
+  assert(D->isStaticLocal());
+  const Decl *STCD = STC->getDecl();
+  if (isa(STCD) || isa(STCD))
+return getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
+getFunctionCodeRegion(cast(STCD)));
+
+  else if (const BlockDecl *BD = dyn_cast(STCD)) {
+// FIXME: The fallback type here is totally bogus -- though it should
+// never be queried, it will prevent uniquing with the real
+// BlockCodeRegion. Ideally we'd fix the AST so that we always had a
+// signature.
+QualType T;
+if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten())
+  T = TSI->getType();
+if (T.isNull())
+  T = getContext().VoidTy;
+if (!T->getAs())
+  T = getContext().getFunctionNoProtoType(T);
+T = getContext().getBlockPointerType(T);
+
+const BlockCodeRegion *BTR =
+  getBlockCodeRegion(BD, C.getCanonicalType(T),
+ STC->getAnalysisDeclContext());
+return getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind,
+BTR);
+  }
+  return getGlobalsRegion();
+}
+
 const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
 const LocationContext *LC) {
   const MemRegion *sReg = nullptr;
 
   if (D->hasGlobalStorage() && !D->isStaticLocal()) {
+sReg = getMemSpaceForGlobalVariable(D);
 
-// First handle the globals defined in system headers.
-if (C.getSourceManager().isInSystemHeader(D->getLocation())) {
-  // Whitelist the system globals which often DO GET modified, assume the
-  // rest are immutable.
-  if (D->getName().find("errno") != StringRef::npos)
-sReg = getGlobalsRegion(MemRegion::GlobalSystemSpaceRegionKind);
-  else
-sReg = getGlobalsRegion(MemRegion::GlobalImmutableSpaceRegionKind);
-
-// Treat other globals as GlobalInternal unless they are constants.
-} else {
-  QualType GQT = D->getTyp

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio marked 2 inline comments as done.
ariccio added a comment.

I've reformatted, and changed the `PointerUnion` to a `const 
StackFrameContext*`.

Built successfully, etc...

(I hope I don't sound too pushy!)



Comment at: llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp:792
@@ +791,3 @@
+
+const MemRegion*
+MemRegionManager::getMemSpaceForLocalVariable(const VarDecl *D,

I'm sorry, I think you're right, but I wanna get this patch over with, so I can 
move on to other things. Some other time, maybe?


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17130: Debloat some headers

[Alexander Riccio via cfe-commits]

ariccio created this revision.
ariccio added a subscriber: cfe-commits.

As discussed in "Code in headers" on llvm-dev, there are lots of headers with 
complex code in them. I've moved some complex constructors & destructors to 
implementation files, using [[ 
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts | Chromium's 
C++ Dos and Don'ts ]] as a guide.

It's compiling right now, and there're a few errors. I'll update the diff in a 
few minutes.

These changes are mostly in the Static Analyzer, as some files there take more 
than 30 seconds to compile.

http://reviews.llvm.org/D17130

Files:
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/SVals.cpp

Index: llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2680,6 +2680,30 @@
   delete interestingRegions.pop_back_val();
 }
 
+BugReport::BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode)
+  : BT(bt), DeclWithIssue(nullptr), Description(desc), ErrorNode(errornode),
+ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+
+BugReport::BugReport(BugType& bt, StringRef shortDesc, StringRef desc,
+  const ExplodedNode *errornode)
+  : BT(bt), DeclWithIssue(nullptr), ShortDescription(shortDesc),
+Description(desc), ErrorNode(errornode), ConfigurationChangeToken(0),
+DoNotPrunePath(false) {}
+
+BugReport::BugReport(BugType &bt, StringRef desc, PathDiagnosticLocation l)
+  : BT(bt), DeclWithIssue(nullptr), Description(desc), Location(l),
+ErrorNode(nullptr), ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+
+BugReport::BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode,
+  PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique)
+  : BT(bt), DeclWithIssue(nullptr), Description(desc),
+UniqueingLocation(LocationToUnique),
+UniqueingDecl(DeclToUnique),
+ErrorNode(errornode), ConfigurationChangeToken(0),
+DoNotPrunePath(false) {}
+
+
+
 const Stmt *BugReport::getStmt() const {
   if (!ErrorNode)
 return nullptr;
Index: llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -143,19 +143,12 @@
   void popInterestingSymbolsAndRegions();
 
 public:
-  BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode)
-: BT(bt), DeclWithIssue(nullptr), Description(desc), ErrorNode(errornode),
-  ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+  BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode);
 
   BugReport(BugType& bt, StringRef shortDesc, StringRef desc,
-const ExplodedNode *errornode)
-: BT(bt), DeclWithIssue(nullptr), ShortDescription(shortDesc),
-  Description(desc), ErrorNode(errornode), ConfigurationChangeToken(0),
-  DoNotPrunePath(false) {}
+const ExplodedNode *errornode);
 
-  BugReport(BugType &bt, StringRef desc, PathDiagnosticLocation l)
-: BT(bt), DeclWithIssue(nullptr), Description(desc), Location(l),
-  ErrorNode(nullptr), ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+  BugReport(BugType &bt, StringRef desc, PathDiagnosticLocation l);
 
   /// \brief Create a BugReport with a custom uniqueing location.
   ///
@@ -165,12 +158,7 @@
   /// for uniquing reports. For example, memory leaks checker, could set this to
   /// the allocation site, rather then the location where the bug is reported.
   BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode,
-PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique)
-: BT(bt), DeclWithIssue(nullptr), Description(desc),
-  UniqueingLocation(LocationToUnique),
-  UniqueingDecl(DeclToUnique),
-  ErrorNode(errornode), ConfigurationChangeToken(0),
-  DoNotPrunePath(false) {}
+PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique);
 
   virtual ~BugReport();
 
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -160,6 +160

Re: [PATCH] D17130: Debloat some headers

[Alexander Riccio via cfe-commits]

ariccio updated the summary for this revision.
ariccio updated this revision to Diff 47603.

http://reviews.llvm.org/D17130

Files:
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
  llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
  llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/SVals.cpp

Index: llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2680,6 +2680,30 @@
   delete interestingRegions.pop_back_val();
 }
 
+BugReport::BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode)
+  : BT(bt), DeclWithIssue(nullptr), Description(desc), ErrorNode(errornode),
+ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+
+BugReport::BugReport(BugType& bt, StringRef shortDesc, StringRef desc,
+  const ExplodedNode *errornode)
+  : BT(bt), DeclWithIssue(nullptr), ShortDescription(shortDesc),
+Description(desc), ErrorNode(errornode), ConfigurationChangeToken(0),
+DoNotPrunePath(false) {}
+
+BugReport::BugReport(BugType &bt, StringRef desc, PathDiagnosticLocation l)
+  : BT(bt), DeclWithIssue(nullptr), Description(desc), Location(l),
+ErrorNode(nullptr), ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+
+BugReport::BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode,
+  PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique)
+  : BT(bt), DeclWithIssue(nullptr), Description(desc),
+UniqueingLocation(LocationToUnique),
+UniqueingDecl(DeclToUnique),
+ErrorNode(errornode), ConfigurationChangeToken(0),
+DoNotPrunePath(false) {}
+
+
+
 const Stmt *BugReport::getStmt() const {
   if (!ErrorNode)
 return nullptr;
Index: llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -143,19 +143,12 @@
   void popInterestingSymbolsAndRegions();
 
 public:
-  BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode)
-: BT(bt), DeclWithIssue(nullptr), Description(desc), ErrorNode(errornode),
-  ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+  BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode);
 
   BugReport(BugType& bt, StringRef shortDesc, StringRef desc,
-const ExplodedNode *errornode)
-: BT(bt), DeclWithIssue(nullptr), ShortDescription(shortDesc),
-  Description(desc), ErrorNode(errornode), ConfigurationChangeToken(0),
-  DoNotPrunePath(false) {}
+const ExplodedNode *errornode);
 
-  BugReport(BugType &bt, StringRef desc, PathDiagnosticLocation l)
-: BT(bt), DeclWithIssue(nullptr), Description(desc), Location(l),
-  ErrorNode(nullptr), ConfigurationChangeToken(0), DoNotPrunePath(false) {}
+  BugReport(BugType &bt, StringRef desc, PathDiagnosticLocation l);
 
   /// \brief Create a BugReport with a custom uniqueing location.
   ///
@@ -165,12 +158,7 @@
   /// for uniquing reports. For example, memory leaks checker, could set this to
   /// the allocation site, rather then the location where the bug is reported.
   BugReport(BugType& bt, StringRef desc, const ExplodedNode *errornode,
-PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique)
-: BT(bt), DeclWithIssue(nullptr), Description(desc),
-  UniqueingLocation(LocationToUnique),
-  UniqueingDecl(DeclToUnique),
-  ErrorNode(errornode), ConfigurationChangeToken(0),
-  DoNotPrunePath(false) {}
+PathDiagnosticLocation LocationToUnique, const Decl *DeclToUnique);
 
   virtual ~BugReport();
 
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -160,6 +160,9 @@
 //===--===//
 // Core analysis engine.
 //===--===//
+CoreEngine::CoreEngine(SubEngine &subengine, FunctionSummariesTy *FS)
+: SubEng(subengine), WList(WorkList::makeDFS()),
+  BCounterFactory(G.getAllocator()), FunctionSummaries(FS) {}
 
 /// ExecuteWorkList - Run the worklist algorithm for a maximum number of steps.
 bool CoreEngine

Re: [PATCH] D17130: Debloat some headers

[Alexander Riccio via cfe-commits]

ariccio added inline comments.


Comment at: 
llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:143
@@ -144,3 +142,3 @@
   void addAbortedBlock(const ExplodedNode *node, const CFGBlock *block) {
-blocksAborted.push_back(std::make_pair(block, node));
+blocksAborted.emplace_back(block, node);
   }

I did this as a drive by "fix", I thought it was a tad bit simpler than 
explicitly calling `make_pair`. I can change it back if you'd like.


http://reviews.llvm.org/D17130



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17130: Debloat some headers

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17130#349744, @craig.topper wrote:

> What's complex about the SVal constructors?


I arbitrarily figured that classes that are more than twice-derived (is there a 
better way to say that) are complex. I don't think there was any 
//particularly// good reason there.


http://reviews.llvm.org/D17130



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17253: Cleanup of analyzer scripts as suggested by pychecker and pep8

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Whoops, forgot to submit my earlier comments. Responding to comments right 
now...



Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/CmpRuns.py:31
@@ -30,3 +30,3 @@
 import plistlib
-import CmpRuns
+import CmpRuns  # ?
 

This file imports itself?

I wasn't sure if I could eliminate this.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:62
@@ -60,3 +61,3 @@
 
 def detectCPUs():
 """

Should this be replaced with `multiprocessing.cpu_count`?


http://reviews.llvm.org/D17253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17253: Cleanup of analyzer scripts as suggested by pychecker and pep8

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17253#353000, @zaks.anna wrote:

> Not sure how you got these changes, but some of them seem wrong and some seem 
> inconsistently applied.
>
> Has this been tested?


Not yet, I was toying with running the first SARD test under it, and read 
through the code to get an idea of how it works. I remembered that I had 
PyChecker and pep8 installed, and ran them.

Line length fixes are inconsistently applied. I figured that I'd deal with the 
worst right now, and deal with the rest later. I can fix all of them in a 
revised patch if you'd like.

...but are there any functional changes? I didn't intend to make any here.

PyChecker produced the following output for `SATestBuild.py`:

  C:\Users\Alexander Riccio\Downloads>C:\Python27\python.exe  
C:\Python27\Lib\site-packages\pychecker\checker.py --limit 1000 -Q 1 -P 
SATestBuild.py
  [system path]\subprocess.py:406: No doc string for function __init__
  [system path]\subprocess.py:410: No doc string for function __str__
  
  SATestBuild.py:67: No module attribute (sysconf_names) found
  SATestBuild.py:69: No module attribute (sysconf) found
  SATestBuild.py:73: No global (capture) found
  SATestBuild.py:114: No doc string for class flushfile
  SATestBuild.py:114: No doc string for function __init__
  SATestBuild.py:116: No doc string for function write
  SATestBuild.py:122: No doc string for function getProjectMapPath
  SATestBuild.py:131: No doc string for function getProjectDir
  SATestBuild.py:134: No doc string for function getSBOutputDirName
  SATestBuild.py:135: Comparisons with True are not necessary and may not work 
as expected
  SATestBuild.py:205: No doc string for function runCleanupScript
  SATestBuild.py:211: No doc string for function runDownloadScript
  SATestBuild.py:216: No doc string for function runScript
  SATestBuild.py:234: No doc string for function downloadAndPatch
  SATestBuild.py:255: No doc string for function applyPatch
  SATestBuild.py:275: No doc string for function runScanBuild
  SATestBuild.py:319: No doc string for function hasNoExtension
  SATestBuild.py:325: No doc string for function isValidSingleInputFile
  SATestBuild.py:335: No doc string for function getSDKPath
  SATestBuild.py:343: No doc string for function runAnalyzePreprocessed
  SATestBuild.py:374: Comparisons with False are not necessary and may not work 
as expected
  SATestBuild.py:397: Comparisons with False are not necessary and may not work 
as expected
  SATestBuild.py:400: No doc string for function getBuildLogPath
  SATestBuild.py:403: No doc string for function removeLogFile
  SATestBuild.py:412: No doc string for function buildProject
  SATestBuild.py:469: No doc string for function CleanUpEmptyPlists
  SATestBuild.py:482: No doc string for function checkBuild
  SATestBuild.py:506: Local variable (FailuresCopied) not used
  SATestBuild.py:526: No doc string for class Discarder
  SATestBuild.py:526: No doc string for function write
  SATestBuild.py:534: No doc string for function runCmpResults
  SATestBuild.py:595: No doc string for function cleanupReferenceResults
  SATestBuild.py:608: No doc string for function updateSVN
  SATestBuild.py:637: No doc string for function testProject
  SATestBuild.py:655: Comparisons with False are not necessary and may not work 
as expected
  SATestBuild.py:663: No doc string for function testAll
  SATestBuild.py:678: Comparisons with True are not necessary and may not work 
as expected
  SATestBuild.py:679: Comparisons with True are not necessary and may not work 
as expected
  SATestBuild.py:688: Comparisons with True are not necessary and may not work 
as expected

`pep8` produced some noisier output for `SATestBuild.py`:

  C:\Users\Alexander Riccio\Downloads>C:\Python27\Scripts\pep8 --ignore=E111 
SATestBuild.py
  SATestBuild.py:6:80: E501 line too long (80 > 79 characters)
  SATestBuild.py:23:80: E501 line too long (81 > 79 characters)
  SATestBuild.py:57:1: E265 block comment should start with '# '
  SATestBuild.py:59:1: E265 block comment should start with '# '
  SATestBuild.py:61:1: E302 expected 2 blank lines, found 1
  SATestBuild.py:67:28: W601 .has_key() is deprecated, use 'in'
  SATestBuild.py:72:14: E261 at least two spaces before inline comment
  SATestBuild.py:75:18: W601 .has_key() is deprecated, use 'in'
  SATestBuild.py:79:13: E261 at least two spaces before inline comment
  SATestBuild.py:81:1: E302 expected 2 blank lines, found 1
  SATestBuild.py:81:25: E251 unexpected spaces around keyword / parameter equals
  SATestBuild.py:81:27: E251 unexpected spaces around keyword / parameter equals
  SATestBuild.py:86:37: E231 missing whitespace after ','
  SATestBuild.py:113:1: E302 expected 2 blank lines, found 1
  SATestBuild.py:116:5: E301 expected 1 blank line, found 0
  SATestBuild.py:122:1: E302 expected 2 blank lines, found 1
  SATestBuild.py:127:17: E126 continuation line over-indented for hanging indent
  SATestBuild.py:131:1: E302 expected 2 blank l

Re: [PATCH] D17253: Cleanup of analyzer scripts as suggested by pychecker and pep8

[Alexander Riccio via cfe-commits]

ariccio added inline comments.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/CmpRuns.py:194
@@ -182,3 +193,3 @@
 # Backward compatibility API.
-def loadResults(path, opts, root = "", deleteEmpty=True):
+def loadResults(path, opts, root="", deleteEmpty=True):
 return loadResultsFromSingleRun(SingleRunInfo(path, root, opts.verboseLog),

zaks.anna wrote:
> PEP 0008: Don't use spaces around the = sign when used to indicate a keyword 
> argument or a default parameter value.
> 
> 
...agreed? Huh?


http://reviews.llvm.org/D17253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17253: Cleanup of analyzer scripts as suggested by pychecker and pep8

[Alexander Riccio via cfe-commits]

ariccio added a comment.

I've added a few comments where I think the changes are not quite clear.



Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/CmpRuns.py:159
@@ -147,3 +158,3 @@
 if 'clang_version' in data:
-if self.clang_version == None:
+if self.clang_version is None:
 self.clang_version = data.pop('clang_version')

This was to fix `E711 comparison to None should be 'if cond is None:'`.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:68
@@ -66,3 +67,3 @@
 if hasattr(os, "sysconf"):
-if os.sysconf_names.has_key("SC_NPROCESSORS_ONLN"):
+if "SC_NPROCESSORS_ONLN" in os.sysconf_names:
 # Linux & Unix:

This was to fix `W601 .has_key() is deprecated, use 'in'`.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:317
@@ -305,3 +316,3 @@
 if (Command.startswith("make ") or Command == "make") and \
-"-j" not in Command:
+"-j" not in Command:
 Command += " -j%d" % Jobs

This funny looking change was to fix `E125 continuation line with same indent 
as next logical line`.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:341
@@ -330,1 +340,3 @@
+(Ext == ".c") or (Ext == ".cpp") or
+(Ext == ".m") or (Ext == "")):
 return True

I replaced `|` with `or` here because I'm fairly certain we want the 
//logical// `or`, not the //bitwise// `or`.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:410
@@ -396,3 +409,3 @@
 # If command did not fail, erase the log file.
-if Failed == False:
-os.remove(LogFile.name);
+if not Failed:
+os.remove(LogFile.name)

This was to fix `SATestBuild.py:397:19: E712 comparison to False should be 'if 
cond is False:' or 'if not cond:'`


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:471
@@ -454,4 +470,3 @@
 PathPrefix = os.path.join(Dir, PatchedSourceDirName)
-Paths = [SourceFile[len(PathPrefix)+1:]\
-  if SourceFile.startswith(PathPrefix)\
-  else SourceFile for SourceFile in Data['files']]
+Paths = [SourceFile[len(PathPrefix) + 1:]
+ if SourceFile.startswith(PathPrefix)

This was to fix `E502 the backslash is redundant between brackets`.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:679
@@ -654,3 +678,3 @@
 
-if IsReferenceBuild == False:
+if not IsReferenceBuild:
 runCmpResults(Dir, Strictness)

This was also to fix `E712 comparison to False should be 'if cond is False:' or 
'if not cond:'`.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:703
@@ -677,4 +702,3 @@
 # update svn. Remove reference results from SVN.
-if UpdateSVN == True:
-assert(IsReferenceBuild == True);
-updateSVN("delete",  PMapFile);
+if UpdateSVN:
+assert(IsReferenceBuild)

This was to fix `E712 comparison to True should be 'if cond is True:' or 'if 
cond:'`.


http://reviews.llvm.org/D17253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 55366.
ariccio added a comment.

Conditionally check Windows methods on Windows.

Is this what you're thinking of? It's kinda ugly - I was hoping to avoid the 
`#if`s - but it does only check the Windows functions on Windows builds only.


http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/alternative-malloc-api.c
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/alternative-malloc-api.c
===
--- llvm/tools/clang/test/Analysis/alternative-malloc-api.c
+++ llvm/tools/clang/test/Analysis/alternative-malloc-api.c
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
+void free(void *);
+
+char *tempnam(const char *dir, const char *pfx);
+
+void testTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+
+// What does "ContentIsDefined" refer to?
+void testTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+#if defined(_WIN32)
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+void testWinTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamDbgLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnamLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnaDbgmLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamDbgNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamDbgNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  wchar_t result = fileName[0];// no warning
+  free(fileName);
+}
+#endif //defined(_WIN32)
Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -32,11 +32,37 @@
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+
 /

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio marked an inline comment as done.
ariccio added a comment.

Wrongly `#if defined` out test is now fixed.


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 55774.
ariccio added a comment.

Only check for MSVC-specific functions when actually building FOR MSVC (i.e. 
`isWindowsMSVCEnvironment`).

Sidenote: is there any reason why none of the `ASTContext&`s are `const`ified 
in this file?


http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/alternative-malloc-api.c
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/alternative-malloc-api.c
===
--- llvm/tools/clang/test/Analysis/alternative-malloc-api.c
+++ llvm/tools/clang/test/Analysis/alternative-malloc-api.c
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
+void free(void *);
+
+char *tempnam(const char *dir, const char *pfx);
+
+void testTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+
+// What does "ContentIsDefined" refer to?
+void testTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+#if defined(_WIN32)
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+void testWinTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamDbgLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnamLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnaDbgmLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamDbgNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamDbgNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  wchar_t result = fileName[0];// no warning
+  free(fileName);
+}
+#endif //defined(_WIN32)
Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -32,11 +32,37 @@
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+
 // Wi

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

I'm sorry, I'm confused.

As an example, for `_wcsdup_dbg`, I've added `testWinWcsdupDbg`, 
`testWinWcsdupDbgContentIsDefined`, and `testWinWideDbgLeakWithinReturn`. Which 
ones should I drop for that API?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Here's my confusion: don't we //kinda// have to duplicate tests? After all 
these functions are //essentially// duplicate functions.


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

So for `_wcsdup_dbg`, I should leave only `testWinWcsdupDbg`?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Maybe I'm being thick headed and I can't see it - sorry if I am - but I'm still 
a bit confused. Can you tell me what I should do in the `_wcsdup_dbg` example?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote:

> > So for _wcsdup_dbg, I should leave only testWinWcsdupDbg?
>
>
> Yes.


Ok, that I can do. Will upload patch later this afternoon/tonight.

In http://reviews.llvm.org/D18073#392972, @zaks.anna wrote:

> Also, since there will be many of these "alternate" functions, could you 
> create a separate test file for them?


But, that contradicts removing them? Huh?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 52876.
ariccio added a comment.

Fewer added test functions.


http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -31,12 +31,50 @@
 wchar_t *wcsdup(const wchar_t *s);
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
+char *tempnam(const char *dir, const char *pfx);
 
+
 // Windows variants
 char *_strdup(const char *strSource);
 wchar_t *_wcsdup(const wchar_t *strSource);
+unsigned char *_mbsdup(const unsigned char *strSource);
+
 void *_alloca(size_t size);
 
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+
+void *_calloc_dbg(size_t num, size_t size, int blockType,
+  const char *filename, int linenumber);
+
+char *_strdup_dbg(const char *strSource, int blockType,
+  const char *filename, int linenumber);
+
+wchar_t *_wcsdup_dbg(const wchar_t *strSource, int blockType,
+ const char *filename, int linenumber);
+
+unsigned char *_mbsdup_dbg(const unsigned char *strSource, int blockType,
+   const char *filename, int linenumber);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+
+
+// Very frequently used debug versions
+void _free_dbg(void *userData, int blockType);
+void _malloc_dbg(size_t size, int blockType, const char *filename,
+ int linenumber);
+
+void *_realloc_dbg(void *userData, size_t newSize,
+   int blockType, const char *filename, int linenumber);
+
+
+
 void myfoo(int *p);
 void myfooint(int p);
 char *fooRetPtr();
@@ -291,25 +329,37 @@
 }
 
 void CheckUseZeroAllocated6() {
+  int *p = _calloc_dbg(0, 2, 0, __FILE__, __LINE__);
+  *p = 1; // expected-warning {{Use of zero-allocated memory}}
+  free(p);
+}
+
+void CheckUseZeroAllocated7() {
   int *p = calloc(2, 0);
   *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
-void CheckUseZeroAllocated7() {
+void CheckUseZeroAllocated8() {
+  int *p = _calloc_dbg(2, 0, 0, __FILE__, __LINE__);
+  *p = 1; // expected-warning {{Use of zero-allocated memory}}
+  free(p);
+}
+
+void CheckUseZeroAllocated9() {
   int *p = realloc(0, 0);
   *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
-void CheckUseZeroAllocated8() {
+void CheckUseZeroAllocated10() {
   int *p = malloc(8);
   int *q = realloc(p, 0);
   *q = 1; // expected-warning {{Use of zero-allocated memory}}
   free(q);
 }
 
-void CheckUseZeroAllocated9() {
+void CheckUseZeroAllocated11() {
   int *p = realloc(0, 0);
   int *q = realloc(p, 0);
   *q = 1; // expected-warning {{Use of zero-allocated memory}}
@@ -1126,6 +1176,26 @@
   s2[validIndex + 1] = 'b';
 } // expected-warning {{Potential leak of memory pointed to by}}
 
+void testWinMbsdup(const unsigned char *s, unsigned validIndex) {
+  unsigned char *s2 = _mbsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinMbsdupDbg(const unsigned char *s, unsigned validIndex) {
+  unsigned char *s2 = _mbsdup_dbg(s, 0, __FILE__, __LINE__);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testStrdupDbg(const char *s, unsigned validIndex) {
+  char *s2 = _strdup_dbg(s, 0, __FILE__, __LINE__);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWcsdupDbg(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup_dbg(s, 0, __FILE__, __LINE__);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
 int testStrndup(const char *s, unsigned validIndex, unsigned size) {
   char *s2 = strndup(s, size);
   s2 [validIndex + 1] = 'b';
@@ -1159,6 +1229,79 @@
   free(s2);
 }
 
+void testWinMbsdupContentIsDefined(const unsigned char *s, unsigned validIndex) {
+  unsigned char *s2 = _mbsdup(s);
+  unsigned char result = s2[1];// no warning
+  free(s2);
+}
+
+void testTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamDbgLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18073#393613, @zaks.anna wrote:

> You will have to add one test function to smoke test that the newly added API 
> is modeled correctly.


Isn't that what I've already done?

> We also have a lot of existing tests that verify that each of the original 
> APIs (malloc. free, new) function correctly in all possible settings.

>  What is the contradiction in asking to add the smoke tests in a separate 
> file?


Ahh, I was confused as to whether you wanted me to remove them entirely - 
moving them to a different file is self-consistent. I'm not exactly sure of the 
benefit of moving these tests to a separate file?

Sidenote: As someone who has worked a little bit with electronics and simple 
hardware design & construction, I love this use of "smoke test".


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 53193.
ariccio added a comment.

I'm not actually sure why I didn't want to split these off into a separate 
file, but now I finally have. Is this what you were looking for?


http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/malloc-uses.c
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/malloc-uses.c
===
--- llvm/tools/clang/test/Analysis/malloc-uses.c
+++ llvm/tools/clang/test/Analysis/malloc-uses.c
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
+void free(void *);
+
+char *tempnam(const char *dir, const char *pfx);
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+void testTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamDbgLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnamLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnaDbgmLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamDbgNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamDbgNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+// What does "ContentIsDefined" refer to?
+void testTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  wchar_t result = fileName[0];// no warning
+  free(fileName);
+}
Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -32,11 +32,38 @@
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+
 // Windows variants
 char *_strdup(const char *strSource);
 wchar_t *_wcsdup(const wchar_t *strSource);
+unsigned char *_mbsdup(const unsigned char *strSource);

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 53495.
ariccio added a comment.

Changed the new file name.


http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/alternative-malloc-api.c
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/alternative-malloc-api.c
===
--- llvm/tools/clang/test/Analysis/alternative-malloc-api.c
+++ llvm/tools/clang/test/Analysis/alternative-malloc-api.c
@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
+void free(void *);
+
+char *tempnam(const char *dir, const char *pfx);
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+void testTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamDbgLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnamLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnaDbgmLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamDbgNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamDbgNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+// What does "ContentIsDefined" refer to?
+void testTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  wchar_t result = fileName[0];// no warning
+  free(fileName);
+}
Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -32,11 +32,38 @@
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+
 // Windows variants
 char *_strdup(const char *strSource);
 wchar_t *_wcsdup(const wchar_t *strSource);
+unsigned char *_mbsdup(const unsigned char *strSource);
+
 void *_alloca(size_t size);
 
+
+
+void *_calloc_dbg(size_t num, si

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18073#398882, @zaks.anna wrote:

> "Since we are adding support for so many new APIs that are only available on 
> Windows, could you please condition checking them only when we build for 
> Windows. You probably can look and Language Options to figure that out."
>
> malloc-uses.c -> "alternative-malloc-api.c" ?


Sorry for being so thickheaded about that the first go-around, I'm not entirely 
sure what was up with me. I've changed the name of the file, but I haven't yet 
figured out how to set up the Windows only tests to run on Windows only. I'm 
assuming I can't simply:

  #if defined(_WIN32)

[...]

  #endif

...because `lit` doesn't define `_WIN32` for me. Which Language Options should 
I look in?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 53995.
ariccio added a comment.

So, duh, it turns out I //can// use `_WIN32` to conditionally test.


http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/alternative-malloc-api.c
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/alternative-malloc-api.c
===
--- llvm/tools/clang/test/Analysis/alternative-malloc-api.c
+++ llvm/tools/clang/test/Analysis/alternative-malloc-api.c
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator.h"
+
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
+void free(void *);
+
+char *tempnam(const char *dir, const char *pfx);
+
+void testTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+
+// What does "ContentIsDefined" refer to?
+void testTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+#if defined(_WIN32)
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+void testWinTempnamLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamDbgLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnamLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWideTempnaDbgmLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+}// expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinTempnamNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinTempnamDbgNoLeak(const char* dir, const char* prefix) {
+  char* fileName = _tempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  free(fileName);// no warning
+}
+
+void testWinWideTempnamDbgNoLeak(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam_dbg(dir, prefix, 0, __FILE__, __LINE__);
+  free(fileName);// no warning
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinTempnamNoLeakContentIsDefined(const char* dir, const char* prefix) {
+  char* fileName = _tempnam(dir, prefix);
+  char result = fileName[0];// no warning
+  free(fileName);
+}
+
+// What does "ContentIsDefined" refer to?
+void testWinWideTempnamNoLeakContentIsDefined(const wchar_t* dir, const wchar_t* prefix) {
+  wchar_t* fileName = _wtempnam(dir, prefix);
+  wchar_t result = fileName[0];// no warning
+  free(fileName);
+}
+#endif //defined(_WIN32)
Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -32,11 +32,37 @@
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+
 // Windows variants
+#if defined(_WIN32)
 char *_strdup(const char *strSource);
 wchar_t *_wcsdup(const wchar_t *strSource);
+unsigned cha

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

I should elaborate. The principle of operation of this latest patch is that the 
`FunctionDecl` in `IsCMemFunction` should never return a `nullptr` 
`IdentifierInfo*` from `getIdentifier` (is that a valid assumption?)... Thus, 
when`! isWindowsMSVCEnvironment`, I leave the Windows-only memory allocating 
functions initialized to `nullptr`, which will never equal a non-null 
`IdentifierInfo*`, and never trigger on a non-Windows platform.


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Bump?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17253: Cleanup of analyzer scripts as suggested by pychecker and pep8

[Alexander Riccio via cfe-commits]

ariccio added inline comments.


Comment at: C:/LLVM/llvm/tools/clang/utils/analyzer/SATestBuild.py:521
@@ -502,3 +520,3 @@
 SummaryLog.write("See the first %d below.\n"
-   % (NumOfFailuresInSummary,))
+ (NumOfFailuresInSummary,))
 # TODO: Add a line "See the results folder for more."

zaks.anna wrote:
> Is removing '%' correct here?
Nope. Good catch!


http://reviews.llvm.org/D17253



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio created this revision.
ariccio added a subscriber: cfe-commits.

I've found & fixed a leak that Clang misses when compiling on Windows.

The leak was found by [[ 
https://samate.nist.gov/SARD/view_testcase.php?tID=149071 | SARD #149071 ]], 
mem1-bad.c. Clang misses it because MSVC uses _strdup instead of strdup. I've 
added an IdentifierInfo* for _strdup, and now Clang catches it. 

As a drive by fix, I added the wide character strdup variants (wcsdup, _wcsdup) 
and the MSVC version of alloca (_alloca).

I will add reviewers when check-all finishes.

http://reviews.llvm.org/D17688

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), 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_if_nameindex(nullptr),
-II_if_freenameindex(nullptr) {}
+  : II_alloca(nullptr), II__alloca(nullptr), 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__strdup(nullptr), II_kmalloc(nullptr),
+II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
+II_wcsdup(nullptr), II__wcsdup(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
@@ -231,10 +232,11 @@
   mutable std::unique_ptr BT_MismatchedDealloc;
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_alloca, *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 IdentifierInfo *II_alloca, *II__alloca, *II_malloc, *II_free,
+ *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
+ *II_strndup, *II_strdup, *II__strdup, *II_kmalloc,
+ *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
+ *II__wcsdup;
   mutable Optional KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext &C) const;
@@ -540,9 +542,15 @@
   II_valloc = &Ctx.Idents.get("valloc");
   II_strdup = &Ctx.Idents.get("strdup");
   II_strndup = &Ctx.Idents.get("strndup");
+  II_wcsdup = &Ctx.Idents.get("wcsdup");
   II_kmalloc = &Ctx.Idents.get("kmalloc");
   II_if_nameindex = &Ctx.Idents.get("if_nameindex");
   II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
+  
+  //MSVC uses `_`-prefixed instead, so we check for them too.
+  II__strdup = &Ctx.Idents.get("_strdup");
+  II__wcsdup = &Ctx.Idents.get("_wcsdup");
+  II__alloca = &Ctx.Idents.get("_alloca");
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const 
{
@@ -585,7 +593,8 @@
 if (Family == AF_Malloc && CheckAlloc) {
   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)
+  FunI == II__strdup || FunI == II_strndup || FunI == II_wcsdup ||
+  FunI == II__wcsdup || FunI == II_kmalloc)
 return true;
 }
 
@@ -600,7 +609,7 @@
 }
 
 if (Family == AF_Alloca && CheckAlloc) {
-  if (FunI == II_alloca)
+  if (FunI == II_alloca || FunI == II__alloca)
 return true;
 }
   }
@@ -789,11 +798,12 @@
   State = ProcessZeroAllocation(C, CE, 1, State);
 } else if (FunI == II_free) {
   State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
-} else if (FunI == II_strdup) {
+} else if (FunI == II_strdup || FunI == II__strdup ||
+   FunI == II_wcsdup || FunI == II__wcsdup) {
   State = MallocUpdateRefState(C, CE, State);
 } else if (FunI == II_strndup) {
   State = MallocUpdateRefState(C, CE, State);
-} else if (FunI == II_alloca) {
+} else if (FunI == II_alloca || FunI == II__alloca) {
   State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
AF_Alloca);
   State = ProcessZeroAllocation(C, CE, 0, State);


Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ 

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#363835, @zaks.anna wrote:

> The two underscores in the names are hard to read. Maybe we should just 
> prefix them with 'win'?


I like that better, will change.


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 49330.
ariccio added a comment.

Changed underscore prefixed variable names to `win` prefixed variable names.


http://reviews.llvm.org/D17688

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), 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_if_nameindex(nullptr),
-II_if_freenameindex(nullptr) {}
+  : II_alloca(nullptr), II_winalloca(nullptr), 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_winstrdup(nullptr), II_kmalloc(nullptr),
+II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
+II_wcsdup(nullptr), II_winwcsdup(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
@@ -231,10 +232,11 @@
   mutable std::unique_ptr BT_MismatchedDealloc;
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_alloca, *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 IdentifierInfo *II_alloca, *II_winalloca, *II_malloc, *II_free,
+ *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
+ *II_strndup, *II_strdup, *II_winstrdup, *II_kmalloc,
+ *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
+ *II_winwcsdup;
   mutable Optional KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext &C) const;
@@ -540,9 +542,15 @@
   II_valloc = &Ctx.Idents.get("valloc");
   II_strdup = &Ctx.Idents.get("strdup");
   II_strndup = &Ctx.Idents.get("strndup");
+  II_wcsdup = &Ctx.Idents.get("wcsdup");
   II_kmalloc = &Ctx.Idents.get("kmalloc");
   II_if_nameindex = &Ctx.Idents.get("if_nameindex");
   II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
+  
+  //MSVC uses `_`-prefixed instead, so we check for them too.
+  II_winstrdup = &Ctx.Idents.get("_strdup");
+  II_winwcsdup = &Ctx.Idents.get("_wcsdup");
+  II_winalloca = &Ctx.Idents.get("_alloca");
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const 
{
@@ -585,7 +593,8 @@
 if (Family == AF_Malloc && CheckAlloc) {
   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)
+  FunI == II_winstrdup || FunI == II_strndup || FunI == II_wcsdup ||
+  FunI == II_winwcsdup || FunI == II_kmalloc)
 return true;
 }
 
@@ -600,7 +609,7 @@
 }
 
 if (Family == AF_Alloca && CheckAlloc) {
-  if (FunI == II_alloca)
+  if (FunI == II_alloca || FunI == II_winalloca)
 return true;
 }
   }
@@ -789,11 +798,12 @@
   State = ProcessZeroAllocation(C, CE, 1, State);
 } else if (FunI == II_free) {
   State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
-} else if (FunI == II_strdup) {
+} else if (FunI == II_strdup || FunI == II_winstrdup ||
+   FunI == II_wcsdup || FunI == II_winwcsdup) {
   State = MallocUpdateRefState(C, CE, State);
 } else if (FunI == II_strndup) {
   State = MallocUpdateRefState(C, CE, State);
-} else if (FunI == II_alloca) {
+} else if (FunI == II_alloca || FunI == II_winalloca) {
   State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
AF_Alloca);
   State = ProcessZeroAllocation(C, CE, 0, State);


Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), 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_if_nameindex(nullptr),
-II_if_freenameindex(nullptr) {}
+  : II_alloca(nullptr), 

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

//(This is mostly for my own reference)//
BTW, there are a few other non-MS-only functions in the C standard library that 
allocate memory that needs to be free()d:

1. getcwd (and _getcwd/_wgetcwd)

And some MS-specific functions that I'm surprised are actually MS-only:

1. _getdcwd/_wgetdcwd
2. _dupenv_s/_wdupenv_s
3. _fullpath/_wfullpath




http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 49456.
ariccio added a comment.

Nit addressed.


http://reviews.llvm.org/D17688

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), 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_if_nameindex(nullptr),
-II_if_freenameindex(nullptr) {}
+  : II_alloca(nullptr), II_win_alloca(nullptr), 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_win_strdup(nullptr), II_kmalloc(nullptr),
+II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
+II_wcsdup(nullptr), II_win_wcsdup(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
@@ -231,10 +232,11 @@
   mutable std::unique_ptr BT_MismatchedDealloc;
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_alloca, *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 IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc, *II_free,
+ *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
+ *II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc,
+ *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
+ *II_win_wcsdup;
   mutable Optional KernelZeroFlagVal;
 
   void initIdentifierInfo(ASTContext &C) const;
@@ -540,9 +542,15 @@
   II_valloc = &Ctx.Idents.get("valloc");
   II_strdup = &Ctx.Idents.get("strdup");
   II_strndup = &Ctx.Idents.get("strndup");
+  II_wcsdup = &Ctx.Idents.get("wcsdup");
   II_kmalloc = &Ctx.Idents.get("kmalloc");
   II_if_nameindex = &Ctx.Idents.get("if_nameindex");
   II_if_freenameindex = &Ctx.Idents.get("if_freenameindex");
+  
+  //MSVC uses `_`-prefixed instead, so we check for them too.
+  II_win_strdup = &Ctx.Idents.get("_strdup");
+  II_win_wcsdup = &Ctx.Idents.get("_wcsdup");
+  II_win_alloca = &Ctx.Idents.get("_alloca");
 }
 
 bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const 
{
@@ -585,7 +593,8 @@
 if (Family == AF_Malloc && CheckAlloc) {
   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)
+  FunI == II_win_strdup || FunI == II_strndup || FunI == II_wcsdup ||
+  FunI == II_win_wcsdup || FunI == II_kmalloc)
 return true;
 }
 
@@ -600,7 +609,7 @@
 }
 
 if (Family == AF_Alloca && CheckAlloc) {
-  if (FunI == II_alloca)
+  if (FunI == II_alloca || FunI == II_win_alloca)
 return true;
 }
   }
@@ -789,11 +798,12 @@
   State = ProcessZeroAllocation(C, CE, 1, State);
 } else if (FunI == II_free) {
   State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory);
-} else if (FunI == II_strdup) {
+} else if (FunI == II_strdup || FunI == II_win_strdup ||
+   FunI == II_wcsdup || FunI == II_win_wcsdup) {
   State = MallocUpdateRefState(C, CE, State);
 } else if (FunI == II_strndup) {
   State = MallocUpdateRefState(C, CE, State);
-} else if (FunI == II_alloca) {
+} else if (FunI == II_alloca || FunI == II_win_alloca) {
   State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State,
AF_Alloca);
   State = ProcessZeroAllocation(C, CE, 0, State);


Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), 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_if_nameindex(nullptr),
-II_if_freenameindex(nullptr) {}
+  : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),
+  

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Is this patch all clear to go?

I hope I don't sound too pushy - I just don't want to lose momentum here.


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D17688: Fix missed leak from MSVC specific allocationfunctions

[Alexander Riccio via cfe-commits]

> I just don't understand why some functions made it in this patch and not 
> others (notably, why the lack of _mbsdup, which is documented on the same 
> page as others you are adding).

It's partly because of the better wrapper class that was mentioned (in a 
discussion that I'm having trouble finding), and partly because I highly prefer 
locking in small changes rather than repeatedly growing patches. 

Personally, the more I grow a patch, the less likely it is that I'll ever 
actually get it accepted. I think this has a bit to do with using svn instead 
of git.

> By "here", do you mean in this review thread, or as part of this section of 
> code?

Whoops. I meant in this thread or an email, but I don't actually remembering 
why I asked this in the first place. I'll just whack some of the easiest ones 
in, and we'll go from there.

I'll need to separately implement the aligned versions, because they shouldn't 
be mixed with the normal versions.


sent from my (stupid) windows phone

-Original Message-
From: "Aaron Ballman" 
Sent: ‎3/‎1/‎2016 3:33 PM
To: "" 
Cc: "reviews+d17688+public+c9d6a65abf2d2...@reviews.llvm.org" 
; "Devin Coughlin" 
; "Anna Zaks" ; "cfe-commits" 

Subject: Re: [PATCH] D17688: Fix missed leak from MSVC specific 
allocationfunctions

On Tue, Mar 1, 2016 at 1:42 PM, 
 wrote:
> I'd quite happily add them... but can I do it in another patch? I think I
> could be more thorough that way.

I'm not certain I understand the reasoning, but I also don't have
strong feelings on whether it's this patch or another. I just don't
understand why some functions made it in this patch and not others
(notably, why the lack of _mbsdup, which is documented on the same
page as others you are adding).

> For the same reason, can we list all the microsoft memory allocating
> routines here? There are a thousand routines we might want to add, and then
> a few others (like _dupenv_s, _malloca, and _expand) which are especially
> important to be able to analyze (because they have really tricky APIs), but
> because of their kooky APIs, they're harder to implement checkers for.

By "here", do you mean in this review thread, or as part of this
section of code?

~Aaron

>
> Sincerely,
> Alexander Riccio
> --
> "Change the world or go home."
> about.me/ariccio
>
> If left to my own devices, I will build more.
> ⁂
>
> On Tue, Mar 1, 2016 at 8:38 AM, Aaron Ballman 
> wrote:
>>
>> On Tue, Mar 1, 2016 at 2:16 AM, Alexander Riccio 
>> wrote:
>> > ariccio updated this revision to Diff 49456.
>> > ariccio added a comment.
>> >
>> > Nit addressed.
>> >
>> >
>> > http://reviews.llvm.org/D17688
>> >
>> > Files:
>> >   llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> >
>> > Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> > ===
>> > --- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> > +++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
>> > @@ -169,11 +169,12 @@
>> >  {
>> >  public:
>> >MallocChecker()
>> > -  : II_alloca(nullptr), 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_if_nameindex(nullptr),
>> > -II_if_freenameindex(nullptr) {}
>> > +  : II_alloca(nullptr), II_win_alloca(nullptr), 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_win_strdup(nullptr),
>> > II_kmalloc(nullptr),
>> > +II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
>> > +II_wcsdup(nullptr), II_win_wcsdup(nullptr) {}
>> >
>> >/// In pessimistic mode, the checker assumes that it does not know
>> > which
>> >/// functions might free the memory.
>> > @@ -231,10 +232,11 @@
>> >mutable std::unique_ptr BT_MismatchedDealloc;
>> >mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
>> >mutable std::unique_ptr
>> > BT_UseZerroAllocated[CK_NumCheckKinds];
>> > -  mutable IdentifierInfo *II_alloca, *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 IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc,
>> > *II_free,
>> > + *II_realloc, *II_calloc, *II_valloc,
>> > *II_reallocf,
>> > + *II_strndup, *II_strdup, *II_win_strdup,
>> > *II_kmalloc,
>> > + *II_if_nameindex, *II_if_freenameindex,
>> > *II_wcsdup,
>> > + *II_win_wcsdup;
>> >mutable Optional KernelZeroFlagVal;
>> >
>> >void initIdentifierInfo(ASTConte

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#365880, @zaks.anna wrote:

> I suggest to update the malloc regression test with these.


Eh? There's a malloc regression test?


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#365951, @zaks.anna wrote:

> ls ./clang/test/Analysis/malloc*


Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the 
beginning:

  #if defined(_WIN32)
  
  #define strdup _strdup
  #define alloca _alloca
  
  #endif //defined(_WIN32)


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#366011, @ariccio wrote:

> In http://reviews.llvm.org/D17688#365951, @zaks.anna wrote:
>
> > ls ./clang/test/Analysis/malloc*
>
>
> Ah, ok. Would it be ok if (for _strdup & _alloca) I just do this at the 
> beginning:
>
>   #if defined(_WIN32)
>  
>   #define strdup _strdup
>   #define alloca _alloca
>  
>   #endif //defined(_WIN32)
>


Nevermind. I'm uploading a proper patch in the next few minutes.


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 49674.
ariccio added a comment.

Added newly checked variants to the malloc checks.

(Running the check-all suite now, will update with results)


http://reviews.llvm.org/D17688

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -13,9 +13,15 @@
 void *reallocf(void *ptr, size_t size);
 void *calloc(size_t nmemb, size_t size);
 char *strdup(const char *s);
+wchar_t *wcsdup(const wchar_t *s);
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+// Windows variants
+char *_strdup(const char *strSource);
+wchar_t *_wcsdup(const wchar_t *strSource);
+void *_alloca(size_t size);
+
 void myfoo(int *p);
 void myfooint(int p);
 char *fooRetPtr();
@@ -55,6 +61,10 @@
   int *p = alloca(sizeof(int));
 } // no warn
 
+void winAllocaTest() {
+  int *p = _alloca(sizeof(int));
+} // no warn
+
 void allocaBuiltinTest() {
   int *p = __builtin_alloca(sizeof(int));
 } // no warn
@@ -210,6 +220,11 @@
   int *p = alloca(0); // no warning
 }
 
+void CheckUseZeroWinAllocatedNoWarn2() {
+  int *p = _alloca(0); // no warning
+}
+
+
 void CheckUseZeroAllocatedNoWarn3() {
   int *p = malloc(0);
   int *q = realloc(p, 8); // no warning
@@ -233,6 +248,11 @@
   return *p; // expected-warning {{Use of zero-allocated memory}}
 }
 
+char CheckUseZeroWinAllocated2() {
+  char *p = _alloca(0);
+  return *p; // expected-warning {{Use of zero-allocated memory}}
+}
+
 void UseZeroAllocated(int *p) {
   if (p)
 *p = 7; // expected-warning {{Use of zero-allocated memory}}
@@ -1076,6 +1096,21 @@
   s2[validIndex + 1] = 'b';
 } // expected-warning {{Potential leak of memory pointed to by}}
 
+void testWinStrdup(const char *s, unsigned validIndex) {
+  char *s2 = _strdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWcsdup(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = wcsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWcsdup(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
 int testStrndup(const char *s, unsigned validIndex, unsigned size) {
   char *s2 = strndup(s, size);
   s2 [validIndex + 1] = 'b';
@@ -1091,6 +1126,24 @@
   free(s2);
 }
 
+void testWinStrdupContentIsDefined(const char *s, unsigned validIndex) {
+  char *s2 = _strdup(s);
+  char result = s2[1];// no warning
+  free(s2);
+}
+
+void testWcsdupContentIsDefined(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = wcsdup(s);
+  wchar_t result = s2[1];// no warning
+  free(s2);
+}
+
+void testWinWcsdupContentIsDefined(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup(s);
+  wchar_t result = s2[1];// no warning
+  free(s2);
+}
+
 // 
 // Test the system library functions to which the pointer can escape.
 // This tests false positive suppression.
@@ -1444,6 +1497,10 @@
   return strdup(strdup(str)); // expected-warning{{leak}}
 }
 
+char *testWinLeakWithinReturn(wchar_t *str) {
+  return _strdup(_strdup(str)); // expected-warning{{leak}}
+}
+
 void passConstPtr(const char * ptr);
 
 void testPassConstPointer() {
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), 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_if_nameindex(nullptr),
-II_if_freenameindex(nullptr) {}
+  : II_alloca(nullptr), II_win_alloca(nullptr), 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_win_strdup(nullptr), II_kmalloc(nullptr),
+II_if_nameindex(nullptr), II_if_freenameindex(nullptr),
+II_wcsdup(nullptr), II_win_wcsdup(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
@@ -231,10 +232,11 @@
   mutable std::unique_ptr BT_MismatchedDealloc;
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_alloca, *I

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Hmm, check-all produced a number of issues, which I think stem from the 
following warning:

  File C:\LLVM\llvm\tools\clang\test\Analysis\malloc.c Line 16: unknown type 
name 'wchar_t'

(with a bunch of instances of that)

...so how do I declare/define `wchar_t` properly?


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#366883, @zaks.anna wrote:

> Please, do not submit patches for review before they pass all tests.


Whoops, sorry. Should I been a bit clearer that check-all hadn't finished when 
I updated the diff, or should I not update the diff at all until check-all 
finishes?


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Hmm. This seems to be because `wchar_t` is enabled by `-fms-compatibility`.

@aaron.ballman do you have any idea how to define `wchar_t` without 
`-fms-compatibility`?


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 49845.
ariccio added a comment.

Alrighty. This final version of the patch causes no test failures (vs the 
unmodified build*).

*An unrelated test normally fails on my machine.


http://reviews.llvm.org/D17688

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -4,6 +4,21 @@
 
 void clang_analyzer_eval(int);
 
+// Without -fms-compatibility, wchar_t isn't a builtin type. MSVC defines
+// _WCHAR_T_DEFINED if wchar_t is available. Microsoft recommends that you use
+// the builtin type: "Using the typedef version can cause portability
+// problems", but we're ok here because we're not actually running anything.
+// Also of note is this cryptic warning: "The wchar_t type is not supported
+// when you compile C code".
+//
+// See the docs for more:
+// https://msdn.microsoft.com/en-us/library/dh8che7s.aspx
+#if !defined(_WCHAR_T_DEFINED)
+// "Microsoft implements wchar_t as a two-byte unsigned value"
+typedef unsigned short wchar_t;
+#define _WCHAR_T_DEFINED
+#endif // !defined(_WCHAR_T_DEFINED)
+
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void *alloca(size_t);
@@ -13,9 +28,15 @@
 void *reallocf(void *ptr, size_t size);
 void *calloc(size_t nmemb, size_t size);
 char *strdup(const char *s);
+wchar_t *wcsdup(const wchar_t *s);
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
 
+// Windows variants
+char *_strdup(const char *strSource);
+wchar_t *_wcsdup(const wchar_t *strSource);
+void *_alloca(size_t size);
+
 void myfoo(int *p);
 void myfooint(int p);
 char *fooRetPtr();
@@ -55,6 +76,10 @@
   int *p = alloca(sizeof(int));
 } // no warn
 
+void winAllocaTest() {
+  int *p = _alloca(sizeof(int));
+} // no warn
+
 void allocaBuiltinTest() {
   int *p = __builtin_alloca(sizeof(int));
 } // no warn
@@ -210,6 +235,11 @@
   int *p = alloca(0); // no warning
 }
 
+void CheckUseZeroWinAllocatedNoWarn2() {
+  int *p = _alloca(0); // no warning
+}
+
+
 void CheckUseZeroAllocatedNoWarn3() {
   int *p = malloc(0);
   int *q = realloc(p, 8); // no warning
@@ -233,6 +263,11 @@
   return *p; // expected-warning {{Use of zero-allocated memory}}
 }
 
+char CheckUseZeroWinAllocated2() {
+  char *p = _alloca(0);
+  return *p; // expected-warning {{Use of zero-allocated memory}}
+}
+
 void UseZeroAllocated(int *p) {
   if (p)
 *p = 7; // expected-warning {{Use of zero-allocated memory}}
@@ -1076,6 +,21 @@
   s2[validIndex + 1] = 'b';
 } // expected-warning {{Potential leak of memory pointed to by}}
 
+void testWinStrdup(const char *s, unsigned validIndex) {
+  char *s2 = _strdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWcsdup(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = wcsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinWcsdup(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
 int testStrndup(const char *s, unsigned validIndex, unsigned size) {
   char *s2 = strndup(s, size);
   s2 [validIndex + 1] = 'b';
@@ -1091,6 +1141,24 @@
   free(s2);
 }
 
+void testWinStrdupContentIsDefined(const char *s, unsigned validIndex) {
+  char *s2 = _strdup(s);
+  char result = s2[1];// no warning
+  free(s2);
+}
+
+void testWcsdupContentIsDefined(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = wcsdup(s);
+  wchar_t result = s2[1];// no warning
+  free(s2);
+}
+
+void testWinWcsdupContentIsDefined(const wchar_t *s, unsigned validIndex) {
+  wchar_t *s2 = _wcsdup(s);
+  wchar_t result = s2[1];// no warning
+  free(s2);
+}
+
 // 
 // Test the system library functions to which the pointer can escape.
 // This tests false positive suppression.
@@ -1444,6 +1512,14 @@
   return strdup(strdup(str)); // expected-warning{{leak}}
 }
 
+char *testWinLeakWithinReturn(char *str) {
+  return _strdup(_strdup(str)); // expected-warning{{leak}}
+}
+
+wchar_t *testWinWideLeakWithinReturn(wchar_t *str) {
+  return _wcsdup(_wcsdup(str)); // expected-warning{{leak}}
+}
+
 void passConstPtr(const char * ptr);
 
 void testPassConstPointer() {
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -169,11 +169,12 @@
 {
 public:
   MallocChecker()
-  : II_alloca(nullptr), II_malloc(nullptr), II_free(nullptr),
-II_reallo

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#368330, @zaks.anna wrote:

>


?


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17688: Fix missed leak from MSVC specific allocation functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17688#366780, @zaks.anna wrote:

> LGTM. Thanks!
>
> I can commit this in your behalf.


Oh, and yeah, I don't have privs.


http://reviews.llvm.org/D17688



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15921: [analyzer] Utility to match function calls.

[Alexander Riccio via cfe-commits]

ariccio added a subscriber: ariccio.
ariccio added a comment.

Side note: Has anybody ever considered just treating `fopen` and `fclose` like 
`malloc` and `free`?

On Windows I use a trick that I discovered what I call "the 
`_Post_ptr_invalid_` hack" because* the `_Post_ptr_invalid_` SAL annotation 
lets me treat the argument to `CloseHandle` like the argument to `free`, and 
reports double closes as double frees.

Here we'd just treat the `FILE*` as a different region of memory, to check that 
they're not mixed.

- this also works in part because a Windows `HANDLE` is just a `typedef`d 
`void*`



Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp:106
@@ -108,3 +105,3 @@
 SimpleStreamChecker::SimpleStreamChecker()
-: IIfopen(nullptr), IIfclose(nullptr) {
+: OpenFn("fopen"), CloseFn("fclose", 1) {
   // Initialize the bug types.

Why specify the RequiredArgs for `fclose`,  and not `fopen`?


Repository:
  rL LLVM

http://reviews.llvm.org/D15921



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D17983: Eliminate many benign instances of "potentially uninitialized local variable" warnings

[Alexander Riccio via cfe-commits]

ariccio created this revision.
ariccio added subscribers: llvm-commits, cfe-commits.
Herald added a reviewer: tstellarAMD.
Herald added subscribers: joker.eph, dsanders, arsenm, MatzeB.

Currently, the "potentially uninitialized local variable" & "potentially 
uninitialized local pointer variable" warnings are turned off. At some point we 
should probably turn them back on, because they can find serious problems. This 
patch eliminates the most obviously benign offenders. 


This leaves about ~33 not-so-obviously benign offenders. which I'll include 
here for convenience:



|Severity| Code | Description | Project | File | Line | Category | Source | 
Suppression |
    -    -    -     
-- -
|Warning|   C4703| potentially uninitialized local pointer variable 
'ExprOffset' used | LLVMMipsAsmParser | 
c:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp | 2668 |
|Warning|   C4701| potentially uninitialized local variable 'Encoding' used 
| clangCodeGen | c:\llvm\llvm\tools\clang\lib\codegen\cgdebuginfo.cpp | 573 |
|Warning|   C4701| potentially uninitialized local variable 'result' used | 
clangCodeGen | c:\llvm\llvm\tools\clang\lib\codegen\cgobjc.cpp | 2675 |
|Warning|   C4701| potentially uninitialized local variable 'Nullability' 
used | clangParse | c:\llvm\llvm\tools\clang\lib\parse\parseobjc.cpp | 1202 |
|Warning|   C4701| potentially uninitialized local variable 
'RepresentationMethod' used | clangParse | 
c:\llvm\llvm\tools\clang\lib\parse\parsepragma.cpp | 1582 |
|Warning|   C4701| potentially uninitialized local variable 'FirstOpKind' 
used | clangSema | c:\llvm\llvm\tools\clang\lib\sema\sematemplate.cpp | 4464 |
|Warning|   C4701| potentially uninitialized local variable 
'FnEntry8BitAbbrev' used | LLVMBitWriter | 
c:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp | 2353 |
|Warning|   C4701| potentially uninitialized local variable 
'FnEntry7BitAbbrev' used | LLVMBitWriter | 
c:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp | 2357 |
|Warning|   C4701| potentially uninitialized local variable 
'FnEntry6BitAbbrev' used | LLVMBitWriter | 
c:\llvm\llvm\lib\bitcode\writer\bitcodewriter.cpp | 2355 |
|Warning|   C4701| potentially uninitialized local variable 'OutIt' used | 
LLVMCodeGen | c:\llvm\llvm\lib\codegen\liveinterval.cpp | 876 |
|Warning|   C4701| potentially uninitialized local variable 'mid' used | 
LLVMHexagonCodeGen | c:\llvm\build\lib\target\hexagon\hexagongeninstrinfo.inc | 
9722 |
|Warning|   C4701| potentially uninitialized local variable 
'RealEightBitCounterArray' used | LLVMInstrumentation | 
c:\llvm\llvm\lib\transforms\instrumentation\sanitizercoverage.cpp | 294 |
|Warning|   C4701| potentially uninitialized local variable 'LoOffset' used 
| LLVMMipsAsmParser | c:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp 
| 2668 |
|Warning|   C4701| potentially uninitialized local variable 'HiOffset' used 
| LLVMMipsAsmParser | c:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp 
| 2659 |
|Warning|   C4701| potentially uninitialized local variable 'ExprOffset' 
used | LLVMMipsAsmParser | 
c:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp | 2668 |
|Warning|   C4701| potentially uninitialized local variable 
'BranchTargetNoTraps' used | LLVMMipsAsmParser | 
c:\llvm\llvm\lib\target\mips\asmparser\mipsasmparser.cpp | 3072 |
Warning|C4701| potentially uninitialized local variable 'MatImm' used | 
LLVMPowerPCCodeGen | c:\llvm\llvm\lib\target\powerpc\ppciseldagtodag.cpp | 808 |
|Warning|   C4701| potentially uninitialized local variable 'MaskEnd' used 
| LLVMPowerPCCodeGen | c:\llvm\llvm\lib\target\powerpc\ppciseldagtodag.cpp | 
809 |
|Warning|   C4701| potentially uninitialized local variable 'FirstLP' used 
| LLVMScalarOpts | c:\llvm\llvm\lib\transforms\scalar\loadcombine.cpp | 198 |
|Warning|   C4701| potentially uninitialized local variable 'CC' used | 
LLVMSelectionDAG | c:\llvm\llvm\lib\codegen\selectiondag\dagcombiner.cpp | 
13889 |
|Warning|   C4701| potentially uninitialized local variable 'Operation' 
used | llvm-ar | c:\llvm\llvm\tools\llvm-ar\llvm-ar.cpp | 287 |
|Warning|   C4701| potentially uninitialized local variable 'Kind' used | 
llvm-ar | c:\llvm\llvm\tools\llvm-ar\llvm-ar.cpp | 602 |
|Warning|   C4701| potentially uninitialized local variable 'CurSegAddress' 
used | llvm-objdump | c:\llvm\llvm\tools\llvm-objdump\machodump.cpp | 8548 |
|Warning|   C4701| potentially uninitialized local variable 'objc_class' 
used | llvm-objdump | c:\llvm\llvm\tools\llvm-objdump\machodump.cpp | 5435 |
|Warning|   C4701| potentially uninitialized local variable 'r_type' used | 
llvm-objdump | c:\llvm\llvm\tools\llvm-objdump\machodump.cpp | 1781 |
|Warning|   C4701| potentially uninitialized local variable 'r_value' used 
| llvm-objdump | c:\llvm\llvm\tools\llvm-objdump\machodump.cpp | 1783 |
|Warnin

Re: [PATCH] D17983: Eliminate many benign instances of "potentially uninitialized local variable" warnings

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D17983#370717, @dblaikie wrote:

> Initializations we never expect to use (eg because we have a covered switch
>  that initializes in all cases, or some slightly complex control flow the
>  compiler can't see through) hinder our ability to find uses of those with
>  tools like msan.


Too bad... Maybe msan should have a way to specify that you're default 
initializing a variable, and treat it as if you're not initializing it at all?

That said, I'm not sure that I agree with keeping `Potentially uninitialized 
local pointer variable 'name' used` off (//that's C4703 
, not C4701 
//), because 
uninitialized local pointer bugs are more dangerous (I'm thinking 

 of write-what-where 

 uninitialized pointer vulnerabilities 
),
 and I'd rather crash on a nullptr deref than some undefined behavior.

I will drop the `default` cases as @dsanders noted, but should I drop all the 
local variable inits too? In several cases (which I didn't specifically note) I 
chose default initialization values that would trigger pre-existing `assert`s 
if nobody assigned anything else to them.

Regarding the 33 warnings I noted, i couldn't tell whether they're obviously 
false positives, which I think warrants someone other than me carefully looking 
through it. Some of the warnings are caused by assigning to a variable inside 
of a loop with a dynamically sized bound; for those, I'd much rather (a) 
`assert` that the bound is nonzero/loop will be executed at least once, or (b) 
assign the variable some sentinel value, and then assert it before the variable 
is actually used. Thoughts?


http://reviews.llvm.org/D17983



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17983: Eliminate many benign instances of "potentially uninitialized local variable" warnings

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Oh, and by the way, what's the policy on using `enum class`es instead of C 
style enums? I bet the compiler would have fewer false positives with strongly 
typed enums?


http://reviews.llvm.org/D17983



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17983: Eliminate many benign instances of "potentially uninitialized local variable" warnings

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 50241.
ariccio added a comment.

For some reason, this smaller diff was harder to upload than the first diff.


http://reviews.llvm.org/D17983

Files:
  llvm/lib/Analysis/DependenceAnalysis.cpp
  llvm/lib/Analysis/InstructionSimplify.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/CodeGen/AtomicExpandPass.cpp
  llvm/lib/CodeGen/LiveInterval.cpp
  llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/MC/MCAsmStreamer.cpp
  llvm/lib/MC/MachObjectWriter.cpp
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/YAMLParser.cpp
  llvm/lib/Target/AArch64/AArch64AdvSIMDScalarPass.cpp
  llvm/lib/Target/AArch64/AArch64FastISel.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
  llvm/lib/Target/AMDGPU/AMDILCFGStructurizer.cpp
  llvm/lib/Target/AMDGPU/R600InstrInfo.cpp
  llvm/lib/Target/ARM/ARMFastISel.cpp
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
  llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp
  llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
  llvm/lib/Target/Mips/MipsAsmPrinter.cpp
  llvm/lib/Target/TargetRecip.cpp
  llvm/lib/Target/X86/X86FastISel.cpp
  llvm/lib/Target/X86/X86FixupLEAs.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86InstrInfo.cpp
  llvm/lib/Target/X86/X86WinEHState.cpp
  llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp
  llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
  llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
  llvm/lib/Transforms/Scalar/SROA.cpp
  llvm/lib/Transforms/Utils/MemorySSA.cpp
  llvm/tools/clang/include/clang/Sema/Lookup.h
  llvm/tools/clang/lib/AST/ExprConstant.cpp
  llvm/tools/clang/lib/AST/MicrosoftMangle.cpp
  llvm/tools/clang/lib/CodeGen/CGCleanup.h
  llvm/tools/clang/lib/CodeGen/CGExprScalar.cpp
  llvm/tools/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  llvm/tools/clang/lib/CodeGen/CodeGenAction.cpp
  llvm/tools/clang/lib/CodeGen/TargetInfo.cpp
  llvm/tools/clang/lib/Driver/ToolChain.cpp
  llvm/tools/clang/lib/Lex/Preprocessor.cpp
  llvm/tools/clang/lib/Parse/ParseObjc.cpp
  llvm/tools/clang/lib/Sema/SemaDecl.cpp
  llvm/tools/clang/lib/Sema/SemaDeclAttr.cpp
  llvm/tools/clang/lib/Sema/SemaDeclCXX.cpp
  llvm/tools/clang/lib/Sema/SemaInit.cpp
  llvm/tools/clang/lib/Sema/SemaLambda.cpp
  llvm/tools/clang/lib/Sema/SemaLookup.cpp
  llvm/tools/clang/lib/Sema/SemaStmtAttr.cpp
  llvm/tools/clang/lib/Serialization/ASTReader.cpp
  llvm/tools/clang/lib/Serialization/ASTReaderStmt.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
  llvm/tools/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

Index: llvm/lib/MC/MCAsmStreamer.cpp
===
--- llvm/lib/MC/MCAsmStreamer.cpp
+++ llvm/lib/MC/MCAsmStreamer.cpp
@@ -990,7 +990,7 @@
   if (PrologueEnd)
 OS << " prologue_end";
 
-  unsigned OldIsStmt = getContext().getCurrentCVLoc().isStmt();
+  bool OldIsStmt = getContext().getCurrentCVLoc().isStmt();
   if (IsStmt != OldIsStmt) {
 OS << " is_stmt ";
 
Index: llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
===
--- llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
+++ llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
@@ -2712,7 +2712,10 @@
   unsigned ZeroSrcOpcode, ZeroTrgOpcode;
   bool ReverseOrderSLT, IsUnsigned, IsLikely, AcceptsEquality;
 
-  unsigned TrgReg;
+  // TrgReg should never normally be assigned NUM_TARGET_REGS.
+  // If you end up with NUM_TARGET_REGS, you have a bug.
+  // FIXME: is there a better way to ensure TrgReg is assigned something?
+  unsigned TrgReg = Mips::NUM_TARGET_REGS;
   if (TrgOp.isReg())
 TrgReg = TrgOp.getReg();
   else if (TrgOp.isImm()) {
Index: llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp
===
--- llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp
+++ llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp
@@ -170,7 +170,7 @@
   }
 
   bool doneShuffling = false;
-  unsigned shuffleError;
+  unsigned shuffleError = HexagonShuffler::SHUFFLE_ERROR_UNKNOWN;
   while (possibleDuplexes.size() > 0 && (!doneShuffling)) {
 // case of Duplex Found
 DuplexCandidate duplexToTry = possibleDuplexes.pop_back_val();
Index: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
===
--- llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -930,7 +930,7 @@
   if (LHSCC == ICmpInst::ICMP_EQ && LHSCC == RHSCC &&
   LHS->hasOneUse() && RHS->hasOneUse()) {
 Value *V;
-ConstantInt *AndC

[PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio created this revision.
ariccio added reviewers: zaks.anna, dcoughlin, aaron.ballman.
ariccio added a subscriber: cfe-commits.

As discussed...

http://reviews.llvm.org/D18073

Files:
  llvm/tools/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  llvm/tools/clang/test/Analysis/malloc.c

Index: llvm/tools/clang/test/Analysis/malloc.c
===
--- llvm/tools/clang/test/Analysis/malloc.c
+++ llvm/tools/clang/test/Analysis/malloc.c
@@ -31,12 +31,50 @@
 wchar_t *wcsdup(const wchar_t *s);
 char *strndup(const char *s, size_t n);
 int memcmp(const void *s1, const void *s2, size_t n);
+char *tempnam(const char *dir, const char *pfx);
 
+
 // Windows variants
 char *_strdup(const char *strSource);
 wchar_t *_wcsdup(const wchar_t *strSource);
+unsigned char *_mbsdup(const unsigned char *strSource);
+
 void *_alloca(size_t size);
 
+char *_tempnam(const char *dir, const char *prefix);
+wchar_t *_wtempnam(const wchar_t *dir, const wchar_t *prefix);
+
+
+void *_calloc_dbg(size_t num, size_t size, int blockType,
+  const char *filename, int linenumber);
+
+char *_strdup_dbg(const char *strSource, int blockType,
+  const char *filename, int linenumber);
+
+wchar_t *_wcsdup_dbg(const wchar_t *strSource, int blockType,
+ const char *filename, int linenumber);
+
+unsigned char *_mbsdup_dbg(const unsigned char *strSource, int blockType,
+   const char *filename, int linenumber);
+
+char *_tempnam_dbg(const char *dir, const char *prefix, int blockType,
+   const char *filename, int linenumber);
+
+wchar_t *_wtempnam_dbg(const wchar_t *dir, const wchar_t *prefix,
+   int blockType, const char *filename, int linenumber);
+
+
+
+// Very frequently used debug versions
+void _free_dbg(void *userData, int blockType);
+void _malloc_dbg(size_t size, int blockType, const char *filename,
+ int linenumber);
+
+void *_realloc_dbg(void *userData, size_t newSize,
+   int blockType, const char *filename, int linenumber);
+
+
+
 void myfoo(int *p);
 void myfooint(int p);
 char *fooRetPtr();
@@ -291,25 +329,37 @@
 }
 
 void CheckUseZeroAllocated6() {
+  int *p = _calloc_dbg(0, 2, 0, __FILE__, __LINE__);
+  *p = 1; // expected-warning {{Use of zero-allocated memory}}
+  free(p);
+}
+
+void CheckUseZeroAllocated7() {
   int *p = calloc(2, 0);
   *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
-void CheckUseZeroAllocated7() {
+void CheckUseZeroAllocated8() {
+  int *p = _calloc_dbg(2, 0, 0, __FILE__, __LINE__);
+  *p = 1; // expected-warning {{Use of zero-allocated memory}}
+  free(p);
+}
+
+void CheckUseZeroAllocated9() {
   int *p = realloc(0, 0);
   *p = 1; // expected-warning {{Use of zero-allocated memory}}
   free(p);
 }
 
-void CheckUseZeroAllocated8() {
+void CheckUseZeroAllocated10() {
   int *p = malloc(8);
   int *q = realloc(p, 0);
   *q = 1; // expected-warning {{Use of zero-allocated memory}}
   free(q);
 }
 
-void CheckUseZeroAllocated9() {
+void CheckUseZeroAllocated11() {
   int *p = realloc(0, 0);
   int *q = realloc(p, 0);
   *q = 1; // expected-warning {{Use of zero-allocated memory}}
@@ -690,6 +740,12 @@
   return; // expected-warning{{Potential leak of memory pointed to by 'buf'}}
 }
 
+// This tests that calloc() buffers need to be freed
+void callocNoFreeDbg () {
+  char *buf = _calloc_dbg(2,2, 0, __FILE__, __LINE__);
+  return; // expected-warning{{Potential leak of memory pointed to by 'buf'}}
+}
+
 // These test that calloc() buffers are zeroed by default
 char callocZeroesGood () {
 	char *buf = calloc(2,2);
@@ -700,6 +756,16 @@
 	return result; // no-warning
 }
 
+// These test that calloc() buffers are zeroed by default
+char callocZeroesGoodDbg () {
+	char *buf = _calloc_dbg(2,2,0, __FILE__, __LINE__);
+	char result = buf[3]; // no-warning
+	if (buf[1] == 0) {
+	  free(buf);
+	}
+	return result; // no-warning
+}
+
 char callocZeroesBad () {
 	char *buf = calloc(2,2);
 	char result = buf[3]; // no-warning
@@ -709,6 +775,15 @@
 	return result; // expected-warning{{Potential leak of memory pointed to by 'buf'}}
 }
 
+char callocZeroesBadDbg () {
+	char *buf = _calloc_dbg(2,2, 0, __FILE__, __LINE__);
+	char result = buf[3]; // no-warning
+	if (buf[1] != 0) {
+	  free(buf); // expected-warning{{never executed}}
+	}
+	return result; // expected-warning{{Potential leak of memory pointed to by 'buf'}}
+}
+
 void nullFree() {
   int *p = 0;
   free(p); // no warning - a nop
@@ -1126,6 +1201,26 @@
   s2[validIndex + 1] = 'b';
 } // expected-warning {{Potential leak of memory pointed to by}}
 
+void testWinMbsdup(const unsigned char *s, unsigned validIndex) {
+  unsigned char *s2 = _mbsdup(s);
+  s2[validIndex + 1] = 'b';
+} // expected-warning {{Potential leak of memory pointed to by}}
+
+void testWinMbsdupDbg(const unsigned char *s, unsigned validIndex) {
+  unsigned char *s2 = _mbsdu

Re: [PATCH] D18073: Add memory allocating functions

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18073#372697, @zaks.anna wrote:

> Since we are adding support for so many new APIs that are only available on 
> Windows, could you please condition checking them only when we build for 
> Windows. You probably can look and Language Options to figure that out.


Is there a problem with checking  them on linux/OSX also?

> Also, we should not duplicate all of our tests. Instead, we should add a 
> single test per added API checking that it is modeling the operation we think 
> it should model.


So should I dump the _dbg versions from the tests?


http://reviews.llvm.org/D18073



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16873: Refactor MemRegionManager::getVarRegion to call two new functions, improving readability

[Alexander Riccio via cfe-commits]

ariccio marked an inline comment as done.
ariccio added a comment.

In http://reviews.llvm.org/D16873#371736, @a.sidorin wrote:

> Alexander, could you update status of this review?


It's become a zombie review. I should just abandon it, as I'll be away for two 
weeks starting tomorrow.


http://reviews.llvm.org/D16873



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18497: Auto-install LLVM Visual Studio visualizers for VS2015 and up

[Alexander Riccio via cfe-commits]

ariccio added a comment.

`utils/llvm.natvis` is a duplicate of `utils/LLVMVisualizers/llvm.natvis`?

Otherwise, assuming everything builds correctly, LGTM.

Once again, thanks for doing this!


http://reviews.llvm.org/D18497



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18498: Auto-install Clang Visual Studio visualizers for VS2015 and up

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18498#384274, @zturner wrote:

> Ahh that's surprising. If it works even with the none tag, i guess my
>  concerns are not valid


I'm only partly surprised... It's not the first time that a Microsoft product 
behaves surprisingly :)


http://reviews.llvm.org/D18498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18498: Auto-install Clang Visual Studio visualizers for VS2015 and up

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Assuming everything builds correctly, LGTM.

Your CMake is better than mine, so I'm not sure if there're better ways to do 
this ;)



Comment at: utils/ClangVisualizers/CMakeLists.txt:2
@@ +1,3 @@
+# Do this by hand instead of using add_llvm_utilities(), which
+# tries to create a corresponding executable, which we don't want
+if (LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION)

Obsessive nit: Use a period after the "want", like `want.`


Comment at: utils/ClangVisualizers/clang.natvis:9
@@ -8,2 +8,3 @@
+For later versions of Visual Studio, no setup is required-->
 http://schemas.microsoft.com/vstudio/debugger/natvis/2010";>
 

Is it just me, or is this a dead link?


http://reviews.llvm.org/D18498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18497: Auto-install LLVM Visual Studio visualizers for VS2015 and up

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18497#384362, @mspertus wrote:

> In http://reviews.llvm.org/D18497#384351, @ariccio wrote:
>
> > Otherwise, assuming everything builds correctly, LGTM.
>
>
> There are a lot scenarios. What do all of these files in the build tree like 
> `cmake_install.cmake`, `install.vcxproj`, `Package.vcxproj`? How do I test 
> them?


Do you have a clean copy of the tree? Maybe do something like the equivalent of 
`make clean` 
?
 Then try to regenerate everything?

> > Once again, thanks for doing this!

> 

> 

> You're welcome. Thanks for all your help. I am in India right now with a 
> limited environment. I will likely due some more testing when I get home and 
> check in at the end of the week.


Well that's certainly more exotic than me! I'm impressed. I assume you have a 
laptop, and you're not doing this on your smartphone.


http://reviews.llvm.org/D18497



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18498: Auto-install Clang Visual Studio visualizers for VS2015 and up

[Alexander Riccio via cfe-commits]

ariccio added inline comments.


Comment at: utils/ClangVisualizers/CMakeLists.txt:3
@@ +2,3 @@
+# tries to create a corresponding executable, which we don't want.
+if (LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION)
+  set(CLANG_VISUALIZERS clang.natvis)

Hmm, that's gonna bug me. Maybe I'll just have to proofread //all// of them.


Comment at: utils/ClangVisualizers/clang.natvis:9
@@ -8,2 +8,3 @@
+For later versions of Visual Studio, no setup is required-->
 http://schemas.microsoft.com/vstudio/debugger/natvis/2010";>
 

Ahh, that's actually quite smart.


http://reviews.llvm.org/D18498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18498: Auto-install Clang Visual Studio visualizers for VS2015 and up

[Alexander Riccio via cfe-commits]

ariccio added a comment.

In http://reviews.llvm.org/D18498#384711, @mspertus wrote:

> I installed both VS2015 and VS2013 on my laptop and saw the correct solution 
> files built by cmake.


Btw, make sure you're running the latest updates for both Visual Studio, else 
you'll get funny build errors (probably about /Zc:strictStrings).


http://reviews.llvm.org/D18498



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio added a comment.

Oops.  The funny thing about the extra spaces is that I wasn't sure why there 
were spaces after the casts already, so I figured that I should keep it that 
way.

I shall fix.


http://reviews.llvm.org/D16748



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16748: Cleanup MemRegion.cpp/MemRegion.h

[Alexander Riccio via cfe-commits]

ariccio updated this revision to Diff 46481.
ariccio marked 10 inline comments as done.
ariccio added a comment.

Removed extra spaces.


http://reviews.llvm.org/D16748

Files:
  
C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp

Index: C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
===
--- C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
+++ C:/LLVM/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
@@ -1319,8 +1319,8 @@
 
   void setTrait(SymbolRef Sym, InvalidationKinds IK);
   void setTrait(const MemRegion *MR, InvalidationKinds IK);
-  bool hasTrait(SymbolRef Sym, InvalidationKinds IK);
-  bool hasTrait(const MemRegion *MR, InvalidationKinds IK);
+  bool hasTrait(SymbolRef Sym, InvalidationKinds IK) const;
+  bool hasTrait(const MemRegion *MR, InvalidationKinds IK) const;
 };
   
 } // end GR namespace
Index: C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ C:/LLVM/llvm/tools/clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -46,7 +46,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -64,7 +64,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -85,7 +85,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, a2, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -104,7 +104,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, a2, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -123,7 +123,7 @@
InsertPos));
 
   if (!R) {
-R = (RegionTy*) A.Allocate();
+R = A.Allocate();
 new (R) RegionTy(a1, a2, a3, superRegion);
 Regions.InsertNode(R, InsertPos);
   }
@@ -246,39 +246,39 @@
 //===--===//
 
 void MemSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast(getKind()));
 }
 
 void StackSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast(getKind()));
   ID.AddPointer(getStackFrame());
 }
 
 void StaticGlobalSpaceRegion::Profile(llvm::FoldingSetNodeID &ID) const {
-  ID.AddInteger((unsigned)getKind());
+  ID.AddInteger(static_cast(getKind()));
   ID.AddPointer(getCodeRegion());
 }
 
 void StringRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
  const StringLiteral* Str,
  const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) StringRegionKind);
+  ID.AddInteger(static_cast(StringRegionKind));
   ID.AddPointer(Str);
   ID.AddPointer(superRegion);
 }
 
 void ObjCStringRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
  const ObjCStringLiteral* Str,
  const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) ObjCStringRegionKind);
+  ID.AddInteger(static_cast(ObjCStringRegionKind));
   ID.AddPointer(Str);
   ID.AddPointer(superRegion);
 }
 
 void AllocaRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
  const Expr *Ex, unsigned cnt,
  const MemRegion *superRegion) {
-  ID.AddInteger((unsigned) AllocaRegionKind);
+  ID.AddInteger(static_cast(AllocaRegionKind));
   ID.AddPointer(Ex);
   ID.AddInteger(cnt);
   ID.AddPointer(superRegion);
@@ -295,15 +295,15 @@
 void CompoundLiteralRegion::ProfileRegion(llvm::FoldingSetNodeID& ID,
   const CompoundLiteralExpr *CL,
   const MemRegion* superRegion) {
-  ID.AddInteger((unsigned) CompoundLiteralRegionKind);
+  ID.AddInteger(static_cast(CompoundLiteralRegionKind));
   ID.AddPointer(CL);
   ID.AddPointer(superRegion);
 }
 
 void CXXThisRegion::ProfileRegion(llvm::FoldingSetNodeID &ID,
   const PointerType *PT,
   const MemRegion *sRegion) {
-  ID.AddInteger((unsigned)