[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

Wed Sep 13 11:25:31 PDT 2023

aaron.ballman requested changes to this revision.
aaron.ballman added reviewers: efriedma, rjmccall, erichkeane.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Thank you for working on this! Please be sure to also add a release note to `clang/docs/ReleaseNotes.rst` so users know about this. Also adding the codegen and attribute code owners for awareness.

================
Comment at: clang/include/clang/AST/Decl.h:4272-4275
+    FieldDecl *FD = nullptr;
+    for (FieldDecl *Field : fields())
+      FD = Field;
+    return FD;
----------------
Could this be implemented as: `return !field_empty() ? *std::prev(field_end()) : nullptr;` ? Then maybe someday we'll get better iterator support that isn't forward-only and this code will automagically get faster.

================
Comment at: clang/include/clang/AST/Decl.h:4277-4282
+  const FieldDecl *getLastField() const {
+    const FieldDecl *FD = nullptr;
+    for (const FieldDecl *Field : fields())
+      FD = Field;
+    return FD;
+  }
----------------
We use this pattern fairly often to avoid repeating the implementation.

================
Comment at: clang/include/clang/Basic/Attr.td:4246-4249
+      SourceRange countedByFieldLoc;
+    public:
+      SourceRange getCountedByFieldLoc(void) const { return countedByFieldLoc; }
+      void setCountedByFieldLoc(SourceRange Loc) { countedByFieldLoc = Loc; }
----------------
Teeny tiniest of coding style nits :-D

================
Comment at: clang/include/clang/Basic/Attr.td:4167
+  let Documentation = [ElementCountDocs];
+  let LangOpts = [COnly];
+}
----------------
void wrote:
> nickdesaulniers wrote:
> > Does C++ not support VLAs?
> C++ doesn't support flexible array members or VLAs.
Clang supports both as extensions in C++, as does GCC: https://godbolt.org/z/5xP7ncoha -- I think we should consider enabling this attribute in C++ mode, but I'm fine considering that in a follow-up.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:7200
+  let Content = [{
+Clang supports the ``counted_by`` attribute for the flexible array member of a
+structure. The argument for the attribute is the name of a field member in the
----------------
If we land the changes with the attribute as a C-only attribute, we should document that explicitly here (unless we're immediately enabling it C++ mode; not asking for busywork).

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6368-6369

+def warn_flexible_array_counted_by_attr_field_not_found : Warning<
+  "counted_by field %0 not found">;
+def err_flexible_array_counted_by_attr_refers_to_self : Error<
----------------
Why is this a warning and not a typical `err_no_member` error? I think we want that behavior so that we get typo correction for free. e.g.,
```
struct S {
  int Count;
  int FAM[] __attribute__((counted_by(Clount))); // Should get a "did you mean 'Count'" fix-it
};
```

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6371-6375
+  "counted_by field %0 cannot refer to the flexible array">;
+def err_flexible_array_counted_by_attr_field_not_integral : Error<
+  "counted_by field %0 is not an integral type">;
+def note_flexible_array_counted_by_attr_field : Note<
+  "counted_by field %0 declared here">;
----------------
We try to wrap syntax elements in single quotes in our diagnostics so it's more visually distinct

================
Comment at: clang/lib/AST/ASTImporter.cpp:8979-8980
+  // Useful for accessing the imported attribute.
+  template <typename T> T *getAttrAs() { return cast<T>(ToAttr); }
+  template <typename T> const T *getAttrAs() const { return cast<T>(ToAttr); }
+
----------------
Should these be `castAttrAs` so it's more clear that type mismatches don't result in a null pointer, but an assertion?

================
Comment at: clang/lib/AST/DeclBase.cpp:443-444
+
+  if (const auto *OID = dyn_cast_if_present<ObjCIvarDecl>(D))
+    return OID->getNextIvar() == nullptr;
+
----------------
Can you explain a bit about what this code is for? I didn't spot any test coverage for it, but perhaps I missed something.

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:857
+  if (IsDynamic) {
+    const LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
+        getLangOpts().getStrictFlexArraysLevel();
----------------
We don't generally use top-level const on local variables.

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:886
+      // At this point, we know that \p ME is a flexible array member.
+      const auto *ArrayTy = dyn_cast<ArrayType>(ME->getType());
+      unsigned Size = getContext().getTypeSize(ArrayTy->getElementType());
----------------

================
Comment at: clang/lib/Sema/SemaDecl.cpp:18000-18002
+    if (const auto *FD = dyn_cast<FieldDecl>(D))
+      if (Pred(FD))
+        return FD;
----------------
Ask and you shall receive! :-D

================
Comment at: clang/lib/Sema/SemaDecl.cpp:18012
+
+static void CheckCountedByAttr(Sema &S, const RecordDecl *RD,
+                               const FieldDecl *FD) {
----------------
This logic seems like it should live in SemaDeclAttr.cpp when handling the attribute. We're given the declaration the attribute is applied to, so we can do everything we need from that (the `FieldDecl` has a `getParent()` so we don't need the `RecordDecl` to be passed in. Is there a reason we need it to live here instead?

================
Comment at: clang/lib/Sema/SemaDecl.cpp:18035
+        << ECA->getCountedByField() << SR;
+  } else if (!Field->getType()->isIntegerType()) {
+    // The "counted_by" field must have an integer type.
----------------
Errr any integer type?
```
struct S {
  bool HerpADerp;
  int FAM[] __attribute__((counted_by(HerpADerp)));
};
```
seems like something we don't want, but I also wonder about enumeration types and more interestingly, plain `char` where it could be treated as signed or unsigned depending on compiler flags.

================
Comment at: clang/test/Misc/warning-flags.c:21

-CHECK: Warnings without flags (65):
+CHECK: Warnings without flags (66):

----------------
One line up: "The list of warnings below should NEVER grow.  It should gradually shrink to 0." ;-)

That said, I don't think this warning needs to be added because I think we have an existing error that is more appropriate.

================
Comment at: clang/test/Sema/attr-counted-by.c:1
+// RUN: %clang_cc1 -triple x86_64-linux -fstrict-flex-arrays=3 -fsanitize=array-bounds \
+// RUN:     -fsyntax-only -verify %s
----------------
Is the triple necessary? Also, do we have to enable `-fsanitize=array-bounds` to test this?

================
Comment at: clang/test/Sema/attr-counted-by.c:21
+  struct bar *fam[] __counted_by(non_integer); // expected-note {{counted_by field 'non_integer' declared here}}
+};
----------------
Please add test with the other strange FAM-like members, as well as one that is just a trailing constant array of size 2.

Some more tests or situations to consider:
```
struct S {
  struct {
    int NotInThisStruct;
  };
  int FAM[] __counted_by(NotInThisStruct); // Should this work?
};

int Global;
struct T {
  int FAM[] __counted_by(Global); // Should fail per your design but is that the behavior you want?
};

struct U {
  struct {
    int Count;
  } data;
  int FAM[] __counted_by(data.Count); // Thoughts?
};

struct V {
  int Sizes[2];
  int FAM[] __counted_by(Sizes[0]); // Thoughts?
};
```
(I'm not suggesting any of this needs to be accepted in order to land the patch.)

You should also have tests showing the attribute is diagnosed when applied to something other than a field, given something other than an identifier, is given zero arguments, and is given two arguments.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381