[libc-commits] [clang-tools-extra] [compiler-rt] [flang] [libcxx] [libc] [llvm] [clang] [Clang] Generate the GEP instead of adding AST nodes (PR #73730)

[John McCall via libc-commits]

================
@@ -4022,8 +4169,36 @@ LValue CodeGenFunction::EmitArraySubscriptExpr(const ArraySubscriptExpr *E,
       ArrayLV = EmitArraySubscriptExpr(ASE, /*Accessed*/ true);
     else
       ArrayLV = EmitLValue(Array);
+
     auto *Idx = EmitIdxAfterBase(/*Promote*/true);
 
+    if (SanOpts.has(SanitizerKind::ArrayBounds)) {
----------------
rjmccall wrote:

But *why* are you implementing a different rule here than we use for other uses of `__counted_by`?  That's an actively bad thing to do.

Look, if we have a use case that requires us to make `__counted_by` more flexible than it currently is, then okay, let's have a proper conversation about that and what the design ought to be.  The right place to do that is an RFC, not a code review thread.  Assuming we accept that proposal, though, this seems like the wrong way to implement it, because it's basically creating a bunch of ad hoc differences just for flexible array members.  There's no reason that `__counted_by` should follow different rules for flexible array members than it does for pointer members, at least on the read/bounds-checking side.  That means that we should be able to use common infrastructure between the two paths for getting back to the root pointer for the bound expression and evaluating it.  Your code here should just be invoking that common infrastructure.

So what I suggest is that you do this in three phases:
1. Make `__counted_by` work with flexible array members using the common infrastructure for `__counted_by`.
2. Get agreement on how we're going to generalize `__counted_by` and what the behavior ought to be.
3. Implement the new behavior for `__counted_by` in the common infrastructure for *all* its applications.

https://github.com/llvm/llvm-project/pull/73730