[PATCH] D16044: getDescriptiveName() for MemRegion

[Alexander Droste via cfe-commits]

Alexander_Droste added inline comments.

================
Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:653
@@ +652,3 @@
+    // name by calling 'getDescriptiveName' recursively.
+    else {
+      std::string Idx = ER->getDescriptiveName(false);
----------------
zaks.anna wrote:
> xazax.hun wrote:
> > Alexander_Droste wrote:
> > > I wasn't able to build a test case yet for which the analyzer could not determine the constant value. Is there a way to trick the analyzer so that the else case is used ? Then I could test for something like `'sendReq1[a][7][b]'`.
> > You can try use a value returned from a function that has an unknown body. E.g.:
> > 
> > int getUnknown();
> > 
> > void f() {
> >   int a = getUnKnown();
> > }
> What happens when you try 'sendReq1[a][7][b]'? Does it know the values for "a" and "b" for some reason? If 'a' would be an input parameter and the analyzer did not see a call site, it won't know the value of 'a'.
If the return value of a function is used for which the body is not known Clang crashes.

```
  int getUnknown(void);
  int idxA = getUnknown();
  MPI_Request sendReq1[10][10][10];
  MPI_Wait(&sendReq1[1][idxA][9], MPI_STATUS_IGNORE); // expected-warning{{Request 'sendReq1[1][7][9]' has no matching nonblocking call.}}
```
Clang also crashes if the index-variable is not initialized.

```
  int idxA;
  MPI_Request sendReq1[10][10][10];
  MPI_Wait(&sendReq1[1][idxA][9], MPI_STATUS_IGNORE); // expected-warning{{Request 'sendReq1[1][7][9]' has no matching nonblocking call.}}
```
In case the variable is initialized with a constant, the `ConcreteInt` is determined.

================
Comment at: test/Analysis/MemRegion.cpp:3
@@ +2,3 @@
+
+#include "MPIMock.h"
+
----------------
zaks.anna wrote:
> Alexander_Droste wrote:
> > The problem about these tests is that they introduce a cyclic commit dependency. MPI-Checker depends on `getDescriptiveName`. `getDescriptiveName` depends on MPI-Checker because of the tests.
> > 
> > Further, MPI-Checker depends on this function:
> > 
> > ```
> > SourceRange MemRegion::sourceRange() const {
> >   const VarRegion *const VR = dyn_cast<VarRegion>(this->getBaseRegion());
> >   const FieldRegion *const FR = dyn_cast<FieldRegion>(this);
> >   const CXXBaseObjectRegion*const COR = dyn_cast<CXXBaseObjectRegion>(this);
> > 
> >   // Check for more specific regions first.
> >   // FieldRegion
> >   if (FR) {
> >     return FR->getDecl()->getSourceRange();
> >   }
> >   // CXXBaseObjectRegion
> >   else if (COR) {
> >     return COR->getDecl()->getSourceRange();
> >   }
> >   // VarRegion
> >   else if (VR) {
> >     return VR->getDecl()->getSourceRange();
> >   }
> >   // Return invalid source range (can be checked by client).
> >   else {
> >     return SourceRange{};
> >   }
> > }
> > ```
> > Initially, my idea was to submit the `sourceRange` patch after `getDescriptiveName`. Maybe it would be most convenient, to include the `sourceRange` function into this patch and finally commit all connected patches in one go.
> > getDescriptiveName depends on MPI-Checker because of the tests.
> 
> Would committing the tests separately fix the  cyclic commit dependency problem? (Though, I'd still prefer to review them along with this patch.)
Yes, that would fix the cyclic commit dependency.


http://reviews.llvm.org/D16044