[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

With body farms, i think, the current declaration also remains unchanged, we're 
just getting a shining new `CompoundStmt` as a body in the middle of nowhere, 
i.e., `ADC->getDecl()->getBody()` isn't the same as `ADC->getBody()`. Which is, 
yeah, still weird :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D60899



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

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358946: [analyzer] Unbreak body farms in presence of 
multiple declarations. (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60899?vs=195852&id=196176#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60899

Files:
  cfe/trunk/lib/Analysis/BodyFarm.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  cfe/trunk/test/Analysis/OSAtomic_mac.c


Index: cfe/trunk/test/Analysis/OSAtomic_mac.c
===
--- cfe/trunk/test/Analysis/OSAtomic_mac.c
+++ cfe/trunk/test/Analysis/OSAtomic_mac.c
@@ -8,13 +8,20 @@
 }
 
 int *invalidSLocOnRedecl() {
-  int *b; // expected-note{{'b' declared without an initial value}}
-
+  // 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, &b); // no-crash
-  // FIXME: We don't really need these notes.
-  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
-  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+  return b;
+}
 
-  return b; // expected-warning{{Undefined or garbage value returned to 
caller}}
-// expected-note@-1{{Undefined or garbage value returned to 
caller}}
+void testThatItActuallyWorks() {
+  void *x = 0;
+  int res = OSAtomicCompareAndSwapPtrBarrier(0, &x, &x);
+  clang_analyzer_eval(res); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
+  clang_analyzer_eval(x == &x); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
 }
Index: cfe/trunk/lib/Analysis/BodyFarm.cpp
===
--- cfe/trunk/lib/Analysis/BodyFarm.cpp
+++ cfe/trunk/lib/Analysis/BodyFarm.cpp
@@ -665,8 +665,6 @@
 }
 
 Stmt *BodyFarm::getBody(const FunctionDecl *D) {
-  D = D->getCanonicalDecl();
-
   Optional &Val = Bodies[D];
   if (Val.hasValue())
 return Val.getValue();
Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -579,6 +579,9 @@
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
 
+// For now this shouldn't trigger, but once it does (as we add more
+// functions to the body farm), we'll need to decide if these reports
+// are worth suppressing as well.
 if (!L.hasValidLocation())
   return nullptr;
 


Index: cfe/trunk/test/Analysis/OSAtomic_mac.c
===
--- cfe/trunk/test/Analysis/OSAtomic_mac.c
+++ cfe/trunk/test/Analysis/OSAtomic_mac.c
@@ -8,13 +8,20 @@
 }
 
 int *invalidSLocOnRedecl() {
-  int *b; // expected-note{{'b' declared without an initial value}}
-
+  // 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, &b); // no-crash
-  // FIXME: We don't really need these notes.
-  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
-  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+  return b;
+}
 
-  return b; // expected-warning{{Undefined or garbage value returned to caller}}
-// expected-note@-1{{Undefined or garbage value returned to caller}}
+void testThatItActuallyWorks() {
+  void *x = 0;
+  int res = OSAtomicCompareAndSwapPtrBarrier(0, &x, &x);
+  clang_analyzer_eval(res); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
+  clang_analyzer_eval(x == &x); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
 }
Index: cfe/trunk/lib/Analysis/BodyFarm.cpp
===
--- cfe/trunk/lib/Analysis/BodyFarm.cpp
+++ cfe/trunk/lib/Analysis/BodyFarm.cpp
@@ -665,8 +665,6 @@
 }
 
 Stmt *BodyFarm::getBody(const FunctionDecl *D) {
-  D = D->getCanonicalDecl();
-
   Optional &Val = Bodies[D];
   if (Val.hasValue())
 return Val.getValue();
Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugRe

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

> Mmm, what are the pros and cons?

This is how we do it in the ASTImporter. It allows us to correctly handle 
redeclarations with and without bodies - the declarations in the current AST 
remain unchanged but a new declaration appears.  But it can be a kind of a 
professional deformation :) BodyFarm is a much lighter facility and I won't 
argue that my approach is better - just an idea.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60899



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

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D60899#1473412 , @a_sidorin wrote:

