-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[BoundsSafety] Support late parsing for counted_by in type positions
#166491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b8f5a00
e60ce60
24c44d6
7ce24ab
bb24800
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1161,10 +1161,13 @@ class Parser : public CodeCompletionHandler { | |
| IdentifierInfo *MacroII = nullptr; | ||
| SourceLocation AttrNameLoc; | ||
| SmallVector<Decl *, 2> Decls; | ||
| unsigned NestedTypeLevel; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain what this means? This is awkwardly worded and isn't clear at all what it should represent.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this feels like it's leaking an implementation detail for bounds safety into something more generally used for late parsed attributes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This represents the pointer indirection level where the attribute applies (e.g., I agree the name could be clearer - maybe This isn't bounds-safety specific - it's a member of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's still an implementation detail though; only bounds safety cares about the pointer indirection level and I don't think [m]any other attributes will need it. I think what I'm asking for here is whether we should make this a bit more clear in the interface. Currently, (outside of bounds safety) But because we're expanding the capabilities here... Do we want
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree bounds safety attributes are the only type attributes that need late parsing right now. That said, any late-parsed type attribute needs to know its position in the type tree, so tracking this seems inherent to the feature—not bounds-safety specific. IOW, even if we stored both QualTypes and walked between them like you suggested, we'd still need that position-calculating logic for any
This makes sense to me. I can try
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I guess I'm not seeing why it's needed for any type attribute. Our existing type attributes either apply to one specific kind of type (like calling conventions which apply to function [pointer] types) or they're general so they apply to any levels of nesting (like __sptr, nullability qualifiers, etc). I don't think we have any other attributes where the nesting level matters, do we?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conceptually, I was thinking that type attributes are generally tied to a specific position in the type tree—like how nullability works with So my mental model is that type attributes are generally meant to be tied to a specific nested type position as written in the source, and if a certain type attribute decides to ignore the position, that's attribute-specific behavior. Though I guess you're saying most type attributes ignore the specific type position?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most attributes don't need to care about the specific type position; as we're converting the specifiers into a type, we're applying the attribute at the proper level of the type. So if the attribute only applies to an Your case is different because you want to have further restrictions on the entire type, not just the piece of the type the attribute it written on. I was thinking about this a bit last night because who needs to sleep when they can think about type attributes, and I think a potentially cleaner way to handle this is from |
||
|
|
||
| explicit LateParsedAttribute(Parser *P, IdentifierInfo &Name, | ||
| SourceLocation Loc) | ||
| : Self(P), AttrName(Name), AttrNameLoc(Loc) {} | ||
| SourceLocation Loc, | ||
| unsigned NestedTypeLevel = 0) | ||
| : Self(P), AttrName(Name), AttrNameLoc(Loc), | ||
| NestedTypeLevel(NestedTypeLevel) {} | ||
|
|
||
| void ParseLexedAttributes() override; | ||
|
|
||
|
|
@@ -1889,10 +1892,12 @@ class Parser : public CodeCompletionHandler { | |
| DeclSpec &DS, AccessSpecifier AS, DeclSpecContext DSContext, | ||
| LateParsedAttrList *LateAttrs = nullptr); | ||
|
|
||
| void ParseSpecifierQualifierList( | ||
| DeclSpec &DS, AccessSpecifier AS = AS_none, | ||
| DeclSpecContext DSC = DeclSpecContext::DSC_normal) { | ||
| ParseSpecifierQualifierList(DS, getImplicitTypenameContext(DSC), AS, DSC); | ||
| void | ||
| ParseSpecifierQualifierList(DeclSpec &DS, AccessSpecifier AS = AS_none, | ||
| DeclSpecContext DSC = DeclSpecContext::DSC_normal, | ||
| LateParsedAttrList *LateAttrs = nullptr) { | ||
| ParseSpecifierQualifierList(DS, getImplicitTypenameContext(DSC), AS, DSC, | ||
| LateAttrs); | ||
| } | ||
|
|
||
| /// ParseSpecifierQualifierList | ||
|
|
@@ -1903,10 +1908,12 @@ class Parser : public CodeCompletionHandler { | |
| /// [GNU] attributes specifier-qualifier-list[opt] | ||
| /// \endverbatim | ||
| /// | ||
| void ParseSpecifierQualifierList( | ||
| DeclSpec &DS, ImplicitTypenameContext AllowImplicitTypename, | ||
| AccessSpecifier AS = AS_none, | ||
| DeclSpecContext DSC = DeclSpecContext::DSC_normal); | ||
| void | ||
| ParseSpecifierQualifierList(DeclSpec &DS, | ||
| ImplicitTypenameContext AllowImplicitTypename, | ||
| AccessSpecifier AS = AS_none, | ||
| DeclSpecContext DSC = DeclSpecContext::DSC_normal, | ||
| LateParsedAttrList *LateAttrs = nullptr); | ||
|
|
||
| /// ParseEnumSpecifier | ||
| /// \verbatim | ||
|
|
@@ -2444,7 +2451,8 @@ class Parser : public CodeCompletionHandler { | |
| SourceLocation ScopeLoc, | ||
| ParsedAttr::Form Form); | ||
|
|
||
| void DistributeCLateParsedAttrs(Decl *Dcl, LateParsedAttrList *LateAttrs); | ||
| void DistributeCLateParsedAttrs(Declarator &D, Decl *Dcl, | ||
| LateParsedAttrList *LateAttrs); | ||
|
|
||
| /// Bounds attributes (e.g., counted_by): | ||
| /// \verbatim | ||
|
|
@@ -2610,7 +2618,8 @@ class Parser : public CodeCompletionHandler { | |
| void ParseTypeQualifierListOpt( | ||
| DeclSpec &DS, unsigned AttrReqs = AR_AllAttributesParsed, | ||
| bool AtomicOrPtrauthAllowed = true, bool IdentifierRequired = false, | ||
| llvm::function_ref<void()> CodeCompletionHandler = {}); | ||
| llvm::function_ref<void()> CodeCompletionHandler = {}, | ||
| LateParsedAttrList *LateAttrs = nullptr); | ||
|
|
||
| /// ParseDirectDeclarator | ||
| /// \verbatim | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1238,6 +1238,13 @@ struct DeclaratorChunk { | |
|
|
||
| ParsedAttributesView AttrList; | ||
|
|
||
| /// Stores pointers to `Parser::LateParsedAttribute`. We use `void*` here | ||
| /// because `LateParsedAttribute` is a nested struct of `class Parser` and | ||
| /// cannot be forward-declared. | ||
| using LateAttrOpaquePtr = void *; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. THIS feels like a code smell, and I'm pretty uncomfortable by what is happening here. I'm hopeful that @AaronBallman can come along and take a look at this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed other alternatives here: #166491 (comment) I couldn't include
So using an opaque type seemed like a cleanest solution, but I'm open to other suggestions.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we introduce a base class, outside of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can do that. |
||
| using LateAttrListTy = SmallVector<LateAttrOpaquePtr, 1>; | ||
| LateAttrListTy LateAttrList; | ||
|
|
||
| struct PointerTypeInfo { | ||
| /// The type qualifiers: const/volatile/restrict/unaligned/atomic. | ||
| LLVM_PREFERRED_TYPE(DeclSpec::TQ) | ||
|
|
@@ -2324,14 +2331,18 @@ class Declarator { | |
| /// EndLoc, which should be the last token of the chunk. | ||
| /// This function takes attrs by R-Value reference because it takes ownership | ||
| /// of those attributes from the parameter. | ||
| void AddTypeInfo(const DeclaratorChunk &TI, ParsedAttributes &&attrs, | ||
| SourceLocation EndLoc) { | ||
| void | ||
| AddTypeInfo(const DeclaratorChunk &TI, ParsedAttributes &&attrs, | ||
| SourceLocation EndLoc, | ||
| ArrayRef<DeclaratorChunk::LateAttrOpaquePtr> LateAttrs = {}) { | ||
| DeclTypeInfo.push_back(TI); | ||
| DeclTypeInfo.back().getAttrs().prepend(attrs.begin(), attrs.end()); | ||
| getAttributePool().takeAllFrom(attrs.getPool()); | ||
|
|
||
| if (!EndLoc.isInvalid()) | ||
| SetRangeEnd(EndLoc); | ||
|
|
||
| DeclTypeInfo.back().LateAttrList.assign(LateAttrs); | ||
| } | ||
|
|
||
| /// AddTypeInfo - Add a chunk to this declarator. Also extend the range to | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2445,6 +2445,8 @@ class Sema final : public SemaBase { | |
| /// | ||
| /// \param FD The FieldDecl to apply the attribute to | ||
| /// \param E The count expression on the attribute | ||
| /// \param NestedTypeLevel The pointer indirection level where the attribute | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... why does this have to be a certain number deep, and not just calculated/stored on that type?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the attribute is late parsed after the struct is fully declared, so the field's type is already constructed. We could use placeholder types and backfill later, but then we'd have to walk through all the type indirections. Or record the type positions that need the backfill anyway. Passing the level directly seemed simpler and avoids that overhead. |
||
| /// applies | ||
| /// \param CountInBytes If true the attribute is from the "sized_by" family of | ||
| /// attributes. If the false the attribute is from | ||
| /// "counted_by" family of attributes. | ||
|
|
@@ -2457,7 +2459,8 @@ class Sema final : public SemaBase { | |
| /// `counted_by_or_null` attribute. | ||
| /// | ||
| /// \returns false iff semantically valid. | ||
| bool CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes, | ||
| bool CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, | ||
| unsigned NestedTypeLevel, bool CountInBytes, | ||
| bool OrNull); | ||
|
|
||
| /// Perform Bounds Safety Semantic checks for assigning to a `__counted_by` or | ||
|
|
@@ -4198,7 +4201,8 @@ class Sema final : public SemaBase { | |
|
|
||
| /// ActOnFinishDelayedAttribute - Invoked when we have finished parsing an | ||
| /// attribute for which parsing is delayed. | ||
| void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs); | ||
| void ActOnFinishDelayedAttribute(Scope *S, Decl *D, ParsedAttributes &Attrs, | ||
| unsigned NestedTypeLevel = 0); | ||
|
|
||
| /// Diagnose any unused parameters in the given sequence of | ||
| /// ParmVarDecl pointers. | ||
|
|
@@ -5071,7 +5075,8 @@ class Sema final : public SemaBase { | |
| void ProcessDeclAttributeList(Scope *S, Decl *D, | ||
| const ParsedAttributesView &AttrList, | ||
| const ProcessDeclAttributeOptions &Options = | ||
| ProcessDeclAttributeOptions()); | ||
| ProcessDeclAttributeOptions(), | ||
| unsigned NestedTypeLevel = 0); | ||
|
|
||
| /// Annotation attributes are the only attributes allowed after an access | ||
| /// specifier. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1943,7 +1943,11 @@ Parser::DeclGroupPtrTy Parser::ParseSimpleDeclaration( | |
|
|
||
| ParsedTemplateInfo TemplateInfo; | ||
| DeclSpecContext DSContext = getDeclSpecContextFromDeclaratorContext(Context); | ||
| ParseDeclarationSpecifiers(DS, TemplateInfo, AS_none, DSContext); | ||
| // FIXME: Why is PSoon true? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats a wonderful question :) |
||
| LateParsedAttrList BoundsSafetyLateAttrs( | ||
| /*PSoon=*/true, /*LateAttrParseExperimentalExtOnly=*/true); | ||
| ParseDeclarationSpecifiers(DS, TemplateInfo, AS_none, DSContext, | ||
| &BoundsSafetyLateAttrs); | ||
|
|
||
| // If we had a free-standing type definition with a missing semicolon, we | ||
| // may get this far before the problem becomes obvious. | ||
|
|
@@ -2725,12 +2729,12 @@ Decl *Parser::ParseDeclarationAfterDeclaratorAndAttributes( | |
|
|
||
| void Parser::ParseSpecifierQualifierList( | ||
| DeclSpec &DS, ImplicitTypenameContext AllowImplicitTypename, | ||
| AccessSpecifier AS, DeclSpecContext DSC) { | ||
| AccessSpecifier AS, DeclSpecContext DSC, LateParsedAttrList *LateAttrs) { | ||
| ParsedTemplateInfo TemplateInfo; | ||
| /// specifier-qualifier-list is a subset of declaration-specifiers. Just | ||
| /// parse declaration-specifiers and complain about extra stuff. | ||
| /// TODO: diagnose attribute-specifiers and alignment-specifiers. | ||
| ParseDeclarationSpecifiers(DS, TemplateInfo, AS, DSC, nullptr, | ||
| ParseDeclarationSpecifiers(DS, TemplateInfo, AS, DSC, LateAttrs, | ||
| AllowImplicitTypename); | ||
|
|
||
| // Validate declspec for type-name. | ||
|
|
@@ -3136,15 +3140,37 @@ void Parser::ParseAlignmentSpecifier(ParsedAttributes &Attrs, | |
| } | ||
| } | ||
|
|
||
| void Parser::DistributeCLateParsedAttrs(Decl *Dcl, | ||
| void Parser::DistributeCLateParsedAttrs(Declarator &D, Decl *Dcl, | ||
| LateParsedAttrList *LateAttrs) { | ||
| if (!LateAttrs) | ||
| return; | ||
|
|
||
| unsigned NestedTypeLevel = 0; | ||
| for (unsigned i = 0; i < D.getNumTypeObjects(); ++i) { | ||
| DeclaratorChunk &DC = D.getTypeObject(i); | ||
|
|
||
| switch (DC.Kind) { | ||
| case DeclaratorChunk::Pointer: | ||
| case DeclaratorChunk::Array: | ||
| break; | ||
| default: | ||
| continue; | ||
| } | ||
|
|
||
| // Extract `LateParsedAttribute *` from `DeclaratorChunk`. | ||
| for (auto *OpaqueLA : DC.LateAttrList) { | ||
| auto *LA = static_cast<LateParsedAttribute *>(OpaqueLA); | ||
| LA->NestedTypeLevel = NestedTypeLevel; | ||
| LateAttrs->push_back(LA); | ||
| } | ||
| NestedTypeLevel++; | ||
| } | ||
|
|
||
| // Attach `Decl *` to each `LateParsedAttribute *`. | ||
| if (Dcl) { | ||
| for (auto *LateAttr : *LateAttrs) { | ||
| if (LateAttr->Decls.empty()) | ||
| LateAttr->addDecl(Dcl); | ||
| for (auto *LA : *LateAttrs) { | ||
| if (LA->Decls.empty()) | ||
| LA->addDecl(Dcl); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -3217,12 +3243,6 @@ void Parser::ParseBoundsAttribute(IdentifierInfo &AttrName, | |
| ArgExprs.push_back(ArgExpr.get()); | ||
| Parens.consumeClose(); | ||
|
|
||
| ASTContext &Ctx = Actions.getASTContext(); | ||
|
|
||
| ArgExprs.push_back(IntegerLiteral::Create( | ||
| Ctx, llvm::APInt(Ctx.getTypeSize(Ctx.getSizeType()), 0), | ||
| Ctx.getSizeType(), SourceLocation())); | ||
|
|
||
| Attrs.addNew(&AttrName, SourceRange(AttrNameLoc, Parens.getCloseLocation()), | ||
| AttributeScopeInfo(), ArgExprs.data(), ArgExprs.size(), Form); | ||
| } | ||
|
|
@@ -4706,7 +4726,8 @@ void Parser::ParseStructDeclaration( | |
| MaybeParseCXX11Attributes(Attrs); | ||
|
|
||
| // Parse the common specifier-qualifiers-list piece. | ||
| ParseSpecifierQualifierList(DS); | ||
| ParseSpecifierQualifierList(DS, AS_none, DeclSpecContext::DSC_normal, | ||
| LateFieldAttrs); | ||
|
|
||
| // If there are no declarators, this is a free-standing declaration | ||
| // specifier. Let the actions module cope with it. | ||
|
|
@@ -4768,7 +4789,7 @@ void Parser::ParseStructDeclaration( | |
| // We're done with this declarator; invoke the callback. | ||
| Decl *Field = FieldsCallback(DeclaratorInfo); | ||
| if (Field) | ||
| DistributeCLateParsedAttrs(Field, LateFieldAttrs); | ||
| DistributeCLateParsedAttrs(DeclaratorInfo.D, Field, LateFieldAttrs); | ||
|
|
||
| // If we don't have a comma, it is either the end of the list (a ';') | ||
| // or an error, bail out. | ||
|
|
@@ -4825,7 +4846,8 @@ void Parser::ParseLexedCAttribute(LateParsedAttribute &LA, bool EnterScope, | |
| SourceLocation(), ParsedAttr::Form::GNU(), nullptr); | ||
|
|
||
| for (auto *D : LA.Decls) | ||
| Actions.ActOnFinishDelayedAttribute(getCurScope(), D, Attrs); | ||
| Actions.ActOnFinishDelayedAttribute(getCurScope(), D, Attrs, | ||
| LA.NestedTypeLevel); | ||
|
|
||
| // 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. | ||
|
|
@@ -6129,7 +6151,8 @@ bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide, | |
|
|
||
| void Parser::ParseTypeQualifierListOpt( | ||
| DeclSpec &DS, unsigned AttrReqs, bool AtomicOrPtrauthAllowed, | ||
| bool IdentifierRequired, llvm::function_ref<void()> CodeCompletionHandler) { | ||
| bool IdentifierRequired, llvm::function_ref<void()> CodeCompletionHandler, | ||
| LateParsedAttrList *LateAttrs) { | ||
| if ((AttrReqs & AR_CXX11AttributesParsed) && | ||
| isAllowedCXX11AttributeSpecifier()) { | ||
| ParsedAttributes Attrs(AttrFactory); | ||
|
|
@@ -6271,7 +6294,9 @@ void Parser::ParseTypeQualifierListOpt( | |
| // recovery is graceful. | ||
| if (AttrReqs & AR_GNUAttributesParsed || | ||
| AttrReqs & AR_GNUAttributesParsedAndRejected) { | ||
| ParseGNUAttributes(DS.getAttributes()); | ||
|
|
||
| assert(!LateAttrs || LateAttrs->lateAttrParseExperimentalExtOnly()); | ||
| ParseGNUAttributes(DS.getAttributes(), LateAttrs); | ||
| continue; // do *not* consume the next token! | ||
| } | ||
| // otherwise, FALL THROUGH! | ||
|
|
@@ -6452,21 +6477,35 @@ void Parser::ParseDeclaratorInternal(Declarator &D, | |
| ((D.getContext() != DeclaratorContext::CXXNew) | ||
| ? AR_GNUAttributesParsed | ||
| : AR_GNUAttributesParsedAndRejected); | ||
| LateParsedAttrList LateAttrs(/*PSoon=*/true, | ||
| /*LateAttrParseExperimentalExtOnly=*/true); | ||
| ParseTypeQualifierListOpt(DS, Reqs, /*AtomicOrPtrauthAllowed=*/true, | ||
| !D.mayOmitIdentifier()); | ||
| !D.mayOmitIdentifier(), {}, &LateAttrs); | ||
| D.ExtendWithDeclSpec(DS); | ||
|
|
||
| // Recursively parse the declarator. | ||
| Actions.runWithSufficientStackSpace( | ||
| D.getBeginLoc(), [&] { ParseDeclaratorInternal(D, DirectDeclParser); }); | ||
| if (Kind == tok::star) | ||
| if (Kind == tok::star) { | ||
| DeclaratorChunk::LateAttrListTy OpaqueLateAttrList; | ||
| if (getLangOpts().ExperimentalLateParseAttributes && !LateAttrs.empty()) { | ||
| // TODO: Support `counted_by` in function parameters, return types, and | ||
| // other contexts (Issue #167365). | ||
| if (!D.isFunctionDeclarator()) { | ||
rapidsna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for (LateParsedAttribute *LA : LateAttrs) { | ||
| OpaqueLateAttrList.push_back(LA); | ||
| } | ||
| } | ||
| LateAttrs.clear(); | ||
| } | ||
| // Remember that we parsed a pointer type, and remember the type-quals. | ||
| D.AddTypeInfo(DeclaratorChunk::getPointer( | ||
| DS.getTypeQualifiers(), Loc, DS.getConstSpecLoc(), | ||
| DS.getVolatileSpecLoc(), DS.getRestrictSpecLoc(), | ||
| DS.getAtomicSpecLoc(), DS.getUnalignedSpecLoc()), | ||
| std::move(DS.getAttributes()), SourceLocation()); | ||
| else | ||
| std::move(DS.getAttributes()), SourceLocation(), | ||
| OpaqueLateAttrList); | ||
| } else | ||
| // Remember that we parsed a Block type, and remember the type-quals. | ||
| D.AddTypeInfo( | ||
| DeclaratorChunk::getBlockPointer(DS.getTypeQualifiers(), Loc), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that "nested pointer type" is sufficiently clear?
is
ptra "nested" pointer type given that it's within the declarator for a function pointer?Maybe "on a pointer to pointer type is not allowed"?