[Clang][Sema] Warn when 'exclude_from_explicit_instantiation' attribute is used on local classes and members thereof #88777

sdkrystian · 2024-04-15T18:47:59Z

A local class and its members declared in a function template are instantiated alongside the definition of that template. It therefore does not make sense to apply the exclude_from_explicit_instantiation attribute to such declarations, and this patch adds a warning to diagnose these cases (in addition to ignoring the attribute).

(The motivation for this patch is to fix a failing test in libc++ for #84050. In particular, this line in libc++ contains the expression this->__value_ (reduced repro here) which will be looked up prior to instantiation (the lookup context is the current instantiation) once #84050 lands. _LIBCPP_HIDE_FROM_ABI includes __attribute__((exclude_from_explicit_instantiation)), which TLDR in the reduced repro results in Local being instantiated with Local::operator A as its DeclContext)

llvmbot · 2024-04-15T18:48:31Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

A local class and its members declared in a function template are instantiated alongside the definition of that template. It therefore does not make sense to apply the exclude_from_explicit_instantiation attribute to such declarations, and this patch adds a warning to diagnose these cases (in addition to ignoring the attribute).

(The motivation for this patch is to fix a failing test in libc++ for #84050. In particular, this line in libc++ contains the expression this->__value_ (reduced repro here) which will be looked up prior to instantiation (the lookup context is the current instantiation) once #84050 lands. _LIBCPP_HIDE_FROM_ABI includes __attribute__((exclude_from_explicit_instantiation)), which TLDR in the reduced repro results in Local being instantiated with Local::operator A as its DeclContext)

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

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+18)
(added) clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.local-class.cpp (+63)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 5ec0218aedfe86..5385c634ca7c45 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3669,6 +3669,9 @@ def warn_attribute_dllexport_explicit_instantiation_decl : Warning<
 def warn_attribute_dllexport_explicit_instantiation_def : Warning<
   "'dllexport' attribute ignored on explicit instantiation definition">,
   InGroup<IgnoredAttributes>;
+def warn_attribute_exclude_from_explicit_instantiation_local_class : Warning<
+  "%0 attribute ignored on local class%select{| member}1">,
+  InGroup<IgnoredAttributes>;
 def warn_invalid_initializer_from_system_header : Warning<
   "invalid constructor from class in system header, should not be explicit">,
   InGroup<DiagGroup<"invalid-initializer-from-system-header">>;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index b7b1fbc625a150..dec9fc4cea234d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -983,6 +983,21 @@ static void handleErrorAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
     D->addAttr(EA);
 }
 