> the correct solution is to synthesize a shining new function and include it 
> into the redeclaration chain.


Mmm, what are the pros and cons?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60899



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

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Artem,
This looks good to me. However, it seems to me that the correct solution is to 
synthesize a shining new function and include it into the redeclaration chain. 
This requires much more work, however.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60899



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

[PATCH] D60899: [analyzer] Unbreak body farms in presence of multiple declarations.

[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, 
JDevlieghere, szepet, kristof.beyls, javed.absar.
Herald added a project: clang.
NoQ added a parent revision: D60808: [analyzer] pr41335: NoStoreFuncVisitor: 
Fix crash when no-store event is in a body-farmed function..

Split out of D60808 .

When growing a body on a body farm, it's essential to use the same 
redeclaration of the function that's going to be used during analysis. 
Otherwise our `ParmVarDecl`s won't match the ones that are used to identify 
argument regions. This boils down to trusting the reasoning in 
`AnalysisDeclContext`. We shouldn't canonicalize the declaration before farming 
the body because it makes us not obey the sophisticated decision-making process 
of `AnalysisDeclContext`.


Repository:
  rC Clang

https://reviews.llvm.org/D60899

Files:
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/OSAtomic_mac.c


Index: clang/test/Analysis/OSAtomic_mac.c
===
--- clang/test/Analysis/OSAtomic_mac.c
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -8,13 +8,20 @@
 }
 
 int *invalidSLocOnRedecl() {
-  int *b; // expected-note{{'b' declared without an initial value}}
-
+  // 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, &b); // no-crash
-  // FIXME: We don't really need these notes.
-  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
-  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+  return b;
+}
 
-  return b; // expected-warning{{Undefined or garbage value returned to 
caller}}
-// expected-note@-1{{Undefined or garbage value returned to 
caller}}
+void testThatItActuallyWorks() {
+  void *x = 0;
+  int res = OSAtomicCompareAndSwapPtrBarrier(0, &x, &x);
+  clang_analyzer_eval(res); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
+  clang_analyzer_eval(x == &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
@@ -579,6 +579,9 @@
 PathDiagnosticLocation L =
 PathDiagnosticLocation::create(N->getLocation(), SM);
 
+// For now this shouldn't trigger, but once it does (as we add more
+// functions to the body farm), we'll need to decide if these reports
+// are worth suppressing as well.
 if (!L.hasValidLocation())
   return nullptr;
 
Index: clang/lib/Analysis/BodyFarm.cpp
===
--- clang/lib/Analysis/BodyFarm.cpp
+++ clang/lib/Analysis/BodyFarm.cpp
@@ -665,8 +665,6 @@
 }
 
 Stmt *BodyFarm::getBody(const FunctionDecl *D) {
-  D = D->getCanonicalDecl();
-
   Optional &Val = Bodies[D];
   if (Val.hasValue())
 return Val.getValue();


Index: clang/test/Analysis/OSAtomic_mac.c
===
--- clang/test/Analysis/OSAtomic_mac.c
+++ clang/test/Analysis/OSAtomic_mac.c
@@ -8,13 +8,20 @@
 }
 
 int *invalidSLocOnRedecl() {
-  int *b; // expected-note{{'b' declared without an initial value}}
-
+  // 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, &b); // no-crash
-  // FIXME: We don't really need these notes.
-  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
-  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+  return b;
+}
 
-  return b; // expected-warning{{Undefined or garbage value returned to caller}}
-// expected-note@-1{{Undefined or garbage value returned to caller}}
+void testThatItActuallyWorks() {
+  void *x = 0;
+  int res = OSAtomicCompareAndSwapPtrBarrier(0, &x, &x);
+  clang_analyzer_eval(res); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
+  clang_analyzer_eval(x == &x); // expected-warning{{TRUE}}
+// expected-note@-1{{TRUE}}
 }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- c