[Attributes] Support Attributes being declared as supporting an experimental late parsing mode "extension" #88596
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
AaronBallman,
erichkeane,
hnrklssn,
rapidsna and
patrykstefanski
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Dan Liew (delcypher) Changes[Attributes] Support Attributes being declared as only supporting late parsing when passing an experimental feature flag This patch changes the
The new In this patch the The motivation for this patch is to be able to late parse the new Adoption of Patch is 20.25 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88596.diff 8 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index dc87a8c6f022dc..da76f807cd676c 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -592,6 +592,16 @@ class AttrSubjectMatcherAggregateRule<AttrSubject subject> {
def SubjectMatcherForNamed : AttrSubjectMatcherAggregateRule<Named>;
+// Late Attribute parsing mode enum
+class LateAttrParseKind <int val> {
+ int Kind = val;
+}
+def LateAttrParsingNever : LateAttrParseKind<0>; // Never late parsed
+def LateAttrParsingAlways: LateAttrParseKind<1>; // Always late parsed
+// Late parsed if `-fexperimental-late-parse-attributes` is on and parsed
+// normally if off.
+def LateAttrParsingExperimentalOnly : LateAttrParseKind<2>;
+
class Attr {
// The various ways in which an attribute can be spelled in source
list<Spelling> Spellings;
@@ -603,8 +613,8 @@ class Attr {
list<Accessor> Accessors = [];
// Specify targets for spellings.
list<TargetSpecificSpelling> TargetSpecificSpellings = [];
- // Set to true for attributes with arguments which require delayed parsing.
- bit LateParsed = 0;
+ // Specifies the late parsing kind.
+ LateAttrParseKind LateParsed = LateAttrParsingNever;
// Set to false to prevent an attribute from being propagated from a template
// to the instantiation.
bit Clone = 1;
@@ -3173,7 +3183,7 @@ def DiagnoseIf : InheritableAttr {
BoolArgument<"ArgDependent", 0, /*fake*/ 1>,
DeclArgument<Named, "Parent", 0, /*fake*/ 1>];
let InheritEvenIfAlreadyPresent = 1;
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let AdditionalMembers = [{
bool isError() const { return diagnosticType == DT_Error; }
bool isWarning() const { return diagnosticType == DT_Warning; }
@@ -3472,7 +3482,7 @@ def AssertCapability : InheritableAttr {
let Spellings = [Clang<"assert_capability", 0>,
Clang<"assert_shared_capability", 0>];
let Subjects = SubjectList<[Function]>;
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3488,7 +3498,7 @@ def AcquireCapability : InheritableAttr {
GNU<"exclusive_lock_function">,
GNU<"shared_lock_function">];
let Subjects = SubjectList<[Function]>;
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3504,7 +3514,7 @@ def TryAcquireCapability : InheritableAttr {
Clang<"try_acquire_shared_capability", 0>];
let Subjects = SubjectList<[Function],
ErrorDiag>;
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3520,7 +3530,7 @@ def ReleaseCapability : InheritableAttr {
Clang<"release_generic_capability", 0>,
Clang<"unlock_function", 0>];
let Subjects = SubjectList<[Function]>;
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3539,7 +3549,7 @@ def RequiresCapability : InheritableAttr {
Clang<"requires_shared_capability", 0>,
Clang<"shared_locks_required", 0>];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3559,7 +3569,7 @@ def NoThreadSafetyAnalysis : InheritableAttr {
def GuardedBy : InheritableAttr {
let Spellings = [GNU<"guarded_by">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3570,7 +3580,7 @@ def GuardedBy : InheritableAttr {
def PtGuardedBy : InheritableAttr {
let Spellings = [GNU<"pt_guarded_by">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3581,7 +3591,7 @@ def PtGuardedBy : InheritableAttr {
def AcquiredAfter : InheritableAttr {
let Spellings = [GNU<"acquired_after">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3592,7 +3602,7 @@ def AcquiredAfter : InheritableAttr {
def AcquiredBefore : InheritableAttr {
let Spellings = [GNU<"acquired_before">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3603,7 +3613,7 @@ def AcquiredBefore : InheritableAttr {
def AssertExclusiveLock : InheritableAttr {
let Spellings = [GNU<"assert_exclusive_lock">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3614,7 +3624,7 @@ def AssertExclusiveLock : InheritableAttr {
def AssertSharedLock : InheritableAttr {
let Spellings = [GNU<"assert_shared_lock">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3627,7 +3637,7 @@ def AssertSharedLock : InheritableAttr {
def ExclusiveTrylockFunction : InheritableAttr {
let Spellings = [GNU<"exclusive_trylock_function">];
let Args = [ExprArgument<"SuccessValue">, VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3640,7 +3650,7 @@ def ExclusiveTrylockFunction : InheritableAttr {
def SharedTrylockFunction : InheritableAttr {
let Spellings = [GNU<"shared_trylock_function">];
let Args = [ExprArgument<"SuccessValue">, VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
@@ -3651,7 +3661,7 @@ def SharedTrylockFunction : InheritableAttr {
def LockReturned : InheritableAttr {
let Spellings = [GNU<"lock_returned">];
let Args = [ExprArgument<"Arg">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let Subjects = SubjectList<[Function]>;
@@ -3661,7 +3671,7 @@ def LockReturned : InheritableAttr {
def LocksExcluded : InheritableAttr {
let Spellings = [GNU<"locks_excluded">];
let Args = [VariadicExprArgument<"Args">];
- let LateParsed = 1;
+ let LateParsed = LateAttrParsingAlways;
let TemplateDependent = 1;
let ParseArgumentsAsUnevaluated = 1;
let InheritEvenIfAlreadyPresent = 1;
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 8ef6700ecdc78e..ed483fe8df131a 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -164,6 +164,7 @@ LANGOPT(ExperimentalLibrary, 1, 0, "enable unstable and experimental library fea
LANGOPT(PointerAuthIntrinsics, 1, 0, "pointer authentication intrinsics")
LANGOPT(DoubleSquareBracketAttributes, 1, 0, "'[[]]' attributes extension for all language standard modes")
+LANGOPT(ExperimentalLateParseAttributes, 1, 0, "enable experimental late parsing of attributes")
COMPATIBLE_LANGOPT(RecoveryAST, 1, 1, "Preserve expressions in AST when encountering errors")
COMPATIBLE_LANGOPT(RecoveryASTType, 1, 1, "Preserve the type in recovery expressions")
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 9a0b5d3304ca6e..711291c0caa6bb 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1603,6 +1603,13 @@ defm double_square_bracket_attributes : BoolFOption<"double-square-bracket-attri
LangOpts<"DoubleSquareBracketAttributes">, DefaultTrue, PosFlag<SetTrue>,
NegFlag<SetFalse>>;
+defm experimental_late_parse_attributes : BoolFOption<"experimental-late-parse-attributes",
+ LangOpts<"ExperimentalLateParseAttributes">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption], "Enable">,
+ NegFlag<SetFalse, [], [ClangOption], "Disable">,
+ BothFlags<[], [ClangOption, CC1Option],
+ " experimental late parsing of attributes">>;
+
defm autolink : BoolFOption<"autolink",
CodeGenOpts<"Autolink">, DefaultTrue,
NegFlag<SetFalse, [], [ClangOption, CC1Option],
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index c719218731c35b..d9bdb07c9e5743 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -2959,6 +2959,10 @@ class Parser : public CodeCompletionHandler {
SourceLocation AttrNameLoc,
SourceLocation *EndLoc);
+ /// isAttributeLateParsed - Return true if the attribute has arguments that
+ /// require late parsing.
+ bool isAttributeLateParsed(const IdentifierInfo &II);
+
IdentifierInfo *TryParseCXX11AttributeIdentifier(
SourceLocation &Loc,
Sema::AttributeCompletion Completion = Sema::AttributeCompletion::None,
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0ad..539861601f359f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -7480,6 +7480,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
Args.addOptInFlag(CmdArgs, options::OPT_fsafe_buffer_usage_suggestions,
options::OPT_fno_safe_buffer_usage_suggestions);
+ Args.addOptInFlag(CmdArgs, options::OPT_fexperimental_late_parse_attributes,
+ options::OPT_fno_experimental_late_parse_attributes);
+
// Setup statistics file output.
SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
if (!StatsFile.empty()) {
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 583232f2d610d0..26e62054b92c12 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -91,11 +91,24 @@ static StringRef normalizeAttrName(StringRef Name) {
/// isAttributeLateParsed - Return true if the attribute has arguments that
/// require late parsing.
-static bool isAttributeLateParsed(const IdentifierInfo &II) {
+bool Parser::isAttributeLateParsed(const IdentifierInfo &II) {
+ // Some attributes might only be late parsed when in the experimental
+ // language mode.
+ if (getLangOpts().ExperimentalLateParseAttributes) {
+#define CLANG_ATTR_LATE_PARSED_EXPERIMENTAL_LIST
+ bool IsExperimentalLateParseAttr =
+ llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+#include "clang/Parse/AttrParserStringSwitches.inc"
+ .Default(false);
+#undef CLANG_ATTR_LATE_PARSED_EXPERIMENTAL_LIST
+ if (IsExperimentalLateParseAttr)
+ return true;
+ }
+
#define CLANG_ATTR_LATE_PARSED_LIST
- return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
+ return llvm::StringSwitch<bool>(normalizeAttrName(II.getName()))
#include "clang/Parse/AttrParserStringSwitches.inc"
- .Default(false);
+ .Default(false);
#undef CLANG_ATTR_LATE_PARSED_LIST
}
diff --git a/clang/test/Driver/experimental-late-parse-attributes.c b/clang/test/Driver/experimental-late-parse-attributes.c
new file mode 100644
index 00000000000000..6b54b898afa749
--- /dev/null
+++ b/clang/test/Driver/experimental-late-parse-attributes.c
@@ -0,0 +1,12 @@
+// RUN: %clang %s -c -fexperimental-late-parse-attributes 2>&1 -### | FileCheck %s -check-prefix=CHECK-ON
+// RUN: %clang %s -c -fno-experimental-late-parse-attributes -fexperimental-late-parse-attributes 2>&1 -### | FileCheck %s -check-prefix=CHECK-ON
+
+// CHECK-ON: -cc1
+// CHECK-ON: -fexperimental-late-parse-attributes
+
+// RUN: %clang %s -c 2>&1 -### | FileCheck %s -check-prefix=CHECK-OFF
+// RUN: %clang %s -c -fno-experimental-late-parse-attributes 2>&1 -### | FileCheck %s -check-prefix=CHECK-OFF
+// RUN: %clang %s -c -fexperimental-late-parse-attributes -fno-experimental-late-parse-attributes 2>&1 -### | FileCheck %s -check-prefix=CHECK-OFF
+
+// CHECK-OFF: -cc1
+// CHECK-OFF-NOT: -fexperimental-late-parse-attributes
diff --git a/clang/utils/TableGen/ClangAttrEmitter.cpp b/clang/utils/TableGen/ClangAttrEmitter.cpp
index 6c56f99f503df4..66e6841299b034 100644
--- a/clang/utils/TableGen/ClangAttrEmitter.cpp
+++ b/clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -1822,28 +1822,106 @@ void WriteSemanticSpellingSwitch(const std::string &VarName,
OS << " }\n";
}
+enum class LateAttrParseKind {
+ LateAttrParsingNever = 0,
+ LateAttrParsingAlways = 1,
+ LateAttrParsingExperimentalOnly = 2
+};
+
+static LateAttrParseKind getLateAttrParseKind(const Record *Attr) {
+ // This function basically does
+ // `Attr->getValueAsDef("LateParsed")->getValueAsInt("Mode")` but does
+ // a bunch of sanity checking in an asserts build to ensure that
+ // `LateAttrParseMode` in `Attr.td` is in sync with the `LateAttrParseKind`
+ // enum in this source file.
+
+ const char *LateParsedStr = "LateParsed";
+ auto *LAPK = Attr->getValueAsDef(LateParsedStr);
+
+ // Typecheck the `LateParsed` field.
+ SmallVector<Record *, 1> SuperClasses;
+ LAPK->getDirectSuperClasses(SuperClasses);
+ if (SuperClasses.size() != 1) {
+ PrintFatalError(Attr, "Field `" + llvm::Twine(LateParsedStr) +
+ "`should only have one super class");
+ }
+ const char *LateAttrParseKindStr = "LateAttrParseKind";
+ if (SuperClasses[0]->getName().compare(LateAttrParseKindStr) != 0) {
+ PrintFatalError(Attr, "Field `" + llvm::Twine(LateParsedStr) +
+ "`should only have type `" +
+ llvm::Twine(LateAttrParseKindStr) +
+ "` but found type `" +
+ SuperClasses[0]->getName() + "`");
+ }
+
+ // Get Kind and verify the enum name matches the name in `Attr.td`.
+ const char *KindFieldStr = "Kind";
+ unsigned Kind = LAPK->getValueAsInt(KindFieldStr);
+ switch (LateAttrParseKind(Kind)) {
+#define CASE(X) \
+ case LateAttrParseKind::X: \
+ if (LAPK->getName().compare(#X) != 0) { \
+ PrintFatalError(Attr, \
+ "Field `" + llvm::Twine(LateParsedStr) + "` set to `" + \
+ LAPK->getName() + \
+ "` but this converts to `LateAttrParseKind::" + \
+ llvm::Twine(#X) + "`"); \
+ } \
+ return LateAttrParseKind::X;
+
+ CASE(LateAttrParsingNever)
+ CASE(LateAttrParsingAlways)
+ CASE(LateAttrParsingExperimentalOnly)
+#undef CASE
+ }
+
+ // The Kind value is completely invalid
+ auto KindValueStr = llvm::utostr(Kind);
+ PrintFatalError(Attr, "Field `" + llvm::Twine(LateParsedStr) + "` set to `" +
+ LAPK->getName() + "` has unexpected `" +
+ llvm::Twine(KindFieldStr) + "` value of " +
+ KindValueStr);
+}
+
// Emits the LateParsed property for attributes.
-static void emitClangAttrLateParsedList(RecordKeeper &Records, raw_ostream &OS) {
- OS << "#if defined(CLANG_ATTR_LATE_PARSED_LIST)\n";
- std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr");
+static void emitClangAttrLateParsedListImpl(RecordKeeper &Records,
+ raw_ostream &OS,
+ LateAttrParseKind LateParseMode) {
+ std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
for (const auto *Attr : Attrs) {
- bool LateParsed = Attr->getValueAsBit("LateParsed");
+ auto LateParsed = getLateAttrParseKind(Attr);
- if (LateParsed) {
- std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(*Attr);
+ if (LateParsed != LateParseMode)
+ continue;
- // FIXME: Handle non-GNU attributes
- for (const auto &I : Spellings) {
- if (I.variety() != "GNU")
- continue;
- OS << ".Case(\"" << I.name() << "\", " << LateParsed << ")\n";
- }
+ std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(*Attr);
+
+ // FIXME: Handle non-GNU attributes
+ for (const auto &I : Spellings) {
+ if (I.variety() != "GNU")
+ continue;
+ OS << ".Case(\"" << I.name() << "\", 1)\n";
}
}
+}
+
+static void emitClangAttrLateParsedList(RecordKeeper &Records,
+ raw_ostream &OS) {
+ OS << "#if defined(CLANG_ATTR_LATE_PARSED_LIST)\n";
+ emitClangAttrLateParsedListImpl(Records, OS,
+ LateAttrParseKind::LateAttrParsingAlways);
OS << "#endif // CLANG_ATTR_LATE_PARSED_LIST\n\n";
}
+static void emitClangAttrLateParsedExperimentalList(RecordKeeper &Records,
+ raw_ostream &OS) {
+ OS << "#if defined(CLANG_ATTR_LATE_PARSED_EXPERIMENTAL_LIST)\n";
+ emitClangAttrLateParsedListImpl(
+ Records, OS, LateAttrParseKind::LateAttrParsingExperimentalOnly);
+ OS << "#endif // CLANG_ATTR_LATE_PARSED_EXPERIMENTAL_LIST\n\n";
+}
+
static bool hasGNUorCXX11Spelling(const Record &Attribute) {
std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(Attribute);
for (const auto &I : Spellings) {
@@ -2101,9 +2179,20 @@ bool PragmaClangAttributeSupport::isAttributedSupported(
return SpecifiedResult;
// Opt-out rules:
- // An attribute requires delayed parsing (LateParsed is on)
- if (Attribute.getValueAsBit("LateParsed"))
+
+ // An attribute requires delayed parsing (LateParsed is on).
+ switch (getLateAttrParseKind(&Attribute)) {
+ case LateAttrParseKind::LateAttrParsingNever:
+ break;
+ case LateAttrParseKind::LateAttrParsingAlways:
+ return false;
+ case LateAttrParseKind::LateAttrParsingExperimentalOnly:
+ // FIXME: This is only late parsed when
+ // `-fexperimental-late-parse-attributes` is on. If that option is off
+ // then we shouldn't return here.
return false;
+ }
+
// An attribute has no GNU/CXX11 spelling
if (!hasGNUorCXX11Spelling(Attribute))
return false;
@@ -2885,8 +2974,23 @@ static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
return;
}
OS << "\n : " << SuperName << "(Ctx, CommonInfo, ";
- OS << "attr::" << R.getName() << ", "
- << (R.getValueAsBit("LateParsed") ? "true" : "false");
+ OS << "attr::" << R.getName() << ", ";
+
+ // Handle different late parsing modes.
+ switch (getLateAttrParseKind(&R)) {
+ case LateAttrParseKind::LateAttrParsingNever:
+ OS << '0';
+ break;
+ case LateAttrParseKind::LateAttrParsingAlways:
+ OS << '1';
+ break;
+ case LateAttrParseKind::LateAttrParsingExperimentalOnly:
+ // Emit code that determines if Late Parsing is enabled based on
+ // the LangOpts.
+ OS << "Ctx.getLangOpts().ExperimentalLateParseAttributes";
+ break;
+ }
+
if (Inheritable) {
OS << ", "
<< (R.getValueAsBit("InheritEvenIfAlreadyPresent") ? "true"
@@ -4843,6 +4947,7 @@ void EmitClangAttrParserStringSwitches(RecordKeeper &Records, raw_ostream &OS) {
emitClangAttrA...
[truncated]
|
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
9e48095 to
554287a
Compare
Sirraide
requested changes
Apr 14, 2024
|
Thanks for the feedback.
The intention is for the functionality being added to be used in a subsequent PR. This is currently sketched in this work-in-progress PR (#87596). This patch is separate from #87596 because it makes reviewing easier. I do not know how I would write tests for declaring an attribute as experimentally late parsed without that. AFAIK I'd have to use
Given that a subsequent patch will add the necessary test coverage and the above problems I think it is reasonable for explicit tests for |
Yeah, if it’s not used anywhere or if both prs are merged at the same time, then that seems fine; as I said, I’m not too familiar w/ the overall context around this unfortunately. |
erichkeane
reviewed
Apr 15, 2024
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
08bbbaf to
3554ada
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
3554ada to
31e3269
Compare
|
@Sirraide I've tried to address your feedback. |
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
31e3269 to
db0483a
Compare
bwendling
reviewed
Apr 15, 2024
rapidsna
reviewed
Apr 15, 2024
delcypher
commented
Apr 16, 2024
delcypher
commented
Apr 16, 2024
rapidsna
reviewed
Apr 17, 2024
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
e6c29e6 to
24ea6b9
Compare
delcypher
changed the title
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
24ea6b9 to
ec02422
Compare
delcypher
changed the title
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
ec02422 to
b859cf0
Compare
rapidsna
reviewed
Apr 23, 2024
rapidsna
reviewed
Apr 23, 2024
rapidsna
reviewed
Apr 23, 2024
There was a problem hiding this comment.
We should probably add a release note to tell users about the new command line option, unless you think it makes more sense to do so after something uses LateAttrParseExperimentalExt?
One concern I have is that nothing is testing the new parsing behavior in ParseDecl.cpp, but I presume that will get tested shortly by making use of the new form?
| // The following contexts are supported: | ||
| // | ||
| // * TODO: Add contexts here when they are implemented. | ||
| // |
There was a problem hiding this comment.
It would be helpful to clarify whether this applies in all language modes or not given that LateAttrParseStandard has special rules.
There was a problem hiding this comment.
@AaronBallman
Is it ok to clarify that when the first use of LateAttrParseExperimentalExt is landed? The first new context where this going to be allowed is late parsing attributes on FieldDecls in C. However, I didn't think it made sense to mention in the patch because none of the implementation is here to do that.
Or did you have something else in mind?
|
@AaronBallman Thanks for the feedback
I had a think about it and I'll add a release note but note that it doesn't do anything and then update the note in the PR that introduces the first use of
So we have this work-in-progress PR #87596 up that will use |
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
b859cf0 to
a6d390c
Compare
delcypher
commented
Apr 23, 2024
| @@ -214,6 +214,10 @@ New Compiler Flags | |||
| This diagnostic can be disabled to make ``-Wmissing-field-initializers`` behave | |||
| like it did before Clang 18.x. Fixes (`#56628 <https://github.com/llvm/llvm-project/issues/68933>`_) | |||
|
|
|||
| - ``-fexperimental-late-parse-attributes`` enables an experimental feature to | |||
There was a problem hiding this comment.
@AaronBallman I added some documentation on the new flag. It's a bit vague right now but I'll make it more specific when the first use of LateAttrParseExperimentalExt is added.
|
@AaronBallman @erichkeane Ping. Is this ready to land? |
erichkeane
approved these changes
Apr 29, 2024
…imental late parsing mode "extension" This patch changes the `LateParsed` field of `Attr` in `Attr.td` to be an instantiation of the new `LateAttrParseKind` class. The instation can be one of the following: * `LateAttrParsingNever` - Corresponds with the false value of `LateParsed` prior to this patch (the default for an attribute). * `LateAttrParseStandard` - Corresponds with the true value of `LateParsed` prior to this patch. * `LateAttrParseExperimentalExt` - A new mode described below. `LateAttrParseExperimentalExt` is an experimental extension to `LateAttrParseStandard`. Essentially this allows `Parser::ParseGNUAttributes(...)` to distinguish between these cases: 1. Only `LateAttrParseExperimentalExt` attributes should be late parsed. 2. Both `LateAttrParseExperimentalExt` and `LateAttrParseStandard` attributes should be late parsed. Callers (and indirect callers) of `Parser::ParseGNUAttributes(...)` indicate the desired behavior by setting a flag in the `LateParsedAttrList` object that is passed to the function. In addition to the above, a new driver and frontend flag (`-fexperimental-late-parse-attributes`) with a corresponding LangOpt (`ExperimentalLateParseAttributes`) is added that changes how `LateAttrParseExperimentalExt` attributes are parsed. * When the flag is disabled (default), in cases where only `LateAttrParsingExperimentalOnly` late parsing is requested, the attribute will be parsed immediately (i.e. **NOT** late parsed). This allows the attribute to act just like a `LateAttrParseStandard` attribute when the flag is disabled. * When the flag is enabled, in cases where only `LateAttrParsingExperimentalOnly` late parsing is requested, the attribute will be late parsed. The motivation behind this change is to allow the new `counted_by` attribute (part of `-fbounds-safety`) to support late parsing but **only** when `-fexperimental-late-parse-attributes` is enabled. This attribute needs to support late parsing to allow it to refer to fields later in a struct definition (or function parameters declared later). However, there isn't a precedent for supporting late attribute parsing in C so this flag allows the new behavior to exist in Clang but not be on by default. This behavior was requested as part of the `-fbounds-safety` RFC process (https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68). This patch doesn't introduce any uses of `LateAttrParseExperimentalExt`. This will be added for the `counted_by` attribute in a future patch (llvm#87596). A consequence is the new behavior added in this patch is not yet testable. Hence, the lack of tests covering the new behavior. rdar://125400257
delcypher
force-pushed
the
users/delcypher/experimental-attr-late-parsing
branch
from
a6d390c to
07ab74a
Compare
|
@erichkeane Thanks for approving. I'll rebase, check this builds and then land this. @AaronBallman When you're back please let me know if there's any follow up changes you want. |
|
Did a message get deleted here? I saw in my email that you pointed out there is a memory leak because of this patch. (https://lab.llvm.org/buildbot/#/builders/239/builds/7043) If so, we probably should revert this until that gets figured out. |
|
@erichkeane I posted the comment on the wrong PR. The memory leak is in #90786 not this PR so I deleted the comment i made here. #90786 has already been reverted. |
Thanks! I am still catching up on emails, and I think I figured that out literally moments ago :) |
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.
[Attributes] Support Attributes being declared as supporting an experimental late parsing mode "extension"
This patch changes the
LateParsedfield ofAttrinAttr.tdto bean instantiation of the new
LateAttrParseKindclass. The instation can be one of the following:LateAttrParsingNever- Corresponds with the false value ofLateParsedprior to this patch (the default for an attribute).LateAttrParseStandard- Corresponds with the true value ofLateParsedprior to this patch.LateAttrParseExperimentalExt- A new mode described below.LateAttrParseExperimentalExtis an experimental extension toLateAttrParseStandard. Essentially this allowsParser::ParseGNUAttributes(...)to distinguish between these cases:LateAttrParseExperimentalExtattributes should be late parsed.LateAttrParseExperimentalExtandLateAttrParseStandardattributes should be late parsed.
Callers (and indirect callers) of
Parser::ParseGNUAttributes(...)indicate the desired behavior by setting a flag in the
LateParsedAttrListobject that is passed to the function.In addition to the above, a new driver and frontend flag
(
-fexperimental-late-parse-attributes) with a corresponding LangOpt(
ExperimentalLateParseAttributes) is added that changes howLateAttrParseExperimentalExtattributes are parsed.When the flag is disabled (default), in cases where only
LateAttrParsingExperimentalOnlylate parsing is requested, theattribute will be parsed immediately (i.e. NOT late parsed). This
allows the attribute to act just like a
LateAttrParseStandardattribute when the flag is disabled.
When the flag is enabled, in cases where only
LateAttrParsingExperimentalOnlylate parsing is requested, theattribute will be late parsed.
The motivation behind this change is to allow the new
counted_byattribute (part of
-fbounds-safety) to support late parsing butonly when
-fexperimental-late-parse-attributesis enabled. Thisattribute needs to support late parsing to allow it to refer to fields
later in a struct definition (or function parameters declared later).
However, there isn't a precedent for supporting late attribute parsing
in C so this flag allows the new behavior to exist in Clang but not be
on by default. This behavior was requested as part of the
-fbounds-safetyRFC process(https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/68).
This patch doesn't introduce any uses of
LateAttrParseExperimentalExt.This will be added for the
counted_byattribute in a future patch(#87596). A consequence is the
new behavior added in this patch is not yet testable. Hence, the lack of
tests covering the new behavior.
rdar://125400257