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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 3 inline comments as done.
chrish_ericsson_atx added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13981
+        bool overflow;
+        llvm::APInt product(index);
+        product += 1;
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13992
+      MaxElems =
+          MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+      MaxElems += 1;
----------------
ebevhan wrote:
> Should this not be AddrBits + 1 if you want to add 1 below?
AddrBits + 1 would also work.  I chose AddrBits << 1 figuring that the width would end up being a nice clean power of 2, but that's not necessarily true.  Functionally, the effect of either approach is identical.  I suppose + 1 is a bit more obvious, though.  I'll make this change.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13993
+          MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+      MaxElems += 1;
+      if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
----------------
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.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:13994-13997
+      if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
+        MaxElems = MaxElems.zext(apElemBytes.getBitWidth());
+      else if (apElemBytes.getBitWidth() < MaxElems.getBitWidth())
+        apElemBytes = apElemBytes.zext(MaxElems.getBitWidth());
----------------
ebevhan wrote:
> MaxElems should already be at a sufficient width here because of the earlier max. You can probably just do apElemBytes = apElemBytes.zextOrTrunc(MaxElems.getBitWidth())?
Yep -- you are right.  index and apElemBytes are already guaranteed to have equal width (>= AddrBits), and MaxElems is guaranteed to have the same width or double width.  So the single zext of apElemBytes will do fine.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796