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



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

Reply via email to