[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358945: [analyzer] PR41335: Fix crash when no-store event is 
in a body-farmed function. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60808?vs=195851=196175#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  test/Analysis/OSAtomic_mac.c

Index: include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
 *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@
   PathDiagnosticPiece::Kind k,
   bool addPosRange = true)
   : PathDiagnosticPiece(s, k), Pos(pos) {
-assert(Pos.isValid() && Pos.asLocation().isValid() &&
+assert(Pos.isValid() && Pos.hasValidLocation() &&
"PathDiagnosticSpotPiece's must have a valid location.");
 if (addPosRange && Pos.hasRange()) addRange(Pos.asRange());
   }
Index: test/Analysis/OSAtomic_mac.c
===
--- test/Analysis/OSAtomic_mac.c
+++ test/Analysis/OSAtomic_mac.c
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection \
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {
+  // There is some body in the actual header,
+  // but we should trust our BodyFarm instead.
+}
+
+int *invalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // no-crash
+  // FIXME: We don't really need these notes.
+  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+// expected-note@-1{{Undefined or garbage value returned to caller}}
+}
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -335,7 +335,7 @@
 if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
 potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
   IvarR->getDecl()))
-  return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+  return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
/*FirstIsReferenceType=*/false, 1);
   }
 }
@@ -344,7 +344,7 @@
   const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
   if (RegionOfInterest->isSubRegionOf(ThisR)
   && !CCall->getDecl()->isImplicit())
-return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
  /*FirstIsReferenceType=*/false, 1);
 
   // Do not generate diagnostics for not modified parameters in
@@ -363,7 +363,7 @@
   QualType T = PVD->getType();
   while (const MemRegion *MR = V.getAsRegion()) {
 if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-  return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+  return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
ParamIsReferenceType, IndirectionLevel);
 
 QualType PT = T->getPointeeType();
@@ -371,7 +371,7 @@
 
 if (const RecordDecl *RD = PT->getAsRecordDecl())
   if (auto P = findRegionOfInterestInRecord(RD, State, MR))
-return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
  ParamIsReferenceType, IndirectionLevel);
 
 V = State->getSVal(MR, PT);
@@ -549,7 +549,7 @@
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
   std::shared_ptr
