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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8841-8844
+def warn_array_index_exceeds_max_addressable_bounds : Warning<
+  "array index %0 refers past the last possible element for an array in %1-bit "
+  "address space containing %2-bit (%3-byte) elements (max possible %4 element%s5)">,
+  InGroup<ArrayBounds>;
----------------
chrish_ericsson_atx wrote:
> aaron.ballman wrote:
> > I'd combine this with the above diagnostic given how similar they are:
> > ```
> > def warn_exceeds_max_addressable_bounds : Warning<
> >   "%select{array index %1|the pointer incremented by %1}0 refers past the last possible element for an array in %2-bit "
> >   "address space containing %3-bit (%4-byte) elements (max possible %5 element%s6)">,
> >   InGroup<ArrayBounds>;
> > ```
> I was attempting to follow the pattern set by the preceding four definitions, which keep pointer math warnings and array index warnings separated, but are otherwise nearly identical.  If there's value in that pattern for those other warnings, I would assume the same value applies to keeping these separated.  Please re-ping if you disagree, otherwise, I'll leave these as two separate warnings.
I don't think there's a whole lot of value, but I also don't insist on the change.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:14063
+  if (isUnboundedArray) {
+    if (index.isUnsigned() || !index.isNegative()) {
+      const auto &ASTC = getASTContext();
----------------
chrish_ericsson_atx wrote:
> aaron.ballman wrote:
> > ebevhan wrote:
> > > This could be early return to avoid the indentation.
> > +1 to using early return here. I might even get rid of `isUnboundedArray` and just use `!BaseType`
> @ebevhan's comment about early return was actually addressed in a previous diff... I should have marked it as such.  My apologies.
> 
> I'd prefer to keep `isUnboundedArray` (with the case corrected, of course), as it seems much clearer than `!BaseType` in terms of expressing what we are actually checking here, and why.
That's fine by me, and my brain scrambled a bit -- I thought there was an early return possible for `!IsUnboundedArray` but I was looking at the wrong line.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:14100
+      // dependent CharUnits)
+      DiagRuntimeBehavior(BaseExpr->getBeginLoc(), BaseExpr,
+                          PDiag(DiagID)
----------------
chrish_ericsson_atx wrote:
> aaron.ballman wrote:
> > It's not clear to me whether we need to pay the extra expense of doing reachability checks for the diagnostic -- do you have evidence that there would be a high false positive rate if we always emit the diagnostic?
> Sorry-- I'm not following you here, @aaron.ballman. What reachability check are you referring to?  The diagnostic isn't conditioned on anything (at least not here, where your comment appears...), so I'm not sure what change you are suggesting that would "always emit the diagnostic"...  Sorry to be slow on the uptake here... Could you clarify that for me?
`DiagRuntimeBehavior()` does a reachability check on the given expression and will not emit the diagnostic if the code is not reachable. This check can be somewhat expensive and so it's usually only used when we need to eliminate false positives in common code patterns.

Instead of using `DiagRuntimeBehavior()`, you could use `Diag()` instead which doesn't do the reachability check. However, looking further at the changes, I think you're correct to use `DiagRuntimeBehavior()` -- we use it elsewhere in this same function. Sorry for the noise!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796