[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

[Balázs Benics via Phabricator via cfe-commits]

steakhal created this revision.
steakhal added reviewers: NoQ, vsavchenko, martong, baloghadamsoftware, ASDenysPetrov, xazax.hun, Szelethus.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.
Herald added a project: clang.
Harbormaster failed remote builds in B65978: Diff 281152!
Harbormaster returned this revision to the author for changes because remote builds failed.
steakhal requested review of this revision.
steakhal added a comment.

Manually force **request review**, to start a discussion about this.

Although I managed to implement this in a way to pass the tests, but the logic became somewhat messy, that's why I stick to this //dummy// patch.
I'm gonna clean that up and update the diff, if you think that such handing of memregions are valid.
Sorry for the `cfe-dev` spam, I did not know that I can manually //request review//.


Currently, if the analyzer evaluates an expression like `to - from`, it only computes reasonable answer if both are representing ElementRegions.

Formally, **for all** memory region `X` and **for all** SVal offset `Y`:
`&Element{SymRegion{X},Y,char} - &SymRegion{X}` => `Y`
The same for the symmetric version, but returning `-Y` instead.

I'm curious why don't we handle the case, when only one of the operands is an `ElementRegion` and the other is a plain `SymbolicRegion`.
IMO if the SuperMemoryRegion is the same, we can still return a reasonable answer.

In this patch, I suppose an extension to resolve this.
If this seems to be a good idea, I will:

- implement the symmetric version of the check
- add tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84736

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp


Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1017,6 +1017,21 @@
       }
     }
 
+    // Try to get the Index as Nonloc, then cast to ArrayIndexTy.
+    // Otherwise return None.
+    const auto MaybeNonLocIndexOfElementRegion =
+        [this](const ElementRegion *ElemReg) -> Optional<NonLoc> {
+      SVal IndexVal = ElemReg->getIndex();
+      Optional<NonLoc> Index = IndexVal.getAs<NonLoc>();
+      if (!Index)
+        return llvm::None;
+      IndexVal = evalCastFromNonLoc(*Index, ArrayIndexTy);
+      Index = IndexVal.getAs<NonLoc>();
+      if (!Index)
+        return llvm::None;
+      return Index.getValue();
+    };
+
     // Handle special cases for when both regions are element regions.
     const ElementRegion *RightER = dyn_cast<ElementRegion>(RightMR);
     const ElementRegion *LeftER = dyn_cast<ElementRegion>(LeftMR);
@@ -1027,31 +1042,32 @@
       // give the correct answer.
       if (LeftER->getSuperRegion() == RightER->getSuperRegion() &&
           LeftER->getElementType() == RightER->getElementType()) {
-        // Get the left index and cast it to the correct type.
-        // If the index is unknown or undefined, bail out here.
-        SVal LeftIndexVal = LeftER->getIndex();
-        Optional<NonLoc> LeftIndex = LeftIndexVal.getAs<NonLoc>();
-        if (!LeftIndex)
-          return UnknownVal();
-        LeftIndexVal = evalCastFromNonLoc(*LeftIndex, ArrayIndexTy);
-        LeftIndex = LeftIndexVal.getAs<NonLoc>();
-        if (!LeftIndex)
-          return UnknownVal();
 
-        // Do the same for the right index.
-        SVal RightIndexVal = RightER->getIndex();
-        Optional<NonLoc> RightIndex = RightIndexVal.getAs<NonLoc>();
-        if (!RightIndex)
-          return UnknownVal();
-        RightIndexVal = evalCastFromNonLoc(*RightIndex, ArrayIndexTy);
-        RightIndex = RightIndexVal.getAs<NonLoc>();
-        if (!RightIndex)
+        Optional<NonLoc> LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER);
+        Optional<NonLoc> RightIndex = MaybeNonLocIndexOfElementRegion(RightER);
+
+        if (!LeftIndex || !RightIndex)
           return UnknownVal();
 
         // Actually perform the operation.
         // evalBinOpNN expects the two indexes to already be the right type.
         return evalBinOpNN(state, op, *LeftIndex, *RightIndex, resultTy);
       }
+    } else if (LeftER && isa<SymbolicRegion>(RightMR)) {
+      if (LeftER->getSuperRegion() != RightMR)
+        return UnknownVal();
+      if (Optional<NonLoc> LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER))
+        return LeftIndex.getValue();
+      return UnknownVal();
+    } else if (RightER && isa<SymbolicRegion>(LeftMR)) {
+      if (RightER->getSuperRegion() != LeftMR)
+        return UnknownVal();
+      if (Optional<NonLoc> RightIndex =
+              MaybeNonLocIndexOfElementRegion(RightER))
+        llvm_unreachable(
+            "TODO: return the negated value of RightIndex - figure out how :D\n"
+            "Maybe a unary minus operator would do the job.");
+      return UnknownVal();
     }
 
     // Special handling of the FieldRegions, even with symbolic offsets.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84736.281152.patch
Type: text/x-patch
Size: 3341 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200728/0fcbbf40/attachment.bin>