-  maybeEmitNode(BugReport , const CallEvent , const ExplodedNode *N,
+  maybeEmitNote(BugReport , const CallEvent , const ExplodedNode *N,
 const RegionVector , const MemRegion *MatchedRegion,
 StringRef FirstElement, bool FirstIsReferenceType,
 unsigned 

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195851.
NoQ marked an inline comment as done.
NoQ added a comment.

Add space before `\`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/OSAtomic_mac.c

Index: clang/test/Analysis/OSAtomic_mac.c
===
--- /dev/null
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection \
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {
+  // There is some body in the actual header,
+  // but we should trust our BodyFarm instead.
+}
+
+int *invalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // no-crash
+  // FIXME: We don't really need these notes.
+  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+// expected-note@-1{{Undefined or garbage value returned to caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -335,7 +335,7 @@
 if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
 potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
   IvarR->getDecl()))
-  return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+  return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
/*FirstIsReferenceType=*/false, 1);
   }
 }
@@ -344,7 +344,7 @@
   const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
   if (RegionOfInterest->isSubRegionOf(ThisR)
   && !CCall->getDecl()->isImplicit())
-return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
  /*FirstIsReferenceType=*/false, 1);
 
   // Do not generate diagnostics for not modified parameters in
@@ -363,7 +363,7 @@
   QualType T = PVD->getType();
   while (const MemRegion *MR = V.getAsRegion()) {
 if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-  return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+  return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
ParamIsReferenceType, IndirectionLevel);
 
 QualType PT = T->getPointeeType();
@@ -371,7 +371,7 @@
 
 if (const RecordDecl *RD = PT->getAsRecordDecl())
   if (auto P = findRegionOfInterestInRecord(RD, State, MR))
-return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
  ParamIsReferenceType, IndirectionLevel);
 
 V = State->getSVal(MR, PT);
@@ -549,7 +549,7 @@
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
   std::shared_ptr
-  maybeEmitNode(BugReport , const CallEvent , const ExplodedNode *N,
+  maybeEmitNote(BugReport , const CallEvent , const ExplodedNode *N,
 const RegionVector , const MemRegion *MatchedRegion,
 StringRef FirstElement, bool FirstIsReferenceType,
 unsigned IndirectionLevel) {
@@ -579,6 +579,9 @@
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
 
+if (!L.hasValidLocation())
+  return nullptr;
+
 SmallString<256> sbuf;
 llvm::raw_svector_ostream os(sbuf);
 os << "Returning without writing to '";
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
 *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@
   PathDiagnosticPiece::Kind k,
   bool addPosRange = true)
   : PathDiagnosticPiece(s, k), Pos(pos) {
-assert(Pos.isValid() && Pos.asLocation().isValid() &&
+assert(Pos.isValid() && Pos.hasValidLocation() &&

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195850.
NoQ added a comment.

Separate the canonicalization change that unbreaks body farms into a separate 
patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/OSAtomic_mac.c

Index: clang/test/Analysis/OSAtomic_mac.c
===
--- /dev/null
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection\
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {
+  // There is some body in the actual header,
+  // but we should trust our BodyFarm instead.
+}
+
+int *invalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // no-crash
+  // FIXME: We don't really need these notes.
+  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+// expected-note@-1{{Undefined or garbage value returned to caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -335,7 +335,7 @@
 if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
 potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
   IvarR->getDecl()))
-  return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+  return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
/*FirstIsReferenceType=*/false, 1);
   }
 }
@@ -344,7 +344,7 @@
   const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
   if (RegionOfInterest->isSubRegionOf(ThisR)
   && !CCall->getDecl()->isImplicit())
-return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
  /*FirstIsReferenceType=*/false, 1);
 
   // Do not generate diagnostics for not modified parameters in
@@ -363,7 +363,7 @@
   QualType T = PVD->getType();
   while (const MemRegion *MR = V.getAsRegion()) {
 if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-  return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+  return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
ParamIsReferenceType, IndirectionLevel);
 
 QualType PT = T->getPointeeType();
@@ -371,7 +371,7 @@
 
 if (const RecordDecl *RD = PT->getAsRecordDecl())
   if (auto P = findRegionOfInterestInRecord(RD, State, MR))
-return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
  ParamIsReferenceType, IndirectionLevel);
 
 V = State->getSVal(MR, PT);
@@ -549,7 +549,7 @@
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
   std::shared_ptr
-  maybeEmitNode(BugReport , const CallEvent , const ExplodedNode *N,
+  maybeEmitNote(BugReport , const CallEvent , const ExplodedNode *N,
 const RegionVector , const MemRegion *MatchedRegion,
 StringRef FirstElement, bool FirstIsReferenceType,
 unsigned IndirectionLevel) {
@@ -579,6 +579,9 @@
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
 
+if (!L.hasValidLocation())
+  return nullptr;
+
 SmallString<256> sbuf;
 llvm::raw_svector_ostream os(sbuf);
 os << "Returning without writing to '";
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
 *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@
   PathDiagnosticPiece::Kind k,
   bool addPosRange = true)
   : PathDiagnosticPiece(s, k), Pos(pos) {
-assert(Pos.isValid() && Pos.asLocation().isValid() &&
+assert(Pos.isValid() && 

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195810.
NoQ added a comment.
This revision is now accepted and ready to land.

Don't canonicalize the decl in the body farm. The decl supplied by the 
AnalysisDeclContext is already the correct one (and not necessarily the 
canonical one).

Keep the defensive behavior for NoStoreFuncVisitor because it's generally the 
right thing to do for future-proofness.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/OSAtomic_mac.c

Index: clang/test/Analysis/OSAtomic_mac.c
===
--- /dev/null
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection\
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {
+  // There is some body in the actual header,
+  // but we should trust our BodyFarm instead.
+}
+
+int *invalidSLocOnRedecl() {
+  // Was crashing when trying to throw a report about returning an uninitialized
+  // value to the caller. FIXME: We should probably still throw that report,
+  // something like "The "compare" part of CompareAndSwap depends on an
+  // undefined value".
+  int *b;
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // no-crash
+  return b;
+}
+
+void testThatItActuallyWorks() {
+  void *x = 0;
+  int res = OSAtomicCompareAndSwapPtrBarrier(0, , );
+  clang_analyzer_eval(res); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
+  clang_analyzer_eval(x == ); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -335,7 +335,7 @@
 if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
 potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
   IvarR->getDecl()))
-  return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+  return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
/*FirstIsReferenceType=*/false, 1);
   }
 }
@@ -344,7 +344,7 @@
   const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
   if (RegionOfInterest->isSubRegionOf(ThisR)
   && !CCall->getDecl()->isImplicit())
-return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
  /*FirstIsReferenceType=*/false, 1);
 
   // Do not generate diagnostics for not modified parameters in
@@ -363,7 +363,7 @@
   QualType T = PVD->getType();
   while (const MemRegion *MR = V.getAsRegion()) {
 if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-  return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+  return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
ParamIsReferenceType, IndirectionLevel);
 
 QualType PT = T->getPointeeType();
@@ -371,7 +371,7 @@
 
 if (const RecordDecl *RD = PT->getAsRecordDecl())
   if (auto P = findRegionOfInterestInRecord(RD, State, MR))
-return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
  ParamIsReferenceType, IndirectionLevel);
 
 V = State->getSVal(MR, PT);
@@ -549,7 +549,7 @@
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
   std::shared_ptr
-  maybeEmitNode(BugReport , const CallEvent , const ExplodedNode *N,
+  maybeEmitNote(BugReport , const CallEvent , const ExplodedNode *N,
 const RegionVector , const MemRegion *MatchedRegion,
 StringRef FirstElement, bool FirstIsReferenceType,
 unsigned IndirectionLevel) {
@@ -579,6 +579,11 @@
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
 
+// For now this shouldn't trigger, but once it does, we'll need to decide
+// if these reports are worth suppressing as well.
+if (!L.hasValidLocation())
+  return nullptr;
+
 SmallString<256> sbuf;
 llvm::raw_svector_ostream os(sbuf);
 os << "Returning without writing to '";
Index: clang/lib/Analysis/BodyFarm.cpp
===
--- clang/lib/Analysis/BodyFarm.cpp
+++ clang/lib/Analysis/BodyFarm.cpp
@@ 

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

(also fix typo in the function name)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808



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

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ planned changes to this revision.
NoQ added a comment.

In D60808#1469734 , @NoQ wrote:

> Hmm, i think i'd love to know why doesn't the uninitialized variable checker 
> fire on the if-statement as farmed by the body farm:


Passing arguments to this whole body farm thing doesn't work. It builds the 
body for the declaration on line 4 but then calls the declaration on line 5, 
and parameter variables in the synthesized body don't match parameter variables 
of the call, so it cannot read argument values :/


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808



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

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

LG with a couple of nits.




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:582
+if (!L.hasValidLocation()) {
+  // Do we need to suppress our report for body-farmed functions as well?
+  // Or maybe attach the note to the call site instead?

Should this be marked with a FIXME?



Comment at: clang/test/Analysis/diagnostics/body-farm-crashes.c:1
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core\
+// RUN:-analyzer-output=text -verify %s

Add a space before the backslash.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808



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

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hmm, i think i'd love to know why doesn't the uninitialized variable checker 
fire on the if-statement as farmed by the body farm:

  592   // Signature:
  593   // _Bool OSAtomicCompareAndSwapPtr(void *__oldValue,
  594   // void *__newValue,
  595   // void * volatile *__theValue)
  596   // Generate body:
  597   //   if (oldValue == *theValue) {
  598   //*theValue = newValue;
  599   //return YES;
  600   //   }
  601   //   else return NO;

(closing brace accidentally omitted in the original comment as well)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808



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

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, 
mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, jfb, a.sidorin, 
szepet, kristof.beyls, javed.absar.
Herald added a project: clang.
NoQ updated this revision to Diff 195497.
NoQ added a comment.

Fix comments.


It is getting increasingly annoying that it's so hard to construct a 
`PathDiagnosticLocation` correctly. I think we should eventually make the API 
more defensive to invalid source locations.


https://reviews.llvm.org/D60808

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/body-farm-crashes.c


Index: clang/test/Analysis/diagnostics/body-farm-crashes.c
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/body-farm-crashes.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core\
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {}
+int *atomicInvalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+   // FIXME: These notes shouldn't be there because there's nothing 
between them.
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // expected-note{{Calling 
'OSAtomicCompareAndSwapPtrBarrier'}}
+   

// expected-note@-1{{Returning from 
'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to 
caller}}
+// expected-note@-1{{Undefined or garbage value returned to 
caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -578,6 +578,11 @@
 
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
+if (!L.hasValidLocation()) {
+  // Do we need to suppress our report for body-farmed functions as well?
+  // Or maybe attach the note to the call site instead?
+  return nullptr;
+}
 
 SmallString<256> sbuf;
 llvm::raw_svector_ostream os(sbuf);
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
 *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@
   PathDiagnosticPiece::Kind k,
   bool addPosRange = true)
   : PathDiagnosticPiece(s, k), Pos(pos) {
-assert(Pos.isValid() && Pos.asLocation().isValid() &&
+assert(Pos.isValid() && Pos.hasValidLocation() &&
"PathDiagnosticSpotPiece's must have a valid location.");
 if (addPosRange && Pos.hasRange()) addRange(Pos.asRange());
   }


Index: clang/test/Analysis/diagnostics/body-farm-crashes.c
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/body-farm-crashes.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core\
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {}
+int *atomicInvalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+	// FIXME: These notes shouldn't be there because there's nothing between them.
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // expected-note{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+			// expected-note@-1{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+// expected-note@-1{{Undefined or garbage value returned to caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -578,6 +578,11 @@
 
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
+if (!L.hasValidLocation()) {
+  // Do we need to suppress our report for body-farmed 

[PATCH] D60808: [analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 195497.
NoQ added a comment.

Fix comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60808/new/

https://reviews.llvm.org/D60808

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/body-farm-crashes.c


Index: clang/test/Analysis/diagnostics/body-farm-crashes.c
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/body-farm-crashes.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core\
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {}
+int *atomicInvalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+   // FIXME: These notes shouldn't be there because there's nothing 
between them.
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // expected-note{{Calling 
'OSAtomicCompareAndSwapPtrBarrier'}}
+   

// expected-note@-1{{Returning from 
'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to 
caller}}
+// expected-note@-1{{Undefined or garbage value returned to 
caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -578,6 +578,11 @@
 
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
+if (!L.hasValidLocation()) {
+  // Do we need to suppress our report for body-farmed functions as well?
+  // Or maybe attach the note to the call site instead?
+  return nullptr;
+}
 
 SmallString<256> sbuf;
 llvm::raw_svector_ostream os(sbuf);
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
 *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@
   PathDiagnosticPiece::Kind k,
   bool addPosRange = true)
   : PathDiagnosticPiece(s, k), Pos(pos) {
-assert(Pos.isValid() && Pos.asLocation().isValid() &&
+assert(Pos.isValid() && Pos.hasValidLocation() &&
"PathDiagnosticSpotPiece's must have a valid location.");
 if (addPosRange && Pos.hasRange()) addRange(Pos.asRange());
   }


Index: clang/test/Analysis/diagnostics/body-farm-crashes.c
===
--- /dev/null
+++ clang/test/Analysis/diagnostics/body-farm-crashes.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core\
+// RUN:-analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {}
+int *atomicInvalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+	// FIXME: These notes shouldn't be there because there's nothing between them.
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, ); // expected-note{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+			// expected-note@-1{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+// expected-note@-1{{Undefined or garbage value returned to caller}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -578,6 +578,11 @@
 
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
+if (!L.hasValidLocation()) {
+  // Do we need to suppress our report for body-farmed functions as well?
+  // Or maybe attach the note to the call site instead?
+  return nullptr;
+}
 
 SmallString<256> sbuf;
 llvm::raw_svector_ostream os(sbuf);
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++