[Clang] Improve testing for the flexible array member

[bwendling]

Testing for the name of the flexible array member isn't as robust as testing the FieldDecl pointers.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Bill Wendling (bwendling)

Changes

Testing for the name of the flexible array member isn't as robust as testing the FieldDecl pointers.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGBuiltin.cpp (+12-9)
• (modified) clang/lib/CodeGen/CodeGenFunction.h (+6-6)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 4ab844d206e48a..0d2b5e1b5120ae 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -822,8 +822,9 @@ CodeGenFunction::evaluateOrEmitBuiltinObjectSize(const Expr *E, unsigned Type,
   return ConstantInt::get(ResType, ObjectSize, /*isSigned=*/true);
 }
 
-const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
-    ASTContext &Ctx, const RecordDecl *RD, StringRef Name, uint64_t &Offset) {
+const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberFieldAndOffset(
+    ASTContext &Ctx, const RecordDecl *RD, const FieldDecl *FAMDecl,
+    uint64_t &Offset) {
   const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
       getLangOpts().getStrictFlexArraysLevel();
   uint32_t FieldNo = 0;
@@ -832,7 +833,7 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
     return nullptr;
 
   for (const FieldDecl *FD : RD->fields()) {
-    if ((Name.empty() || FD->getNameAsString() == Name) &&
+    if ((!FAMDecl || FD == FAMDecl) &&
         Decl::isFlexibleArrayMemberLike(
             Ctx, FD, FD->getType(), StrictFlexArraysLevel,
             /*IgnoreTemplateOrMacroSubstitution=*/true)) {
@@ -843,8 +844,8 @@ const FieldDecl *CodeGenFunction::FindFlexibleArrayMemberField(
 
     QualType Ty = FD->getType();
     if (Ty->isRecordType()) {
-      if (const FieldDecl *Field = FindFlexibleArrayMemberField(
-              Ctx, Ty->getAsRecordDecl(), Name, Offset)) {
+      if (const FieldDecl *Field = FindFlexibleArrayMemberFieldAndOffset(
+              Ctx, Ty->getAsRecordDecl(), FAMDecl, Offset)) {
         const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(RD);
         Offset += Layout.getFieldOffset(FieldNo);
         return Field;
@@ -930,12 +931,12 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
 
   // Get the flexible array member Decl.
   const RecordDecl *OuterRD = nullptr;
-  std::string FAMName;
+  const FieldDecl *FAMDecl = nullptr;
   if (const auto *ME = dyn_cast<MemberExpr>(Base)) {
     // Check if \p Base is referencing the FAM itself.
     const ValueDecl *VD = ME->getMemberDecl();
     OuterRD = VD->getDeclContext()->getOuterLexicalRecordContext();
-    FAMName = VD->getNameAsString();
+    FAMDecl = dyn_cast<FieldDecl>(VD);
   } else if (const auto *DRE = dyn_cast<DeclRefExpr>(Base)) {
     // Check if we're pointing to the whole struct.
     QualType Ty = DRE->getDecl()->getType();
@@ -974,9 +975,11 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E, unsigned Type,
   if (!OuterRD)
     return nullptr;
 
+  // We call FindFlexibleArrayMemberAndOffset even if FAMDecl is non-null to
+  // get its offset.
   uint64_t Offset = 0;
-  const FieldDecl *FAMDecl =
-      FindFlexibleArrayMemberField(Ctx, OuterRD, FAMName, Offset);
+  FAMDecl =
+      FindFlexibleArrayMemberFieldAndOffset(Ctx, OuterRD, FAMDecl, Offset);
   Offset = Ctx.toCharUnitsFromBits(Offset).getQuantity();
 
   if (!FAMDecl || !FAMDecl->getType()->isCountAttributedType())
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index ff1873325d409f..a751649cdb597a 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -3204,12 +3204,12 @@ class CodeGenFunction : public CodeGenTypeCache {
                            llvm::Value *Index, QualType IndexType,
                            QualType IndexedType, bool Accessed);
 
-  // Find a struct's flexible array member. It may be embedded inside multiple
-  // sub-structs, but must still be the last field.
-  const FieldDecl *FindFlexibleArrayMemberField(ASTContext &Ctx,
-                                                const RecordDecl *RD,
-                                                StringRef Name,
-                                                uint64_t &Offset);
+  // Find a struct's flexible array member and get its offset. It may be
+  // embedded inside multiple sub-structs, but must still be the last field.
+  const FieldDecl *
+  FindFlexibleArrayMemberFieldAndOffset(ASTContext &Ctx, const RecordDecl *RD,
+                                        const FieldDecl *FAMDecl,
+                                        uint64_t &Offset);
 
   /// Find the FieldDecl specified in a FAM's "counted_by" attribute. Returns
   /// \p nullptr if either the attribute or the field doesn't exist.

[kees]

Does this still work for cases where there are multiple flexible arrays? e.g.

struct weird_protocol {
    unsigned int cmd_type;
    unsigned int data_len;
    union {
        struct cmd_one one[];
        struct cmd_two two[];
        struct cmd_three three[];
        unsigned char raw_bytes[];
    };
};

[bwendling]

Does this still work for cases where there are multiple flexible arrays? e.g.

struct weird_protocol {
    unsigned int cmd_type;
    unsigned int data_len;
    union {
        struct cmd_one one[];
        struct cmd_two two[];
        struct cmd_three three[];
        unsigned char raw_bytes[];
    };
};

Ah! One of your heresies. :-)

The current code probably doesn't support this (I'll check). This PR shouldn't change this. I'll have to revisit this code to make sure that it can be supported.

[efriedma-quic]

Strictly speaking, I don't think this is NFC. Consider, for example:

struct S {
  struct X { int array[1]; } x;
  struct Y { int count; int array[] __attribute__((__counted_by__(count))); } y;
};
void f(void* __attribute__((pass_dynamic_object_size(0))));
void g(struct S* s) { f(s->y.array); }

[bwendling]

Strictly speaking, I don't think this is NFC. Consider, for example:

struct S {
  struct X { int array[1]; } x;
  struct Y { int count; int array[] __attribute__((__counted_by__(count))); } y;
};
void f(void* __attribute__((pass_dynamic_object_size(0))));
void g(struct S* s) { f(s->y.array); }

Hmm.. Yeah, this change makes it more accurate. I'll remove the tag.

[bwendling]

Would changing the decl_iterator into a bidirectional iterator be acceptable? That way I could grab the last FieldDecl to check if it's a FAM rather than iterating through unnecessary Decls...

[efriedma-quic]

If you can pull it off without increasing the size of Decl, sure; not sure it's worthwhile otherwise.

[bwendling]

A quick look at Decl seems like it would require adding a PrevInContextAndBits, which would increase the size by sizeof(Decl *). I might be able to massage a way to make it work, but it's probably not worth it.

[efriedma-quic]

LGTM