[clang] Instantiate attributes on LabelDecls #115924

ericastor · 2024-11-12T18:49:18Z

Start propagating attributes on (e.g.) labels inside of templated functions to their instances.

llvmbot · 2024-11-12T18:49:51Z

@llvm/pr-subscribers-clang

Author: Eric Astor (ericastor)

Changes

Start propagating attributes on (e.g.) labels inside of templated functions to their instances.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+13-3)
(modified) clang/test/SemaCXX/attr-mode-tmpl.cpp (+2-2)

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 5a001843e2ba46..bfc5913dbafd0f 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -990,6 +990,7 @@ Decl *
 TemplateDeclInstantiator::VisitLabelDecl(LabelDecl *D) {
   LabelDecl *Inst = LabelDecl::Create(SemaRef.Context, Owner, D->getLocation(),
                                       D->getIdentifier());
+  SemaRef.InstantiateAttrs(TemplateArgs, D, Inst, LateAttrs, StartingScope);
   Owner->addDecl(Inst);
   return Inst;
 }
@@ -1009,6 +1010,7 @@ TemplateDeclInstantiator::VisitNamespaceAliasDecl(NamespaceAliasDecl *D) {
                                  D->getQualifierLoc(),
                                  D->getTargetNameLoc(),
                                  D->getNamespace());
+  SemaRef.InstantiateAttrs(TemplateArgs, D, Inst, LateAttrs, StartingScope);
   Owner->addDecl(Inst);
   return Inst;
 }
@@ -1095,15 +1097,21 @@ Decl *TemplateDeclInstantiator::InstantiateTypedefNameDecl(TypedefNameDecl *D,
 
 Decl *TemplateDeclInstantiator::VisitTypedefDecl(TypedefDecl *D) {
   Decl *Typedef = InstantiateTypedefNameDecl(D, /*IsTypeAlias=*/false);
-  if (Typedef)
+  if (Typedef) {
+    SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef, LateAttrs,
+                             StartingScope);
     Owner->addDecl(Typedef);
+  }
   return Typedef;
 }
 
 Decl *TemplateDeclInstantiator::VisitTypeAliasDecl(TypeAliasDecl *D) {
   Decl *Typedef = InstantiateTypedefNameDecl(D, /*IsTypeAlias=*/true);
-  if (Typedef)
+  if (Typedef) {
+    SemaRef.InstantiateAttrs(TemplateArgs, D, Typedef, LateAttrs,
+                             StartingScope);
     Owner->addDecl(Typedef);
+  }
   return Typedef;
 }
 
