[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:14063
+  if (isUnboundedArray) {
+    if (index.isUnsigned() || !index.isNegative()) {
+      const auto &ASTC = getASTContext();
----------------
This could be early return to avoid the indentation.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13981
+        bool overflow;
+        llvm::APInt product(index);
+        product += 1;
----------------
chrish_ericsson_atx wrote:
> ebevhan wrote:
> > What if index is wider than AddrBits, but the active bits are fewer? Then you might miss out on triggering the overflow case in the multiplication.
> Line 13984 checks for active bits of product being less than AddrBits, which is the same case (since product, by definition, has same width as index).  So I think this is covered.  If I've misunderstood, please re-ping.
The overflow limit for _ovf is determined by the width of the APInt. If index is 32 bits wide but only has 14 bits active, and AddrBits is 16, then an umul_ovf might overflow past 16 bits but not for 32 bits since the product is the same width as the index. Then we won't detect the overflow.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13993
+          MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+      MaxElems += 1;
+      if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
----------------
chrish_ericsson_atx wrote:
> ebevhan wrote:
> > Though, why is the +1 here? Isn't this already the maximum number of elements?
> Initial value of MaxElems is APInt::getMaxValue(AddrBits), which is the index of the last addressable CharUnit in the address space.  Adding 1 makes it the total number of addressable CharUnits in the address space, which is what we want as the numerator for computing total number of elements of a given size that will fit in that address space.
> 
Ah, yes. Got indexes and sizes mixed up again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796