[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:13981
+        bool overflow;
+        llvm::APInt product(index);
+        product += 1;
----------------
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.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13992
+      MaxElems =
+          MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+      MaxElems += 1;
----------------
Should this not be AddrBits + 1 if you want to add 1 below?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13993
+          MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+      MaxElems += 1;
+      if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
----------------
Though, why is the +1 here? Isn't this already the maximum number of elements?


================
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());
----------------
MaxElems should already be at a sufficient width here because of the earlier max. You can probably just do apElemBytes = apElemBytes.zextOrTrunc(MaxElems.getBitWidth())?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13966
   if (index.isUnsigned() || !index.isNegative()) {
-    // It is possible that the type of the base expression after
-    // IgnoreParenCasts is incomplete, even though the type of the base
-    // expression before IgnoreParenCasts is complete (see PR39746 for an
-    // example). In this case we have no information about whether the array
-    // access exceeds the array bounds. However we can still diagnose an array
-    // access which precedes the array bounds.
-    if (BaseType->isIncompleteType())
-      return;
+    if (isUnboundedArray) {
+      const auto &ASTC = getASTContext();
----------------
chrish_ericsson_atx wrote:
> ebevhan wrote:
> > It might simplify the patch to move this condition out of the tree and just early return for the other case. That is:
> > 
> > ```
> > if (isUnboundedArray) {
> >   if (!(index.isUnsigned() || !index.isNegative()))
> >     return;
> > 
> >   ...
> >   return;
> > }
> > 
> > if (index.isUnsigned() ...
> > ```
> There's a bit more code (starting at line 14094 in this patch set) that applies in all cases, so an early return here would prevent the "Array declared here" note from being generated.
Ah, the note.

I wonder if it wouldn't be cleaner (and avoid indenting the entire block) if that was just duplicated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796