Support attribute used in member funcs of class templates II

[rafauler]

Summary

As PR17480 describes, clang does not support the used attribute
for member functions of class templates. This means that if the member
function is not used, its definition is never instantiated. This patch
changes clang to emit the definition if it has the used attribute.

This is a second attempt after the revert of D56928. The original diff
broke the swift toolchain because its source contained the attribute
used in a member function that was later explictly specialized. A
check for C++14 [temp.expl.spec]p6 was mistakenly firing here: D56928
set the point of instantiation of member functions with attribute used
at the same point of instantiation of its containing class template.
When a explicit specialization was done, Clang was complaining that a
explicit specialization cannot occur after the use recorded to be at
the instantiation of the class template. The problem is that the use
was not real, but induced by the attribute used. To avoid running
into this check, I no longer pass a valid SourceLocation to
MarkFunctionReferenced. The goal is to cause the function to be
instantiated at the end of the TU, but without a valid point of
instantiation, making this use not interfere anymore with the check
for C++14 [temp.expl.spec]p6.

Test Plan: Kept the original testcase and added another one to cover the
swift runtime issue. Also tested this diff in the swift build and
verified it is working.

[modocache]

Friendly ping! @aaron.ballman it looks like you accepted D56928, could you take a look at this as well?

[aaron.ballman]

In D58216#1414864, @modocache wrote:

Friendly ping! @aaron.ballman it looks like you accepted D56928, could you take a look at this as well?

Sorry for the delay -- I was at WG21 meetings and then on vacation. I think the behavior in D56928 was correct; at least, it matches the behavior of GCC: https://godbolt.org/z/Un5ugT whereas the current patch does not.

[rafauler]

Both approaches make sense to me. I'll re-land the previous patch in favor of gcc compatibility, since the semantics of attribute used in member functions of class templates were first implemented in gcc.

@davezarzycki Heads up that this will land again. Can you change the code in swift to use the attribute used in the declaration of the specialization, not in the declaration of the template? (that is, if you really need the attribute, of course).

[aaron.ballman]

In D58216#1420149, @rafauler wrote:

Both approaches make sense to me. I'll re-land the previous patch in favor of gcc compatibility, since the semantics of attribute used in member functions of class templates were first implemented in gcc.

I think we should be compatible with GCC here.

@davezarzycki Heads up that this will land again. Can you change the code in swift to use the attribute used in the declaration of the specialization, not in the declaration of the template? (that is, if you really need the attribute, of course).

I know very little about the Swift code base. Can it compile with GCC? If it can, then I think something is missing from the test case. If it can't compile with GCC, then I'm guessing the attribute was effectively a noop before and can be removed.

[rafauler]

I'm not an expert in swift either. I'll wait for @davezarzycki input on what's happening, but I suspect the code base is indeed incompatible with gcc due to the nature of the error they experienced. This testcase is creduce-d from what I observed in swift.

[davezarzycki]

I can't get Swift top-of-tree to build with gcc (8.3.1 20190223 (Red Hat 8.3.1-2)), and I doubt that it is supported, but I could be wrong.

In any case, the requested change to the Swift compiler source sounds reasonable to me.

CC: @bob.wilson – Just FYI, top-of-tree clang will probably force a subtle change to the "master" and "upstream-with-swift" branches of Swift.

[bob.wilson]

OK, that doesn't sound like it will be a problem

[davezarzycki]

I've got a patch pending for Swift. That being said, the clang diagnostics leave something to be desired. If __attribute__((used)) on definitions is sloppy at best or wrong at worst, then that should have a warning/error. Want a bug report? Or does one exist already?

[rafauler]

I definitely understand how the diagnostic can be confusing. However, it's the same diagnostic gcc provides too, so gcc users wouldn't be surprised. But you're right this can be improved by at least mentioning the attribute used in the diagnostic message. There's no bug report to this yet, it would be nice to file it.

[davezarzycki]

https://bugs.llvm.org/show_bug.cgi?id=41016

lib/Sema/SemaTemplateInstantiateDecl.cpp

TemplateDeclInstantiator::VisitCXXMethodDecl(CXXMethodDecl *D,
 
   // Otherwise, add the declaration.  We don't need to do this for
   // class-scope specializations because we'll have matched them with
   // the appropriate template.
   } else if (!IsClassScopeSpecialization) {
     Owner->addDecl(Method);
   }
 
+  // PR17480: Honor the used attribute to instantiate member function
+  // definitions
+  if (Method->hasAttr<UsedAttr>())
+    SemaRef.MarkFunctionReferenced(SourceLocation(), Method);
+
   return Method;
 }
 
 Decl *TemplateDeclInstantiator::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
   return VisitCXXMethodDecl(D);
 }
 
 Decl *TemplateDeclInstantiator::VisitCXXDestructorDecl(CXXDestructorDecl *D) {

test/CodeGenCXX/attr-used-member-function-implicit-instantiation.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+// Check that PR17480 is fixed: __attribute__((used)) ignored in templated
+// classes
+namespace InstantiateUsedMemberDefinition {
+template <typename T>
+struct S {
+  int __attribute__((used)) f() {
+    return 0;
+  }
+};
+
+void test() {
+  // Check that InstantiateUsedMemberDefinition::S<int>::f() is defined
+  // as a result of the S class template implicit instantiation
+  // CHECK: define linkonce_odr i32 @_ZN31InstantiateUsedMemberDefinition1SIiE1fEv
+  S<int> inst;
+}
+} // namespace InstantiateUsedMemberDefinition

test/SemaTemplate/attr-used-member-function-implicit-instantiation.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// Check that __attribute__((used)) in member functions of templated classes
+// does not break explicit specializations of those functions
+
+// expected-no-diagnostics
+template <typename>
+struct a {
+  void verify() __attribute__((__used__));
+};
+using b = a<int>;
+template<> void b::verify() {}