Reland #90786 ([BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C) #93121
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
delcypher
added
the
clang:bounds-safety
delcypher
requested review from
kees,
AaronBallman,
bwendling,
erichkeane,
hnrklssn,
rapidsna and
patrykstefanski
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Dan Liew (delcypher) ChangesThis PR attempts to reland #90786 that was reverted due to a memory leak. This PR includes the following commits
The intention will be squash all the changes into a single commit once we've agreed to the memory leak fix. The Memory leakThe problem was that these lines parsed the attributes but then did nothing to free the memory assert(!getLangOpts().CPlusPlus);
for (auto *LateAttr : LateFieldAttrs)
ParseLexedCAttribute(*LateAttr);To fix this the existing A more principled fixed here would be to fix the ownership of the // Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
// Attributes in a class are parsed at the end of the class, along
// with other late-parsed declarations.
if (!ClassStack.empty() && !LateAttrs->parseSoon())
getCurrentClass().LateParsedDeclarations.push_back(LA);this means the ownership Patch is 63.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93121.diff 24 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 81e9d0423f96a..5d59b3b053600 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -317,7 +317,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental feature to
allow late parsing certain attributes in specific contexts where they would
- not normally be late parsed.
+ not normally be late parsed. Currently this allows late parsing the
+ `counted_by` attribute in C. See `Attribute Changes in Clang`_.
- ``-fseparate-named-sections`` uses separate unique sections for global
symbols in named special sections (i.e. symbols annotated with
@@ -406,6 +407,24 @@ Attribute Changes in Clang
- The ``clspv_libclc_builtin`` attribute has been added to allow clspv
(`OpenCL-C to Vulkan SPIR-V compiler <https://github.com/google/clspv>`_) to identify functions coming from libclc
(`OpenCL-C builtin library <https://libclc.llvm.org>`_).
+- The ``counted_by`` attribute is now allowed on pointers that are members of a
+ struct in C.
+
+- The ``counted_by`` attribute can now be late parsed in C when
+ ``-fexperimental-late-parse-attributes`` is passed but only when attribute is
+ used in the declaration attribute position. This allows using the
+ attribute on existing code where it previously impossible to do so without
+ re-ordering struct field declarations would break ABI as shown below.
+
+ .. code-block:: c
+
+ struct BufferTy {
+ /* Refering to `count` requires late parsing */
+ char* buffer __counted_by(count);
+ /* Swapping `buffer` and `count` to avoid late parsing would break ABI */
+ size_t count;
+ };
+
Improvements to Clang's diagnostics
-----------------------------------
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 9a5c6e8d562c3..263b632df23ce 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2515,6 +2515,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
bool isRecordType() const;
bool isClassType() const;
bool isStructureType() const;
+ bool isStructureTypeWithFlexibleArrayMember() const;
bool isObjCBoxableRecordType() const;
bool isInterfaceType() const;
bool isStructureOrClassType() const;
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 7008bea483c87..565708cef28ce 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -2256,7 +2256,8 @@ def TypeNullUnspecified : TypeAttr {
def CountedBy : DeclOrTypeAttr {
let Spellings = [Clang<"counted_by">];
let Subjects = SubjectList<[Field], ErrorDiag>;
- let Args = [ExprArgument<"Count">, IntArgument<"NestedLevel">];
+ let Args = [ExprArgument<"Count">, IntArgument<"NestedLevel", 1>];
+ let LateParsed = LateAttrParseExperimentalExt;
let ParseArgumentsAsUnevaluated = 1;
let Documentation = [CountedByDocs];
let LangOpts = [COnly];
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 4cb4f3d999f7a..4fad4d1a0eca7 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1447,6 +1447,10 @@ def FunctionMultiVersioning
def NoDeref : DiagGroup<"noderef">;
+// -fbounds-safety and bounds annotation related warnings
+def BoundsSafetyCountedByEltTyUnknownSize :
+ DiagGroup<"bounds-safety-counted-by-elt-type-unknown-size">;
+
// A group for cross translation unit static analysis related warnings.
def CrossTU : DiagGroup<"ctu">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c7dea1d54d063..0e73e0207766e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6544,8 +6544,10 @@ def warn_superclass_variable_sized_type_not_at_end : Warning<
def err_flexible_array_count_not_in_same_struct : Error<
"'counted_by' field %0 isn't within the same struct as the flexible array">;
-def err_counted_by_attr_not_on_flexible_array_member : Error<
- "'counted_by' only applies to C99 flexible array members">;
+def err_counted_by_attr_not_on_ptr_or_flexible_array_member : Error<
+ "'counted_by' only applies to pointers or C99 flexible array members">;
+def err_counted_by_attr_on_array_not_flexible_array_member : Error<
+ "'counted_by' on arrays only applies to C99 flexible array members">;
def err_counted_by_attr_refer_to_itself : Error<
"'counted_by' cannot refer to the flexible array member %0">;
def err_counted_by_must_be_in_structure : Error<
@@ -6560,6 +6562,23 @@ def err_counted_by_attr_refer_to_union : Error<
"'counted_by' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
+def err_counted_by_attr_pointee_unknown_size : Error<
+ "'counted_by' %select{cannot|should not}3 be applied to %select{"
+ "a pointer with pointee|" // pointer
+ "an array with element}0" // array
+ " of unknown size because %1 is %select{"
+ "an incomplete type|" // CountedByInvalidPointeeTypeKind::INCOMPLETE
+ "a sizeless type|" // CountedByInvalidPointeeTypeKind::SIZELESS
+ "a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
+ // CountedByInvalidPointeeTypeKind::FLEXIBLE_ARRAY_MEMBER
+ "a struct type with a flexible array member"
+ "%select{|. This will be an error in a future compiler version}3"
+ ""
+ "}2">;
+
+def warn_counted_by_attr_elt_type_unknown_size :
+ Warning<err_counted_by_attr_pointee_unknown_size.Summary>,
+ InGroup<BoundsSafetyCountedByEltTyUnknownSize>;
let CategoryName = "ARC Semantic Issue" in {
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 3c4ab649e3b4c..f5d990b400550 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1648,6 +1648,8 @@ class Parser : public CodeCompletionHandler {
bool EnterScope, bool OnDefinition);
void ParseLexedAttribute(LateParsedAttribute &LA,
bool EnterScope, bool OnDefinition);
+ void ParseLexedCAttribute(LateParsedAttribute &LA,
+ ParsedAttributes *OutAttrs = nullptr);
void ParseLexedMethodDeclarations(ParsingClass &Class);
void ParseLexedMethodDeclaration(LateParsedMethodDeclaration &LM);
void ParseLexedMethodDefs(ParsingClass &Class);
@@ -2534,7 +2536,8 @@ class Parser : public CodeCompletionHandler {
void ParseStructDeclaration(
ParsingDeclSpec &DS,
- llvm::function_ref<void(ParsingFieldDeclarator &)> FieldsCallback);
+ llvm::function_ref<Decl *(ParsingFieldDeclarator &)> FieldsCallback,
+ LateParsedAttrList *LateFieldAttrs = nullptr);
DeclGroupPtrTy ParseTopLevelStmtDecl();
@@ -3112,6 +3115,8 @@ class Parser : public CodeCompletionHandler {
SourceLocation ScopeLoc,
ParsedAttr::Form Form);
+ void DistributeCLateParsedAttrs(Decl *Dcl, LateParsedAttrList *LateAttrs);
+
void ParseBoundsAttribute(IdentifierInfo &AttrName,
SourceLocation AttrNameLoc, ParsedAttributes &Attrs,
IdentifierInfo *ScopeName, SourceLocation ScopeLoc,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 01ddba5eaf01d..a37e22055d852 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -11381,7 +11381,8 @@ class Sema final : public SemaBase {
QualType BuildMatrixType(QualType T, Expr *NumRows, Expr *NumColumns,
SourceLocation AttrLoc);
- QualType BuildCountAttributedArrayType(QualType WrappedTy, Expr *CountExpr);
+ QualType BuildCountAttributedArrayOrPointerType(QualType WrappedTy,
+ Expr *CountExpr);
QualType BuildAddressSpaceAttr(QualType &T, LangAS ASIdx, Expr *AddrSpace,
SourceLocation AttrLoc);
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 3b90b8229dd18..04f105c128872 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -632,6 +632,16 @@ bool Type::isStructureType() const {
return false;
}
+bool Type::isStructureTypeWithFlexibleArrayMember() const {
+ const auto *RT = getAs<RecordType>();
+ if (!RT)
+ return false;
+ const auto *Decl = RT->getDecl();
+ if (!Decl->isStruct())
+ return false;
+ return Decl->hasFlexibleArrayMember();
+}
+
bool Type::isObjCBoxableRecordType() const {
if (const auto *RT = getAs<RecordType>())
return RT->getDecl()->hasAttr<ObjCBoxableAttr>();
diff --git a/clang/lib/Parse/ParseCXXInlineMethods.cpp b/clang/lib/Parse/ParseCXXInlineMethods.cpp
index 943ce0fdde3a3..e1dfd572d21a7 100644
--- a/clang/lib/Parse/ParseCXXInlineMethods.cpp
+++ b/clang/lib/Parse/ParseCXXInlineMethods.cpp
@@ -744,7 +744,13 @@ void Parser::ParseLexedAttributeList(LateParsedAttrList &LAs, Decl *D,
for (unsigned i = 0, ni = LAs.size(); i < ni; ++i) {
if (D)
LAs[i]->addDecl(D);
- ParseLexedAttribute(*LAs[i], EnterScope, OnDefinition);
+ // FIXME: There has to be a better way to ask "is this C?""
+ if (LangStandard::getLangStandardForKind(getLangOpts().LangStd)
+ .getLanguage() == Language::C) {
+ // TODO: Use `EnterScope`
+ ParseLexedCAttribute(*LAs[i]);
+ } else
+ ParseLexedAttribute(*LAs[i], EnterScope, OnDefinition);
delete LAs[i];
}
LAs.clear();
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 445d3fd66e387..36e130a3bb6cb 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -3306,6 +3306,19 @@ void Parser::ParseAlignmentSpecifier(ParsedAttributes &Attrs,
}
}
+void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
+ LateParsedAttrList *LateAttrs) {
+ assert(Dcl && "Dcl cannot be null");
+
+ if (!LateAttrs)
+ return;
+
+ for (auto *LateAttr : *LateAttrs) {
+ if (LateAttr->Decls.empty())
+ LateAttr->addDecl(Dcl);
+ }
+}
+
/// Bounds attributes (e.g., counted_by):
/// AttrName '(' expression ')'
void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName,
@@ -4843,13 +4856,14 @@ static void DiagnoseCountAttributedTypeInUnnamedAnon(ParsingDeclSpec &DS,
///
void Parser::ParseStructDeclaration(
ParsingDeclSpec &DS,
- llvm::function_ref<void(ParsingFieldDeclarator &)> FieldsCallback) {
+ llvm::function_ref<Decl *(ParsingFieldDeclarator &)> FieldsCallback,
+ LateParsedAttrList *LateFieldAttrs) {
if (Tok.is(tok::kw___extension__)) {
// __extension__ silences extension warnings in the subexpression.
ExtensionRAIIObject O(Diags); // Use RAII to do this.
ConsumeToken();
- return ParseStructDeclaration(DS, FieldsCallback);
+ return ParseStructDeclaration(DS, FieldsCallback, LateFieldAttrs);
}
// Parse leading attributes.
@@ -4914,10 +4928,12 @@ void Parser::ParseStructDeclaration(
}
// If attributes exist after the declarator, parse them.
- MaybeParseGNUAttributes(DeclaratorInfo.D);
+ MaybeParseGNUAttributes(DeclaratorInfo.D, LateFieldAttrs);
// We're done with this declarator; invoke the callback.
- FieldsCallback(DeclaratorInfo);
+ Decl *Field = FieldsCallback(DeclaratorInfo);
+ if (Field)
+ DistributeCLateParsedAttrs(Field, LateFieldAttrs);
// If we don't have a comma, it is either the end of the list (a ';')
// or an error, bail out.
@@ -4928,6 +4944,69 @@ void Parser::ParseStructDeclaration(
}
}
+/// Finish parsing an attribute for which parsing was delayed.
+/// This will be called at the end of parsing a class declaration
+/// for each LateParsedAttribute. We consume the saved tokens and
+/// create an attribute with the arguments filled in. We add this
+/// to the Attribute list for the decl.
+void Parser::ParseLexedCAttribute(LateParsedAttribute &LA,
+ ParsedAttributes *OutAttrs) {
+ // Create a fake EOF so that attribute parsing won't go off the end of the
+ // attribute.
+ Token AttrEnd;
+ AttrEnd.startToken();
+ AttrEnd.setKind(tok::eof);
+ AttrEnd.setLocation(Tok.getLocation());
+ AttrEnd.setEofData(LA.Toks.data());
+ LA.Toks.push_back(AttrEnd);
+
+ // Append the current token at the end of the new token stream so that it
+ // doesn't get lost.
+ LA.Toks.push_back(Tok);
+ PP.EnterTokenStream(LA.Toks, /*DisableMacroExpansion=*/true,
+ /*IsReinject=*/true);
+ // Drop the current token and bring the first cached one. It's the same token
+ // as when we entered this function.
+ ConsumeAnyToken(/*ConsumeCodeCompletionTok=*/true);
+
+ ParsedAttributes Attrs(AttrFactory);
+
+ assert(LA.Decls.size() <= 1 &&
+ "late field attribute expects to have at most one declaration.");
+
+ // Dispatch based on the attribute and parse it
+ const AttributeCommonInfo::Form ParsedForm = ParsedAttr::Form::GNU();
+ IdentifierInfo *ScopeName = nullptr;
+ const ParsedAttr::Kind AttrKind =
+ ParsedAttr::getParsedKind(&LA.AttrName, /*ScopeName=*/ScopeName,
+ /*SyntaxUsed=*/ParsedForm.getSyntax());
+ switch (AttrKind) {
+ case ParsedAttr::Kind::AT_CountedBy:
+ ParseBoundsAttribute(LA.AttrName, LA.AttrNameLoc, Attrs,
+ /*ScopeName=*/ScopeName, SourceLocation(),
+ /*Form=*/ParsedForm);
+ break;
+ default:
+ llvm_unreachable("Unhandled late parsed attribute");
+ }
+
+ for (auto *D : LA.Decls)
+ Actions.ActOnFinishDelayedAttribute(getCurScope(), D, Attrs);
+
+ // Due to a parsing error, we either went over the cached tokens or
+ // there are still cached tokens left, so we skip the leftover tokens.
+ while (Tok.isNot(tok::eof))
+ ConsumeAnyToken();
+
+ // Consume the fake EOF token if it's there
+ if (Tok.is(tok::eof) && Tok.getEofData() == AttrEnd.getEofData())
+ ConsumeAnyToken();
+
+ if (OutAttrs) {
+ OutAttrs->takeAllFrom(Attrs);
+ }
+}
+
/// ParseStructUnionBody
/// struct-contents:
/// struct-declaration-list
@@ -4951,6 +5030,11 @@ void Parser::ParseStructUnionBody(SourceLocation RecordLoc,
ParseScope StructScope(this, Scope::ClassScope|Scope::DeclScope);
Actions.ActOnTagStartDefinition(getCurScope(), TagDecl);
+ // `LateAttrParseExperimentalExtOnly=true` requests that only attributes
+ // marked with `LateAttrParseExperimentalExt` are late parsed.
+ LateParsedAttrList LateFieldAttrs(/*PSoon=*/true,
+ /*LateAttrParseExperimentalExtOnly=*/true);
+
// While we still have something to read, read the declarations in the struct.
while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
Tok.isNot(tok::eof)) {
@@ -5001,18 +5085,19 @@ void Parser::ParseStructUnionBody(SourceLocation RecordLoc,
}
if (!Tok.is(tok::at)) {
- auto CFieldCallback = [&](ParsingFieldDeclarator &FD) {
+ auto CFieldCallback = [&](ParsingFieldDeclarator &FD) -> Decl * {
// Install the declarator into the current TagDecl.
Decl *Field =
Actions.ActOnField(getCurScope(), TagDecl,
FD.D.getDeclSpec().getSourceRange().getBegin(),
FD.D, FD.BitfieldSize);
FD.complete(Field);
+ return Field;
};
// Parse all the comma separated declarators.
ParsingDeclSpec DS(*this);
- ParseStructDeclaration(DS, CFieldCallback);
+ ParseStructDeclaration(DS, CFieldCallback, &LateFieldAttrs);
} else { // Handle @defs
ConsumeToken();
if (!Tok.isObjCAtKeyword(tok::objc_defs)) {
@@ -5053,7 +5138,12 @@ void Parser::ParseStructUnionBody(SourceLocation RecordLoc,
ParsedAttributes attrs(AttrFactory);
// If attributes exist after struct contents, parse them.
- MaybeParseGNUAttributes(attrs);
+ MaybeParseGNUAttributes(attrs, &LateFieldAttrs);
+
+ // Late parse field attributes if necessary.
+ assert(!getLangOpts().CPlusPlus);
+ ParseLexedAttributeList(LateFieldAttrs, /*Decl=*/nullptr,
+ /*EnterScope=*/false, /*OnDefinition=*/false);
SmallVector<Decl *, 32> FieldDecls(TagDecl->fields());
diff --git a/clang/lib/Parse/ParseObjc.cpp b/clang/lib/Parse/ParseObjc.cpp
index 89f4acbd25e49..6a2088a73c55b 100644
--- a/clang/lib/Parse/ParseObjc.cpp
+++ b/clang/lib/Parse/ParseObjc.cpp
@@ -780,16 +780,16 @@ void Parser::ParseObjCInterfaceDeclList(tok::ObjCKeywordKind contextKey,
}
bool addedToDeclSpec = false;
- auto ObjCPropertyCallback = [&](ParsingFieldDeclarator &FD) {
+ auto ObjCPropertyCallback = [&](ParsingFieldDeclarator &FD) -> Decl * {
if (FD.D.getIdentifier() == nullptr) {
Diag(AtLoc, diag::err_objc_property_requires_field_name)
<< FD.D.getSourceRange();
- return;
+ return nullptr;
}
if (FD.BitfieldSize) {
Diag(AtLoc, diag::err_objc_property_bitfield)
<< FD.D.getSourceRange();
- return;
+ return nullptr;
}
// Map a nullability property attribute to a context-sensitive keyword
@@ -818,6 +818,7 @@ void Parser::ParseObjCInterfaceDeclList(tok::ObjCKeywordKind contextKey,
MethodImplKind);
FD.complete(Property);
+ return Property;
};
// Parse all the comma separated declarators.
@@ -2013,7 +2014,7 @@ void Parser::ParseObjCClassInstanceVariables(ObjCContainerDecl *interfaceDecl,
continue;
}
- auto ObjCIvarCallback = [&](ParsingFieldDeclarator &FD) {
+ auto ObjCIvarCallback = [&](ParsingFieldDeclarator &FD) -> Decl * {
assert(getObjCDeclContext() == interfaceDecl &&
"Ivar should have interfaceDecl as its decl context");
// Install the declarator into the interface decl.
@@ -2024,6 +2025,7 @@ void Parser::ParseObjCClassInstanceVariables(ObjCContainerDecl *interfaceDecl,
if (Field)
AllIvarDecls.push_back(Field);
FD.complete(Field);
+ return Field;
};
// Parse all the comma separated declarators.
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index ca5938083917f..5041fd65286fa 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -8663,31 +8663,95 @@ static const RecordDecl *GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
return RD;
}
-static bool
-CheckCountExpr(Sema &S, FieldDecl *FD, Expr *E,
- llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls) {
+enum class CountedByInvalidPointeeTypeKind {
+ INCOMPLETE,
+ SIZELESS,
+ FUNCTION,
+ FLEXIBLE_ARRAY_MEMBER,
+ VALID,
+};
+
+static bool CheckCountedByAttrOnField(
+ Sema &S, FieldDecl *FD, Expr *E,
+ llvm::SmallVectorImpl<TypeCoupledDeclRefInfo> &Decls) {
+ // Check the context the attribute is used in
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
<< FD->getSourceRange();
return true;
}
- if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
- S.Diag(E->getBeginLoc(), diag::err_counted_by_attr_argument_not_integer)
- << E->getSourceRange();
+ const auto FieldTy = FD->getType();
+ if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
+ S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
+ << FD->getLocation();
return true;
}
LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
LangOptions::StrictFlexArraysLevelKind::IncompleteOnly;
-
- if (!Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FD->getType(),
+ if (FieldTy->isArrayType() &&
+ !Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FieldTy,
StrictFlexArraysLevel, true)) {
- // The "counted_by" attribute must be on a flexible array member.
- SourceRange SR = FD->getLocation();
- S.Diag(SR.getBegin(),
- diag::err_counted_by_attr_not_on_flexible_array_member)
- << SR;
+ S.Diag(FD->getBeginLoc(),
+ diag::err_c...
[truncated]
|
delcypher
commented
May 23, 2024
| @@ -744,7 +744,13 @@ void Parser::ParseLexedAttributeList(LateParsedAttrList &LAs, Decl *D, | |||
| for (unsigned i = 0, ni = LAs.size(); i < ni; ++i) { | |||
| if (D) | |||
| LAs[i]->addDecl(D); | |||
| ParseLexedAttribute(*LAs[i], EnterScope, OnDefinition); | |||
| // FIXME: There has to be a better way to ask "is this C?"" | |||
| if (LangStandard::getLangStandardForKind(getLangOpts().LangStd) | |||
There was a problem hiding this comment.
@AaronBallman @erichkeane Is there a better way to check that we're building for C?
|
Hmm looks like I broke Let's see if I can figure out what I did... |
|
Okay this is interesting... This means I'm going to redo the memory leak patch to not use |
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing_v2
branch
from
604e274 to
671dd73
Compare
delcypher
commented
May 23, 2024
clang/lib/Parse/ParseDecl.cpp
Outdated
| @@ -4993,20 +4993,8 @@ void Parser::ParseLexedCAttribute(LateParsedAttribute &LA, | |||
| "late field attribute expects to have at most one declaration."); | |||
|
|
|||
| // Dispatch based on the attribute and parse it | |||
| const AttributeCommonInfo::Form ParsedForm = ParsedAttr::Form::GNU(); | |||
|
Hmm looks like I'll have to tweak the assert. Looks like some unit tests don't specify the language. |
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing_v2
branch
from
a643967 to
3feb18b
Compare
rapidsna
approved these changes
May 23, 2024
| int count; | ||
| // Treating this as a warning is a temporary fix for existing attribute adopters. It **SHOULD BE AN ERROR**. | ||
| // expected-warning@+1{{'counted_by' should not be applied to an array with element of unknown size because 'struct has_unannotated_VLA' is a struct type with a flexible array member. This will be an error in a future compiler version}} | ||
| struct has_unannotated_VLA Arr[] __counted_by(count); |
There was a problem hiding this comment.
@bwendling Just to be clear. I'm not sure if Linux builds treat warnings as errors by default. If so, they would still need to do things so as not to promote it as an error in order to have the builds succeed.
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing_v2
branch
from
3feb18b to
bc5b8bc
Compare
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing_v2
branch
from
bc5b8bc to
f9c8abd
Compare
delcypher
added a commit
to delcypher/apple-llvm-project
that referenced
this pull request
May 23, 2024
…ters in structs in C (llvm#93121) Fixes llvm#92687. Previously the attribute was only allowed on flexible array members. This patch patch changes this to also allow the attribute on pointer fields in structs and also allows late parsing of the attribute in some contexts. For example this previously wasn't allowed: ``` struct BufferTypeDeclAttributePosition { size_t count; char* buffer __counted_by(count); // Now allowed } ``` Note the attribute is prevented on pointee types where the size isn't known at compile time. In particular pointee types that are: * Incomplete (e.g. `void`) and sizeless types * Function types (e.g. the pointee of a function pointer) * Struct types with a flexible array member This patch also introduces late parsing of the attribute when used in the declaration attribute position. For example ``` struct BufferTypeDeclAttributePosition { char* buffer __counted_by(count); // Now allowed size_t count; } ``` is now allowed but **only** when passing `-fexperimental-late-parse-attributes`. The motivation for using late parsing here is to avoid breaking the data layout of structs in existing code that want to use the `counted_by` attribute. This patch is the first use of `LateAttrParseExperimentalExt` in `Attr.td` that was introduced in a previous patch. Note by allowing the attribute on struct member pointers this now allows the possiblity of writing the attribute in the type attribute position. For example: ``` struct BufferTypeAttributePosition { size_t count; char *__counted_by(count) buffer; // Now allowed } ``` However, the attribute in this position is still currently parsed immediately rather than late parsed. So this will not parse currently: ``` struct BufferTypeAttributePosition { char *__counted_by(count) buffer; // Fails to parse size_t count; } ``` The intention is to lift this restriction in future patches. It has not been done in this patch to keep this size of this commit small. There are also several other follow up changes that will need to be addressed in future patches: * Make late parsing working with anonymous structs (see `on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`). * Allow `counted_by` on more subjects (e.g. parameters, returns types) when `-fbounds-safety` is enabled. * Make use of the attribute on pointer types in code gen (e.g. for `_builtin_dynamic_object_size` and UBSan's array-bounds checks). This work is heavily based on a patch originally written by Yeoul Na. ** Differences between llvm#93121 and this patch ** * The memory leak that caused llvm#93121 to be reverted (see llvm#92687) should now be fixed. See "The Memory Leak". * The fix to `pragma-attribute-supported-attributes-list.test` (originally in cef6387) has been incorporated into this patch. * A relaxation of counted_by semantics (originally in 112eadd) has been incorporated into this patch. * The assert in `Parser::DistributeCLateParsedAttrs` has been removed because that broke downstream code. * The switch statement in `Parser::ParseLexedCAttribute` has been removed in favor of using `Parser::ParseGNUAttributeArgs` which does the same thing but is more feature complete. * The `EnterScope` parameter has been plumbed through `Parser::ParseLexedCAttribute` and `Parser::ParseLexedCAttributeList`. It currently doesn't do anything but it will be needed in future commits. ** The Memory Leak ** The problem was that these lines parsed the attributes but then did nothing to free the memory ``` assert(!getLangOpts().CPlusPlus); for (auto *LateAttr : LateFieldAttrs) ParseLexedCAttribute(*LateAttr); ``` To fix this this a new `Parser::ParseLexedCAttributeList` method has been added (based on `Parser::ParseLexedAttributeList`) which does the necessary memory management. The intention is to merge these two methods together so there is just one implementation in a future patch. A more principled fixed here would be to fix the ownership of the `LateParsedAttribute` objects. In principle `LateParsedAttrList` should own its pointers exclusively and be responsible for deallocating them. Unfortunately this is complicated by `LateParsedAttribute` objects also being stored in another data structure (`LateParsedDeclarations`) as can be seen below (`LA` gets stored in two places). ``` // Handle attributes with arguments that require late parsing. LateParsedAttribute *LA = new LateParsedAttribute(this, *AttrName, AttrNameLoc); LateAttrs->push_back(LA); // Attributes in a class are parsed at the end of the class, along // with other late-parsed declarations. if (!ClassStack.empty() && !LateAttrs->parseSoon()) getCurrentClass().LateParsedDeclarations.push_back(LA); ``` this means the ownership of LateParsedAttribute objects isn't very clear. rdar://125400257
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing_v2
branch
from
f9c8abd to
9e1a9e4
Compare
|
Filed to #93263 to track the potential refactor. |
…ters in structs in C (llvm#93121) Fixes llvm#92687. Previously the attribute was only allowed on flexible array members. This patch patch changes this to also allow the attribute on pointer fields in structs and also allows late parsing of the attribute in some contexts. For example this previously wasn't allowed: ``` struct BufferTypeDeclAttributePosition { size_t count; char* buffer __counted_by(count); // Now allowed } ``` Note the attribute is prevented on pointee types where the size isn't known at compile time. In particular pointee types that are: * Incomplete (e.g. `void`) and sizeless types * Function types (e.g. the pointee of a function pointer) * Struct types with a flexible array member This patch also introduces late parsing of the attribute when used in the declaration attribute position. For example ``` struct BufferTypeDeclAttributePosition { char* buffer __counted_by(count); // Now allowed size_t count; } ``` is now allowed but **only** when passing `-fexperimental-late-parse-attributes`. The motivation for using late parsing here is to avoid breaking the data layout of structs in existing code that want to use the `counted_by` attribute. This patch is the first use of `LateAttrParseExperimentalExt` in `Attr.td` that was introduced in a previous patch. Note by allowing the attribute on struct member pointers this now allows the possiblity of writing the attribute in the type attribute position. For example: ``` struct BufferTypeAttributePosition { size_t count; char *__counted_by(count) buffer; // Now allowed } ``` However, the attribute in this position is still currently parsed immediately rather than late parsed. So this will not parse currently: ``` struct BufferTypeAttributePosition { char *__counted_by(count) buffer; // Fails to parse size_t count; } ``` The intention is to lift this restriction in future patches. It has not been done in this patch to keep this size of this commit small. There are also several other follow up changes that will need to be addressed in future patches: * Make late parsing working with anonymous structs (see `on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`). * Allow `counted_by` on more subjects (e.g. parameters, returns types) when `-fbounds-safety` is enabled. * Make use of the attribute on pointer types in code gen (e.g. for `_builtin_dynamic_object_size` and UBSan's array-bounds checks). This work is heavily based on a patch originally written by Yeoul Na. ** Differences between llvm#93121 and this patch ** * The memory leak that caused llvm#93121 to be reverted (see llvm#92687) should now be fixed. See "The Memory Leak". * The fix to `pragma-attribute-supported-attributes-list.test` (originally in cef6387) has been incorporated into this patch. * A relaxation of counted_by semantics (originally in 112eadd) has been incorporated into this patch. * The assert in `Parser::DistributeCLateParsedAttrs` has been removed because that broke downstream code. * The switch statement in `Parser::ParseLexedCAttribute` has been removed in favor of using `Parser::ParseGNUAttributeArgs` which does the same thing but is more feature complete. * The `EnterScope` parameter has been plumbed through `Parser::ParseLexedCAttribute` and `Parser::ParseLexedCAttributeList`. It currently doesn't do anything but it will be needed in future commits. ** The Memory Leak ** The problem was that these lines parsed the attributes but then did nothing to free the memory ``` assert(!getLangOpts().CPlusPlus); for (auto *LateAttr : LateFieldAttrs) ParseLexedCAttribute(*LateAttr); ``` To fix this this a new `Parser::ParseLexedCAttributeList` method has been added (based on `Parser::ParseLexedAttributeList`) which does the necessary memory management. The intention is to merge these two methods together so there is just one implementation in a future patch (llvm#93263). A more principled fixed here would be to fix the ownership of the `LateParsedAttribute` objects. In principle `LateParsedAttrList` should own its pointers exclusively and be responsible for deallocating them. Unfortunately this is complicated by `LateParsedAttribute` objects also being stored in another data structure (`LateParsedDeclarations`) as can be seen below (`LA` gets stored in two places). ``` // Handle attributes with arguments that require late parsing. LateParsedAttribute *LA = new LateParsedAttribute(this, *AttrName, AttrNameLoc); LateAttrs->push_back(LA); // Attributes in a class are parsed at the end of the class, along // with other late-parsed declarations. if (!ClassStack.empty() && !LateAttrs->parseSoon()) getCurrentClass().LateParsedDeclarations.push_back(LA); ``` this means the ownership of LateParsedAttribute objects isn't very clear. rdar://125400257
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing_v2
branch
from
9e1a9e4 to
c1f0914
Compare
fmayer
reviewed
Feb 27, 2025
| diag::err_counted_by_attr_not_on_flexible_array_member) | ||
| << SR; | ||
| S.Diag(FD->getBeginLoc(), | ||
| diag::err_counted_by_attr_on_array_not_flexible_array_member) |
There was a problem hiding this comment.
I wonder what the motivation for this was. Wouldn't it also be nice to be able apply this to objects that isFlexibleArrayMemberLike for a different StrictFlexArraysLevelKind, such as arrays of size 0 or 1 (or even more), which is often use in old code?
There was a problem hiding this comment.
@fmayer This is an artificial limitation to meet the current upstream uses of the attribute. The downstream implementation doesn't restrict __counted_by to just flexible array members. the full implementation of -fbounds-safety can be found in https://github.com/swiftlang/llvm-project/tree/next
There was a problem hiding this comment.
I believe the question is not why only flexible array members are allowed the attribute, but why only incomplete arrays are. Some old projects use this as syntax as incomplete array:
struct S {
int len;
char fam[0];
};
Would converting the above to an incomplete array like this be an issue in those projects? It seems to me that if you're introducing __counted_by, removing any old integer constant isn't much more extra work.
struct S {
int len;
char fam[__counted_by(len)];
};
The reason is that a complete array already has a bound, which we use for bounds checking with -fbounds-safety enabled. In fact, an array parameter like this will decay into a int * __counted_by(len) pointer:
void foo(int len, int arr[len]);
If you allow annotating these arrays with __counted_by also, you effectively end up with two conflicting sources of bounds info.
There was a problem hiding this comment.
Thanks a lot for the replies.
It is slightly easier (more mechanical) to not change the (size of the) underlying type, because then we do not need to make sure size calculations etc are correct. In theory I can also imagine some API considerations around the actual struct size as well (e.g. some client of the code manually calculates the size).
My thinking was that __counted_by on any FAM-eligible (i.e. last member of a struct) array would make clang consider it a FAM, regardless of its size (and regardless of -fstrict-flex-arrays), because there is a clear user intent.
There was a problem hiding this comment.
-fstrict-flex-arrays should not be overridden. counted_by should only be legal on actual flexible arrays. The other stuff (trailing array of any size) cannot be deterministically reasoned about. A project using fake flexible arrays must fix their code before using these features, and attempts to mix counted_by with sized arrays should be an error.
Refactoring to use only true flexible arrays is mostly mechanical:
https://people.kernel.org/kees/bounded-flexible-arrays-in-c
There was a problem hiding this comment.
I am not sure I follow. The linked blogpost does override -fstrict-flex-arrays (although it seems incorrectly, because it doesn't pass the level). Your point is that the only true levels are 0 or 3?
There was a problem hiding this comment.
Yes, =1 and =2 were introduced by GCC against my recommendation even though they could not justify it beyond theoretical concerns. IMO, our experience purging the Linux kernel of fake flexible arrays supports the "all or nothing" case for dealing with flexible arrays. But, whatever the case, at the end of the day, =3 needs to stick to the "strict" part of the option name. :) Anything else allows for ambiguous bounds calculations, and ambiguity is what breeds bugs and security flaws.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to reland #90786 that was reverted due to a memory leak.
[BoundsSafety] Reland #93121 Allow 'counted_by' attribute on pointers in structs in C (#93121)
Fixes #92687.
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
void) and sizeless typesThis patch also introduces late parsing of the attribute when used in
the declaration attribute position. For example
is now allowed but only when passing
-fexperimental-late-parse-attributes. The motivation for using lateparsing here is to avoid breaking the data layout of structs in existing
code that want to use the
counted_byattribute. This patch is thefirst use of
LateAttrParseExperimentalExtinAttr.tdthat wasintroduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
on_pointer_anon_bufinattr-counted-by-late-parsed-struct-ptrs.c).counted_byon more subjects (e.g. parameters, returns types)when
-fbounds-safetyis enabled._builtin_dynamic_object_sizeand UBSan's array-bounds checks).This work is heavily based on a patch originally written by Yeoul Na.
Differences between #93121 and this patch
now be fixed. See "The Memory Leak".
pragma-attribute-supported-attributes-list.test(originally in cef6387) has been incorporated into this patch.
incorporated into this patch.
Parser::DistributeCLateParsedAttrshas been removedbecause that broke downstream code.
Parser::ParseLexedCAttributehas beenremoved in favor of using
Parser::ParseGNUAttributeArgswhich doesthe same thing but is more feature complete.
EnterScopeparameter has been plumbed throughParser::ParseLexedCAttributeandParser::ParseLexedCAttributeList.It currently doesn't do anything but it will be needed in future
commits.
The Memory Leak
The problem was that these lines parsed the attributes but then did nothing to free the memory
To fix this this a new
Parser::ParseLexedCAttributeListmethod has beenadded (based on
Parser::ParseLexedAttributeList) which does thenecessary memory management. The intention is to merge these two
methods together so there is just one implementation in a future patch.
A more principled fixed here would be to fix the ownership of the
LateParsedAttributeobjects. In principleLateParsedAttrListshould ownits pointers exclusively and be responsible for deallocating them.
Unfortunately this is complicated by
LateParsedAttributeobjects alsobeing stored in another data structure (
LateParsedDeclarations) ascan be seen below (
LAgets stored in two places).this means the ownership of LateParsedAttribute objects isn't very
clear.
rdar://125400257