[clang] Fix gnu::init_priority attribute handling for reserved values

[el-ev]

Closes #121108.

• Added a new diagnostic group InitPriorityReserved
• Allow values within the range 0-100 of init_priority to be used outside system library, but with a warning
• Updated relavant tests

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Iris (el-ev)

Changes

Closes #121108.

• Added a new diagnostic group InitPriorityReserved
• Allow values within the range 0-100 of init_priority to be used outside system library, but with a warning
• Updated relavant tests

---

Full diff: https://github.com/llvm/llvm-project/pull/121577.diff

4 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/SemaDeclAttr.cpp (+10-5)
• (modified) clang/test/SemaCXX/init-priority-attr.cpp (+6-7)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 3ac490d30371b1..046bd0c5c8f2b9 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -510,6 +510,7 @@ def PrivateModule : DiagGroup<"private-module">;
 def CXX11InlineNamespace : DiagGroup<"c++11-inline-namespace">;
 def InlineNamespaceReopenedNoninline
     : DiagGroup<"inline-namespace-reopened-noninline">;
+def InitPriorityReserved : DiagGroup<"init-priority-reserved">;
 def InvalidNoreturn : DiagGroup<"invalid-noreturn">;
 def InvalidSourceEncoding : DiagGroup<"invalid-source-encoding">;
 def KNRPromotedParameter : DiagGroup<"knr-promoted-parameter">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 330ae045616aba..acc8868829df64 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3324,6 +3324,9 @@ def err_attribute_argument_out_of_range : Error<
 def err_init_priority_object_attr : Error<
   "can only use 'init_priority' attribute on file-scope definitions "
   "of objects of class type">;
+def warn_init_priority_reserved : Warning<
+  "requested 'init_priority' %0 is reserved for internal use">,
+  InGroup<InitPriorityReserved>;
 def err_attribute_argument_out_of_bounds : Error<
   "%0 attribute parameter %1 is out of bounds">;
 def err_attribute_only_once_per_parameter : Error<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index bb4d33560b93b8..de3b867eaa7c52 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3591,15 +3591,20 @@ static void handleInitPriorityAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
     return;
   }
 
+  if (prioritynum < 0 || prioritynum > 65535) {
+    S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_range)
+        << E->getSourceRange() << AL << 0 << 65535;
+    AL.setInvalid();
+    return;
+  }
+
   // Only perform the priority check if the attribute is outside of a system
   // header. Values <= 100 are reserved for the implementation, and libc++
   // benefits from being able to specify values in that range.
-  if ((prioritynum < 101 || prioritynum > 65535) &&
+  if (prioritynum < 101 &&
       !S.getSourceManager().isInSystemHeader(AL.getLoc())) {
-    S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_range)
-        << E->getSourceRange() << AL << 101 << 65535;
-    AL.setInvalid();
-    return;
+    S.Diag(AL.getLoc(), diag::warn_init_priority_reserved)
+        << E->getSourceRange() << prioritynum;
   }
   D->addAttr(::new (S.Context) InitPriorityAttr(S.Context, AL, prioritynum));
 }
diff --git a/clang/test/SemaCXX/init-priority-attr.cpp b/clang/test/SemaCXX/init-priority-attr.cpp
index 8c0a17682bb02a..0d41f4ddc62179 100644
--- a/clang/test/SemaCXX/init-priority-attr.cpp
+++ b/clang/test/SemaCXX/init-priority-attr.cpp
@@ -33,15 +33,15 @@ Two goo __attribute__((init_priority(2,3))) ( 5, 6 ); // expected-error {{'init_
 
 Two coo[2]  __attribute__((init_priority(100)));
 #if !defined(SYSTEM)
-  // expected-error@-2 {{'init_priority' attribute requires integer constant between 101 and 65535 inclusive}}
+  // expected-warning@-2 {{requested 'init_priority' 100 is reserved for internal use}}
   // unknown-warning@-3 {{unknown attribute 'init_priority' ignored}}
 #endif
 
-Two boo[2]  __attribute__((init_priority(65536)));
-#if !defined(SYSTEM)
- // expected-error@-2 {{'init_priority' attribute requires integer constant between 101 and 65535 inclusive}}
- // unknown-warning@-3 {{unknown attribute 'init_priority' ignored}}
-#endif
+Two zoo[2]  __attribute__((init_priority(-1))); // expected-error {{'init_priority' attribute requires integer constant between 0 and 65535 inclusive}}
+// unknown-warning@-1 {{unknown attribute 'init_priority' ignored}}
+
+Two boo[2]  __attribute__((init_priority(65536))); // expected-error {{'init_priority' attribute requires integer constant between 0 and 65535 inclusive}}
+// unknown-warning@-1 {{unknown attribute 'init_priority' ignored}}
 
 Two koo[4]  __attribute__((init_priority(1.13))); // expected-error {{'init_priority' attribute requires an integer constant}}
 // unknown-warning@-1 {{unknown attribute 'init_priority' ignored}}