@@ -1160,8 +1168,10 @@ Decl *TemplateDeclInstantiator::InstantiateTypeAliasTemplateDecl(
 Decl *
 TemplateDeclInstantiator::VisitTypeAliasTemplateDecl(TypeAliasTemplateDecl *D) {
   Decl *Inst = InstantiateTypeAliasTemplateDecl(D);
-  if (Inst)
+  if (Inst) {
+    SemaRef.InstantiateAttrs(TemplateArgs, D, Inst, LateAttrs, StartingScope);
     Owner->addDecl(Inst);
+  }
 
   return Inst;
 }
diff --git a/clang/test/SemaCXX/attr-mode-tmpl.cpp b/clang/test/SemaCXX/attr-mode-tmpl.cpp
index f665b1ba491237..58266f947051c5 100644
--- a/clang/test/SemaCXX/attr-mode-tmpl.cpp
+++ b/clang/test/SemaCXX/attr-mode-tmpl.cpp
@@ -9,7 +9,7 @@ void CheckEnumerations() {
   // Check that non-vector 'mode' attribute is OK with enumeration types.
   typedef T __attribute__((mode(QI))) T1;
   typedef T T2 __attribute__((mode(HI)));
-  typedef T __attribute__((mode(V8SI))) T3; // expected-error{{mode 'V8SI' is not supported for enumeration types}}
+  typedef T __attribute__((mode(V8SI))) T3; // expected-error2{{mode 'V8SI' is not supported for enumeration types}}
   // expected-warning@-1{{specifying vector types with the 'mode' attribute is deprecated}}
 
   typedef enum __attribute__((mode(HI))) { A4, B4 } T4;
@@ -62,7 +62,7 @@ struct TemplatedStruct {
 
   // Check typedefs.
   typedef T __attribute__((mode(DI)))   T1;
-  typedef T __attribute__((mode(V8DI))) T2;   // expected-error{{mode 'V8DI' is not supported for enumeration types}}
+  typedef T __attribute__((mode(V8DI))) T2;   // expected-error2{{mode 'V8DI' is not supported for enumeration types}}
                                               // expected-warning@-1{{deprecated}}
 
   // Check parameters.

erichkeane

The change is reasonable, but I need to see tests for each of the types.

In general, we've had the attitude of "we can enable this instantiation once we see examples of it being useful", so we need tests to make sure it is instantiated, AND that it is useful.

erichkeane · 2024-11-12T18:54:18Z

clang/test/SemaCXX/attr-mode-tmpl.cpp

@@ -9,7 +9,7 @@ void CheckEnumerations() {
  // Check that non-vector 'mode' attribute is OK with enumeration types.
  typedef T __attribute__((mode(QI))) T1;
  typedef T T2 __attribute__((mode(HI)));
-  typedef T __attribute__((mode(V8SI))) T3; // expected-error{{mode 'V8SI' is not supported for enumeration types}}
+  typedef T __attribute__((mode(V8SI))) T3; // expected-error2{{mode 'V8SI' is not supported for enumeration types}}


This looks like it should just cause the expected check to go away? What is going on here? If this is causing it to happen 2x, we might wonder if the attribute mode should have not been generated if it errored the 1st time.

Yeah, unfortunately the way InstantiateAttrs is implemented seems to cause it to copy attributes even if they errored previously. You can see this used elsewhere in the same file for much the same reason... this is a case where we're just having a pre-existing problem show up in a new scenario.

ericastor · 2024-11-12T22:55:06Z

The change is reasonable, but I need to see tests for each of the types.

In general, we've had the attitude of "we can enable this instantiation once we see examples of it being useful", so we need tests to make sure it is instantiated, AND that it is useful.

This makes a lot of sense... but I'm not sure there's currently a way to write a FileCheck test for (e.g.) attributes on labels, which is actually my motivating case here. It'd be a pretty big change to start dumping label attributes in text AST dumps when the LabelDecl isn't even shown. Any suggestions?

erichkeane · 2024-11-12T23:04:31Z

The change is reasonable, but I need to see tests for each of the types.
In general, we've had the attitude of "we can enable this instantiation once we see examples of it being useful", so we need tests to make sure it is instantiated, AND that it is useful.

This makes a lot of sense... but I'm not sure there's currently a way to write a FileCheck test for (e.g.) attributes on labels, which is actually my motivating case here. It'd be a pretty big change to start dumping label attributes in text AST dumps when the LabelDecl isn't even shown. Any suggestions?

the others make sense for it.

LabelDecl is an interesting beast, and it actually is a little awkward that LabelDecl can have attributes, since it is actually the statement that has them written on it. IMO, we should have it ast-dump the attributes for the LabelStmt, and it should just print the ones from the Decl. It isn't really possible AFAIK to have an 'implicit' LabelDecl have attributes anyway, so it makes sense to print them there.

We apply this to add a test of the new label attribute instantiation support.

erichkeane

Still need tests for namespacealiasdecl, typedefnamedecl, and typealiastemplatedecl actually being instantiated.

Also, is there a reason why the only way to currently test these are with the plugins? I would expect there to be SOME builtin attribute that could be validated for all of those as well.

clang/examples/Attribute/Attribute.cpp

clang/test/Frontend/plugin-attribute.cpp

ericastor · 2024-11-14T19:40:01Z

Still need tests for namespacealiasdecl, typedefnamedecl, and typealiastemplatedecl actually being instantiated.

Also, is there a reason why the only way to currently test these are with the plugins? I would expect there to be SOME builtin attribute that could be validated for all of those as well.

Since I'm not fully familiar with all of the attributes that could be applied to those decls, I'm withdrawing those parts of this PR for now. I'll be happy to help with those separately.

I'll look into whether I can test it without using plugins, but I first wanted to get any test working with LabelDecl, and (as mentioned) I wasn't sure what built-in attributes would apply. I'll follow up with more tests.

ericastor · 2024-11-14T22:16:40Z

Alright, I've scoped this change back down to just LabelDecls, and added tests that avoid the use of plugins. I appreciate your review!

erichkeane

test needs some simplification/help, else this is fine.

clang/test/SemaCXX/attr-annotate.cpp

ericastor · 2024-11-15T14:52:58Z

test needs some simplification/help, else this is fine.

Thanks, I was mimicking another file that apparently isn't following best practices. Done!

erichkeane

Test seems to have disappeared entirely!

ericastor · 2024-11-15T15:13:49Z

Test seems to have disappeared entirely!

Whoops. Apologies, missed that I'd forgotten to track the new file.

ericastor added 2 commits November 12, 2024 18:14

[clang] Instantiate attributes on other decl types

da2e66a

Start propagating attributes on (e.g.) labels inside of templated functions to their instances.

Merge branch 'llvm:main' into attributes

a6b8309

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 12, 2024

ericastor requested review from AaronBallman and erichkeane November 12, 2024 18:50

erichkeane reviewed Nov 12, 2024

View reviewed changes

Include attributes on LabelStmts (forwarding from decls) in AST dumps

eb4b3a2

We apply this to add a test of the new label attribute instantiation support.

erichkeane reviewed Nov 14, 2024

View reviewed changes

clang/examples/Attribute/Attribute.cpp Outdated Show resolved Hide resolved

clang/examples/Attribute/Attribute.cpp Outdated Show resolved Hide resolved

clang/test/Frontend/plugin-attribute.cpp Outdated Show resolved Hide resolved

Scope this change back to just applying to LabelDecls

6ba54c4

ericastor added 3 commits November 14, 2024 17:11

Merge branch 'llvm:main' into attributes

a9f73b9

Add test using standard attribute

627a4d2

Remove unnecessary use of plugins for testing

e0d7698

ericastor changed the title ~~[clang] Instantiate attributes on other decl types~~ [clang] Instantiate attributes on LabelDecls Nov 14, 2024

Fix the no-attributes case

fb705ad

erichkeane reviewed Nov 15, 2024

View reviewed changes

clang/test/SemaCXX/attr-annotate.cpp Outdated Show resolved Hide resolved

clang/test/SemaCXX/attr-annotate.cpp Outdated Show resolved Hide resolved

Clean up & simplify tests

9622499

Merge branch 'main' into attributes

a87977a

ericastor removed the request for review from AaronBallman November 15, 2024 14:56

erichkeane reviewed Nov 15, 2024

View reviewed changes

Add actual test (missed in previous commit)

fe5f361

erichkeane approved these changes Nov 15, 2024

View reviewed changes

Merge branch 'main' into attributes

778e2b5

ericastor merged commit e9e8f59 into llvm:main Nov 15, 2024
4 of 6 checks passed

ericastor deleted the attributes branch November 15, 2024 17:33

[clang] Instantiate attributes on LabelDecls #115924

[clang] Instantiate attributes on LabelDecls #115924

Uh oh!

Conversation

ericastor commented Nov 12, 2024

Uh oh!

llvmbot commented Nov 12, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

ericastor Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

ericastor commented Nov 12, 2024

Uh oh!

erichkeane commented Nov 12, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericastor commented Nov 14, 2024

Uh oh!

ericastor commented Nov 14, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ericastor commented Nov 15, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

ericastor commented Nov 15, 2024

Uh oh!

Uh oh!

Uh oh!