[analyzer] NFC: Avoid element regions of void type.

[NoQ]

Summary

Add an assertion that ElementRegion's element type is not void.

Fix two violations of this rule that shown up on four of our existing test files.

I accidentally noticed that problem when i was looking at how c++-allocator-inlining mode handles int *x = new int(5) - apparently, it binds 5 to such broken region if a void pointer is returned by operator new(), which is a regression compared to the default mode. But this patch doesn't fix it (there were no tests for that).

[NoQ]

I accidentally noticed that problem

The actual problem was that RegionStore was unable to retrieve the binding to such ElementRegion (in case of this example, 5 S32b). There may be more problems with such regions. But at the same time i believe that the very idea of a void ElementRegion is frightening enough to be fixed on its own.

[xazax.hun]

I like the idea of adding those assertions but a bit worried about the other changes. Basically (if I get this right), we are masking the issues here and I am a bit afraid that they will get forgotten. I think it would be nice to at least add a FIXME that this path should never be triggered.

[NoQ]

I think the new behavior is correct in the sense that in our region hierarchy byte offsets (such as arithmetic on void pointers) are normally represented as char-type element regions. For instance, we have a similar mechanism is implemented in pointer casts case, when the byte offset of the pointer is not divisible by the casted object size: we just add a character element region to account for the remainder of the offset.

Like, it's not the situation when we couldn't figure out the type - it would have been null in that case. Here we know exactly that the type is void. And when you add N to a void pointer, it changes by N bytes, which is what we should, in my opinion, try to represent somehow. So i believe that the newly constructed SVals are correctly representing their respective expressions or arithmetic results, and i'm not immediately seeing a better behavior.

[xazax.hun]

In D40939#948252, @NoQ wrote:

Like, it's not the situation when we couldn't figure out the type - it would have been null in that case. Here we know exactly that the type is void.

Oh, thank you for the clarification!

[NoQ]

Rebase on top of D40584.

Stronger assertion for CXXThisRegion type. Uhm, forgot to mention that i also added a similar assertion to CXXThisRegion (which didn't find any bugs yet). Other typed value regions don't store types directly.

[dcoughlin]

Looks good to me, although I have a super nit about wording in a comment.

lib/StaticAnalyzer/Core/ExprEngine.cpp

2215

Super nit: Since we can't actually have lvalues for a forbidden lvalue type, we should instead call it a 'location' in the comment. "We still need to have sensible locations to represent this stuff".

[NoQ]

• Fix comments as suggested by Devin.
• Point out that arithmetic on void pointers is a GNU extension.

[NoQ]

@lebedev.ri wrote:

No tests?

This patch adds an assertion => Multiple existing tests immediately starts crashing => This patch fixes the crashes with the new functionality.

So in my understanding the new assertion is all the tests we need. Old tests didn't pass when this assertion was enforced, now they do. If somebody breaks the newly added functionality, the regression will immediately show up on tests by hitting the assertion.

Additionally, i'm adding actual practical tests (of how the analyzer's behavior actually change) in a follow-up patch (D41250). So if we merge these two patches (if necessary), we'd have new stuff in the tests directory.

Or i could try to come up with actual tests for this patch, but i'm not sure it's worth it given the above.

Please correct me if my logic is flawed or contradicts existing policies.

[NoQ]

I guess i'd commit it together with D41250 in a single commit, so that there obviously were tests.

[NoQ]

Rebase.

include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h

 ///  in a call to a C++ method.  This region doesn't represent the object
 ///  referred to by 'this', but rather 'this' itself.
 class CXXThisRegion : public TypedValueRegion {
   friend class MemRegionManager;
 
   CXXThisRegion(const PointerType *thisPointerTy,
                 const StackArgumentsSpaceRegion *sReg)
       : TypedValueRegion(sReg, CXXThisRegionKind),
-        ThisPointerTy(thisPointerTy) {}
+        ThisPointerTy(thisPointerTy) {
+    assert(ThisPointerTy->getPointeeType()->getAsCXXRecordDecl() &&
+           "Invalid region type!");
+  }
 
   static void ProfileRegion(llvm::FoldingSetNodeID &ID,
                             const PointerType *PT,
                             const MemRegion *sReg);
 
 public:
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
class ElementRegion : public TypedValueRegion {
   NonLoc Index;
 
   ElementRegion(QualType elementType, NonLoc Idx, const SubRegion *sReg)
       : TypedValueRegion(sReg, ElementRegionKind),
         ElementType(elementType), Index(Idx) {
     assert((!Idx.getAs<nonloc::ConcreteInt>() ||
             Idx.castAs<nonloc::ConcreteInt>().getValue().isSigned()) &&
            "The index must be signed");
+    assert(!elementType.isNull() && !elementType->isVoidType() &&
+           "Invalid region type!");
   }
 
   static void ProfileRegion(llvm::FoldingSetNodeID& ID, QualType elementType,
                             SVal Idx, const MemRegion* superRegion);
 
 public:
 
   NonLoc getIndex() const { return Index; }

lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::VisitArraySubscriptExpr(const ArraySubscriptExpr *A,
   bool IsGLValueLike = A->isGLValue() ||
     (A->getType().isCForbiddenLValueType() && !AMgr.getLangOpts().CPlusPlus);
 
   for (auto *Node : CheckerPreStmt) {
     const LocationContext *LCtx = Node->getLocationContext();
     ProgramStateRef state = Node->getState();
 
     if (IsGLValueLike) {
-      SVal V = state->getLValue(A->getType(),
+      QualType T = A->getType();
+
+      // One of the forbidden LValue types! We still need to have sensible
+      // symbolic locations to represent this stuff. Note that arithmetic on
+      // void pointers is a GCC extension.
+      if (T->isVoidType())
+        T = getContext().CharTy;
+
+      SVal V = state->getLValue(T,
                                 state->getSVal(Idx, LCtx),
                                 state->getSVal(Base, LCtx));
       Bldr.generateNode(A, Node, state->BindExpr(A, LCtx, V), nullptr,
           ProgramPoint::PostLValueKind);
     } else if (IsVectorType) {
       // FIXME: non-glvalue vector reads are not modelled.
       Bldr.generateNode(A, Node, state, nullptr);
     } else {
       llvm_unreachable("Array subscript should be an lValue when not \
 a vector and not a forbidden lvalue type");

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

else if (isa<SubRegion>(region)) {
       // TODO: Is this actually reliable? Maybe improving our MemRegion
       // hierarchy to provide typed regions for all non-void pointers would be
       // better. For instance, we cannot extend this towards LocAsInteger
       // operations, where result type of the expression is integer.
       if (resultTy->isAnyPointerType())
         elementType = resultTy->getPointeeType();
     }
 
+    // Represent arithmetic on void pointers as arithmetic on char pointers.
+    // It is fine when a TypedValueRegion of char value type represents
+    // a void pointer. Note that arithmetic on void pointers is a GCC extension.
+    if (elementType->isVoidType())
+      elementType = getContext().CharTy;
+
     if (Optional<NonLoc> indexV = index.getAs<NonLoc>()) {
       return loc::MemRegionVal(MemMgr.getElementRegion(elementType, *indexV,
                                                        superR, getContext()));
     }
   }
   return UnknownVal();
 }