+static void handleExcludeFromExplicitInstantiationAttr(Sema &S, Decl *D,
+                                                       const ParsedAttr &AL) {
+  const auto *PD = isa<CXXRecordDecl>(D)
+                       ? cast<DeclContext>(D)
+                       : D->getDeclContext()->getRedeclContext();
+  if (const auto *RD = dyn_cast<CXXRecordDecl>(PD); RD && RD->isLocalClass()) {
+    S.Diag(AL.getLoc(),
+           diag::warn_attribute_exclude_from_explicit_instantiation_local_class)
+        << AL << /*IsMember=*/!isa<CXXRecordDecl>(D);
+    return;
+  }
+  D->addAttr(::new (S.Context)
+                 ExcludeFromExplicitInstantiationAttr(S.Context, AL));
+}
+
 namespace {
 /// Determines if a given Expr references any of the given function's
 /// ParmVarDecls, or the function's implicit `this` parameter (if applicable).
@@ -9315,6 +9330,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
   case ParsedAttr::AT_Error:
     handleErrorAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_ExcludeFromExplicitInstantiation:
+    handleExcludeFromExplicitInstantiationAttr(S, D, AL);
+    break;
   case ParsedAttr::AT_DiagnoseIf:
     handleDiagnoseIfAttr(S, D, AL);
     break;
diff --git a/clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.local-class.cpp b/clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.local-class.cpp
new file mode 100644
index 00000000000000..bac11d9cffbc23
--- /dev/null
+++ b/clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.local-class.cpp
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Test that the exclude_from_explicit_instantiation attribute is ignored
+// for local classes and members thereof.
+
+#define EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((exclude_from_explicit_instantiation)) // expected-note 0+{{expanded from macro}}
+
+namespace N0 {
+
+template<typename T>
+void f() {
+  struct EXCLUDE_FROM_EXPLICIT_INSTANTIATION A { // expected-warning {{attribute ignored on local class}}
+                                                 // expected-note@-1 2{{in instantiation of}}
+    EXCLUDE_FROM_EXPLICIT_INSTANTIATION void g(T t) { // expected-warning {{attribute ignored on local class member}}
+      *t; // expected-error {{indirection requires pointer operand ('int' invalid)}}
+    }
+
+    struct EXCLUDE_FROM_EXPLICIT_INSTANTIATION B { // expected-warning {{attribute ignored on local class}}
+      void h(T t) {
+        *t; // expected-error {{indirection requires pointer operand ('int' invalid)}}
+      }
+    };
+  };
+}
+
+template void f<int>(); // expected-note 2{{in instantiation of}}
+
+}
+
+// This is a reduced example from libc++ which required that 'value'
+// be prefixed with 'this->' because the definition of 'Local::operator A'
+// was not instantiated when the definition of 'g' was.
+namespace N1 {
+
+  struct A { };
+
+  struct B {
+    operator A() {
+      return A();
+    }
+  };
+
+  auto f(auto&& x) {
+    return A(x);
+  }
+
+  template<typename T>
+  auto g(T x) {
+    struct Local {
+      T value;
+
+      EXCLUDE_FROM_EXPLICIT_INSTANTIATION // expected-warning {{attribute ignored on local class member}}
+      operator A() {
+        return A(value);
+      }
+    };
+
+    return f(Local(x));
+  }
+
+  auto x = g(B());
+
+}

erichkeane

Needs a release note, probably int he 'potentially breaking changes' section?

erichkeane · 2024-04-15T20:14:25Z

clang/lib/Sema/SemaDeclAttr.cpp

@@ -9315,6 +9330,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
  case ParsedAttr::AT_Error:
    handleErrorAttr(S, D, AL);
    break;
+  case ParsedAttr::AT_ExcludeFromExplicitInstantiation:


What is the previous 'handle' function? Typically with decl attributes we need have an entry on this switch.

exclude_from_explicit_instantiation had no 'handle' function prior to this patch -- the only restriction was that it must be applied to the declaration of a variable, function, or class.

It still needs to be 'added' to the AST though? Where was that happening?

In the default case of the switch statement (line 9142, AL.getInfo().handleDeclAttribute(S, D, AL))

sdkrystian · 2024-04-16T19:39:18Z

@erichkeane Perhaps the release note should go under "Attribute Changes in Clang"?

erichkeane · 2024-04-16T19:40:53Z

@erichkeane Perhaps the release note should go under "Attribute Changes in Clang"?

That would be fine for me as well.

…for members of local classes

sdkrystian · 2024-04-17T13:28:12Z

Release note added

mikaelholmen · 2024-04-19T05:26:04Z

I noticed that when compiling libcxx with this patch there are zillions of warnings like

[1950/2073] Building CXX object libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.o
In file included from /repo/libcxx/src/filesystem/operations.cpp:16:
In file included from /repo/llvm/build-all-builtins/include/c++/v1/iterator:687:
In file included from /repo/llvm/build-all-builtins/include/c++/v1/__iterator/common_iterator.h:31:
/repo/llvm/build-all-builtins/include/c++/v1/variant:912:9: warning: '__exclude_from_explicit_instantiation__' attribute ignored on local class member [-Wignored-attributes]
  912 |         _LIBCPP_HIDE_FROM_ABI void operator()(true_type) const { __this->__emplace<_Ip>(std::forward<_Arg>(__arg)); }
      |         ^
/repo/llvm/build-all-builtins/include/c++/v1/__config:792:22: note: expanded from macro '_LIBCPP_HIDE_FROM_ABI'
  792 |       _LIBCPP_HIDDEN _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION                                                       \
      |                      ^
/repo/llvm/build-all-builtins/include/c++/v1/__config:695:72: note: expanded from macro '_LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION'
  695 | #    define _LIBCPP_EXCLUDE_FROM_EXPLICIT_INSTANTIATION __attribute__((__exclude_from_explicit_instantiation__))
      |                                                                        ^

Is there any plan to clean this out?

sdkrystian · 2024-04-19T10:40:04Z

@mikaelholmen I believe there are two uses of exclude_from_explicit_instantiation in libc++ which are responsible for all these warnings. I was assuming that someone more experienced with the libc++ side of things would handle it, but I can submit a patch that addresses these myself.

mikaelholmen · 2024-04-19T11:27:53Z

Nice! I have no idea about this I just saw the warnings when I compiled and connected them to this patch.

sdkrystian · 2024-04-19T12:28:25Z

@mikaelholmen See #89377

… on local class members (#89377) #88777 adds a warning for when the `exclude_from_explicit_instantiation` attribute is applied to local classes and members thereof. This patch addresses the few instances of `exclude_from_explicit_instantiation` being applied to local class members in libc++.

… on local class members (llvm#89377) llvm#88777 adds a warning for when the `exclude_from_explicit_instantiation` attribute is applied to local classes and members thereof. This patch addresses the few instances of `exclude_from_explicit_instantiation` being applied to local class members in libc++.

sdkrystian requested a review from erichkeane April 15, 2024 18:47

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

sdkrystian force-pushed the suppress-expl-inst-local branch from bc39616 to c4a31c7 Compare April 15, 2024 19:03

erichkeane reviewed Apr 15, 2024

View reviewed changes

[Clang][Sema] Ignore 'exclude_from_explicit_instantiation' attribute …

5f19ff4

…for members of local classes

sdkrystian force-pushed the suppress-expl-inst-local branch from c4a31c7 to 72fec49 Compare April 17, 2024 13:00

sdkrystian added 2 commits April 17, 2024 09:17

[FOLD] add warning + tests

695e618

[FOLD] add release note

2cb7815

sdkrystian force-pushed the suppress-expl-inst-local branch from 72fec49 to 2cb7815 Compare April 17, 2024 13:27

sdkrystian requested a review from erichkeane April 17, 2024 13:27

erichkeane approved these changes Apr 17, 2024

View reviewed changes

sdkrystian merged commit 652ae4e into llvm:main Apr 18, 2024

sdkrystian mentioned this pull request Apr 19, 2024

[libc++] Fix usage of 'exclude_from_explicit_instantiation' attribute on local class members #89377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][Sema] Warn when 'exclude_from_explicit_instantiation' attribute is used on local classes and members thereof #88777

[Clang][Sema] Warn when 'exclude_from_explicit_instantiation' attribute is used on local classes and members thereof #88777

Uh oh!

sdkrystian commented Apr 15, 2024

Uh oh!

llvmbot commented Apr 15, 2024

Uh oh!

erichkeane left a comment

Uh oh!

erichkeane Apr 15, 2024

Uh oh!

sdkrystian Apr 15, 2024

Uh oh!

erichkeane Apr 15, 2024

Uh oh!

sdkrystian Apr 15, 2024

Uh oh!

sdkrystian commented Apr 16, 2024

Uh oh!

erichkeane commented Apr 16, 2024

Uh oh!

sdkrystian commented Apr 17, 2024

Uh oh!

mikaelholmen commented Apr 19, 2024

Uh oh!

sdkrystian commented Apr 19, 2024

Uh oh!

mikaelholmen commented Apr 19, 2024

Uh oh!

sdkrystian commented Apr 19, 2024

Uh oh!

Uh oh!

[Clang][Sema] Warn when 'exclude_from_explicit_instantiation' attribute is used on local classes and members thereof #88777

[Clang][Sema] Warn when 'exclude_from_explicit_instantiation' attribute is used on local classes and members thereof #88777

Uh oh!

Conversation

sdkrystian commented Apr 15, 2024

Uh oh!

llvmbot commented Apr 15, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

sdkrystian Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

sdkrystian Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

sdkrystian commented Apr 16, 2024

Uh oh!

erichkeane commented Apr 16, 2024

Uh oh!

sdkrystian commented Apr 17, 2024

Uh oh!

mikaelholmen commented Apr 19, 2024

Uh oh!

sdkrystian commented Apr 19, 2024

Uh oh!

mikaelholmen commented Apr 19, 2024

Uh oh!

sdkrystian commented Apr 19, 2024

Uh oh!

Uh oh!