[Clang][counted_by] Don't treat a __bdos argument as an array if it isn't

[bwendling]

If the __bdos argument isn't an array (e.g. a pointer), default
to using the 'llvm.objectsize' intrinsic, since we're not (yet)
able to calculate the size.

Fixes: cff0a46 ("[Clang][counted_by] Refactor __builtin_dynamic_object_size on FAMs (#122198)")
Signed-off-by: Bill Wendling [email protected]

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Bill Wendling (bwendling)

Changes

If the __bdos argument isn't an array (e.g. a pointer), due to
'-Wno-int-conversion' or some other shenanigans, default to using the
llvm.objectsize intrinsic.

Fixes: cff0a46 ("[Clang][counted_by] Refactor __builtin_dynamic_object_size on FAMs (#122198)")
Signed-off-by: Bill Wendling <[email protected]>

---

Full diff: https://github.com/llvm/llvm-project/pull/125298.diff

1 Files Affected:

• (modified) clang/lib/CodeGen/CGBuiltin.cpp (+24-6)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 11fa295dad9524..21faf85a16e2d2 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -1319,12 +1319,36 @@ CodeGenFunction::emitCountedByMemberSize(const Expr *E, llvm::Value *EmittedE,
 
   //  size_t field_offset = offsetof (struct s, field);
   Value *FieldOffset = nullptr;
+  llvm::ConstantInt *FieldBaseSize = nullptr;
   if (FlexibleArrayMemberFD != FD) {
     std::optional<int64_t> Offset = GetFieldOffset(Ctx, RD, FD);
     if (!Offset)
       return nullptr;
     FieldOffset =
         llvm::ConstantInt::get(ResType, *Offset / CharWidth, IsSigned);
+
+    if (Idx) {
+      // From option (4):
+      //  size_t field_base_size = sizeof (*ptr->field_array);
+      if (!FieldTy->isArrayType())
+        // The field isn't an array. For example:
+        //
+        //     struct {
+        //         int count;
+        //         char *string;
+        //         int array[] __counted_by(count);
+        //     } x;
+        //
+        //     __builtin_dynamic_object_size(x.string[42], 0);
+        //
+        // If built with '-Wno-int-conversion', FieldTy won't be an array here.
+        return nullptr;
+
+      const ArrayType *ArrayTy = Ctx.getAsArrayType(FieldTy);
+      CharUnits BaseSize = Ctx.getTypeSizeInChars(ArrayTy->getElementType());
+      FieldBaseSize =
+          llvm::ConstantInt::get(ResType, BaseSize.getQuantity(), IsSigned);
+    }
   }
 
   //  size_t count = (size_t) ptr->count;
@@ -1376,12 +1400,6 @@ CodeGenFunction::emitCountedByMemberSize(const Expr *E, llvm::Value *EmittedE,
         llvm::ConstantInt::get(ResType, Size.getKnownMinValue() / CharWidth);
 
     if (Idx) { // Option (4) '&ptr->field_array[idx]'
-      //  size_t field_base_size = sizeof (*ptr->field_array);
-      const ArrayType *ArrayTy = Ctx.getAsArrayType(FieldTy);
-      CharUnits BaseSize = Ctx.getTypeSizeInChars(ArrayTy->getElementType());
-      auto *FieldBaseSize =
-          llvm::ConstantInt::get(ResType, BaseSize.getQuantity(), IsSigned);
-
       //  field_offset += index * field_base_size;
       Value *Mul = Builder.CreateMul(Index, FieldBaseSize, "field_offset",
                                      !IsSigned, IsSigned);

[bwendling]

Note that the early placement of this code is so that no executable code is generated if we have to bail out.

[nathanchance]

This tentatively looks good to me. Do you need a reduced test case for this? cvise gave me:

typedef struct {
  char __padding[0];
} spinlock_t;
struct {
  int priv_len;
  spinlock_t addr_list_lock;
  char *dev_addr;
  char priv[] __attribute__((__counted_by__(priv_len)));
} falcon_reconfigure_xmac_core_efx;
void falcon_reconfigure_xmac_core() {
  long __q_size_field = __builtin_dynamic_object_size(
      &falcon_reconfigure_xmac_core_efx.dev_addr[4], 1);
  _Bool __ret_do_once = __q_size_field, __ret_cond = __ret_do_once;
  static _Bool __already_done;
  __ret_cond &&__already_done;
}

[bwendling]

I got a testcase from creduce. It's similar to your testcase. Basically, it's indexing into a pointer field, which isn't going to give the correct result either way. I expect that it's indexing into an array instead.

[bwendling]

Oops! I forgot to push the testcases :)

[efriedma-quic]

Sorry I didn't get around to re-reviewing #122198 earlier... it looks like this is the with lvalue-to-rvalue conversions I mentioned on the review. You do not want to look through lvalue-to-rvalue conversions; the StructFieldAccess visitor should bail out if it sees one.

[bwendling]

@efriedma-quic Thanks, I'll send a patch.

[bwendling]

@efriedma-quic Created #125571

[bwendling]

This was superseded by #125571.