@@ -49,7 +49,6 @@ Two koo[4]  __attribute__((init_priority(1.13))); // expected-error {{'init_prio
 Two func()  __attribute__((init_priority(1001))); // expected-error {{'init_priority' attribute only applies to variables}}
 // unknown-warning@-1 {{unknown attribute 'init_priority' ignored}}
 
-
 int i  __attribute__((init_priority(1001))); // expected-error {{can only use 'init_priority' attribute on file-scope definitions of objects of class type}}
 // unknown-warning@-1 {{unknown attribute 'init_priority' ignored}}
 

[el-ev]

Ping.

[erichkeane]

2 nits, else this is on the right track.

[AaronBallman]

Allow values within the range 0-100 of init_priority to be used outside system library, but with a warning

This was an intentional decision, not a bug or an oversight (I'm sorry, I didn't see that @philnik777 had marked this a good first issue!). See #67673 for details. Basically: a warning is insufficient because it provides zero protection for the real world uses of the attribute. Reserved in this case really does mean "you only get to use those values if you're a system header" and we live with the "hole" that GNU line markers provide.

I don't think we should move forward with this change unless there's a strong need to do so.

[philnik777]

Allow values within the range 0-100 of init_priority to be used outside system library, but with a warning

This was an intentional decision, not a bug or an oversight (I'm sorry, I didn't see that @philnik777 had marked this a good first issue!). See #67673 for details. Basically: a warning is insufficient because it provides zero protection for the real world uses of the attribute. Reserved in this case really does mean "you only get to use those values if you're a system header" and we live with the "hole" that GNU line markers provide.

I don't think we should move forward with this change unless there's a strong need to do so.

While this is a valid concern, I don't think it's much of a problem in reality. GCC just warns on this (not even an error by default) and I wasn't able to find a single use outside implementations of the reserved value range. For that reason I don't think we should make life unnecessarily hard for implementations. Using line markers is a huge burden, since they are anything but readable, people don't know what they are, and they are really easy to get wrong when modifying code, since you have to hard-code the line number. When you have a warning that's by default an error I don't see many people disabling that to stomp on implementers' toes.

[AaronBallman]

While this is a valid concern, I don't think it's much of a problem in reality. GCC just warns on this (not even an error by default) and I wasn't able to find a single use outside implementations of the reserved value range.

I wouldn't expect to find uses within the reserved value range except in system headers given that we have an error.

For that reason I don't think we should make life unnecessarily hard for implementations. Using line markers is a huge burden, since they are anything but readable, people don't know what they are, and they are really easy to get wrong when modifying code, since you have to hard-code the line number. When you have a warning that's by default an error I don't see many people disabling that to stomp on implementers' toes.

I disagree. There's no value to a reserved range that's not actually reserved. You can use GNU line markers, but if you find those distasteful (which is reasonable), there's also #pragma GCC system_header as well.

[philnik777]

While this is a valid concern, I don't think it's much of a problem in reality. GCC just warns on this (not even an error by default) and I wasn't able to find a single use outside implementations of the reserved value range.

I wouldn't expect to find uses within the reserved value range except in system headers given that we have an error.

Since GCC doesn't reject it I don't see why you'd expect zero instances. AFAIK there's lots of code that only compiles with GCC.

For that reason I don't think we should make life unnecessarily hard for implementations. Using line markers is a huge burden, since they are anything but readable, people don't know what they are, and they are really easy to get wrong when modifying code, since you have to hard-code the line number. When you have a warning that's by default an error I don't see many people disabling that to stomp on implementers' toes.

I disagree. There's no value to a reserved range that's not actually reserved. You can use GNU line markers, but if you find those distasteful (which is reasonable), there's also #pragma GCC system_header as well.

#pragma GCC system_header only works on headers, and even in that case it's horrible for testing. If you want warnings when testing the implementation you'd have to put the variable into its own header just to add the pragma specifically to that.

[AaronBallman]

We talked about this during my morning office hours today (thanks @erichkeane and @philnik777 for coming!) and I'm now comfortable with the approach taken in this patch. Warning which defaults to error strikes a pretty reasonable compromise.

[AaronBallman]

Aside from a minor testing nit, I think the only thing left is to add a release note to clang/docs/ReleaseNotes.rst so users know about the change.

[el-ev]

ping

[AaronBallman]

LGTM!

[github-actions]

@el-ev Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!