[PATCH] D21453: Add support for attribute "overallocated"

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/Decl.h:3250
+  /// This is true if this struct ends with an array marked 'flexible_array'.
+  bool HasFlexibleArrayAttr : 1;
+
----------------
How is this different from `HasFlexibleArrayMember`? Do we really need both?


================
Comment at: include/clang/Basic/AttrDocs.td:2078-2079
+Structs or classes that have an array member marked ``flexible_array`` cannot be nested or subclassed.
+This attribute is useful when you want __builtin_object_size to be conservative
+when computing the size of an over-allocated array. For example:
+
----------------
Instead of "This attribute is useful when you want", I would suggest "This attribute causes", since that is far from the only effect. It should also disable the sanitizer checks for array bounds violations, for example.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2173
+def err_flexible_array_nested : Error<
+  "struct with a member marked 'flexible_array' cannot be nested">;
 def err_aligned_attribute_argument_not_int : Error<
----------------
I don't think it's very clear what "nested" means here. I assume you mean that a struct with a `flexible_array` member can't be used as the type of a field. If so, why not? C allows that for other kinds of flexible array members.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2520
+  "variables, functions, methods, types, enumerations, enumerators, labels, and non-static data members|"
+  "fields}1">,
   InGroup<IgnoredAttributes>;
----------------
aaron.ballman wrote:
> non-static data members instead of fields.
Again, this will be confusing in C. Our diagnostics generally use the word "field" to refer to fields in C or non-static data members in C++.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2170
+  "'flexible_array' attribute only applies to %select{"
+  "the last member of a struct|members of structs or classes|"
+  "fixed sized array members|array members that have at least one element}0">;
----------------
ahatanak wrote:
> aaron.ballman wrote:
> > I think "members of structs or classes" and "the last member of a struct" could be combined into "the last member of a non-union class", instead of using separate diagnostic text. What do you think?
> I agree, I think these two diagnostics should be combined.
We shouldn't talk about classes when targeting C.


================
Comment at: lib/AST/ExprConstant.cpp:287
         Entries.back().ArrayIndex += N;
         if (Entries.back().ArrayIndex > MostDerivedArraySize) {
           diagnosePointerArithmetic(Info, E, Entries.back().ArrayIndex);
----------------
Pointer arithmetic that leaves the initial portion of a member with the `flexible_array` attribute will lose its designator here. Is that what you want?


================
Comment at: lib/AST/ExprConstant.cpp:5200
+    } else
       Result.Designator.setInvalid();
     return true;
----------------
Should we also set `HasFlexibleArrayAttr` on *real* flexible array members (which will be fields with `IncompleteArrayType`) rather than invalidating the designator?


================
Comment at: lib/CodeGen/CGExpr.cpp:695-696
     if (const auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl())) {
-      RecordDecl::field_iterator FI(
-          DeclContext::decl_iterator(const_cast<FieldDecl *>(FD)));
-      return ++FI == FD->getParent()->field_end();
+      if (FD->getParent()->hasFlexibleArrayMemberOrAttr())
+        return true;
+      // For compatibility with existing code, we treat arrays of length 0 or
----------------
This is wrong. We shouldn't disable all bounds checking for all array members in a class just because it ends in a flexible array member: only that member should have its checks disabled. You can reverse the sense of this as a fast-path for the common case of a class without a flexible array member, though.


https://reviews.llvm.org/D21453