[PR30341] Alias must point to a definition

[hiraditya]

Summary

Inlining the destructor caused the compiler to generate bad IR which failed the Verifier in the backend.
https://llvm.org/bugs/show_bug.cgi?id=30341

This patch disables alias to available_externally definitions.

[sebpop]

clang/lib/CodeGen/CGCXX.cpp

137

Please remove the reference to r254170.
You may want to put a reference to a bug, or better, just describe in full why this is disabled.

166

You may want to adjust this comment, now that the condition does not check for "AvailableExternally".
You can move part of the FIXME in the comment above.

[hiraditya]

Added a test case (contributed by Eric).

[sebpop]

The change looks good to me. Somebody else should approve.

[hiraditya]

ping!

[rsmith]

clang/lib/CodeGen/CGCXX.cpp

137–138

Instead of this, I'd just say:

available_externally definitions aren't real definitions, so we cannot create an alias to one.

or similar. It doesn't seem necessary or relevant to reference a particular PR here.

140–142

It's not clear what this comment about AlwaysInline is referring to, since the code does not mention that.

142–143

What "corresponding LLVM changes" are you expecting here? It seems to be fundamental to aliases that they can only denote definitions.

170

Did you mean to change the behavior here? For non-available_externally functions, we never used to care whether they're always_inline. Why do we care now?

[sebpop]

clang/lib/CodeGen/CGCXX.cpp

140–142

I think we should just remove all the FIXME note: Aditya has just moved this comment from below... to here.

162

... from here ...

170

I think this change does not modify the current behavior: the check has been moved up and we early return in that case.

[hiraditya]

Addressed Richard's comments.

[rsmith]

clang/lib/CodeGen/CGCXX.cpp

170

We can now only reach this code for the case where TargetLinkage != llvm::GlobalValue::AvailableExternallyLinkage. Substituting true for that expression here gives

if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) &&
    (true || !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>())) {

which simplifies to

if (llvm::GlobalValue::isDiscardableIfUnused(Linkage)) {

not

if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) &&
    !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>()) {

We used to enter this case for a function that is discardable if unused, not available externally, and always inline (that is, a normal inline+always_inline function). With this change, we don't do so any more.

[hiraditya]

Addressed Richard's comment.

clang/lib/CodeGen/CGCXX.cpp

169

Thanks for pointing this out.

[hiraditya]

I had to revert the patch because this test case was failing. The problem was that the destructors have different return type for different targets. So I'll modify the testcase and put the patch back in.

clang/lib/CodeGen/CGCXX.cpp

   llvm::GlobalValue::LinkageTypes TargetLinkage =
       getFunctionLinkage(TargetDecl);
 
+  // Disallow aliases to available_externally because available_externally
+  // will not be there in the end to allow the creation of the alias (PR30341).
+  // FIXME: An extern template instantiation will create functions with
+  // linkage "AvailableExternally". In libc++, some classes also define
+  // members with attribute "AlwaysInline" and expect no reference to
+  // be generated. It is desirable to reenable this optimisation after
+  // corresponding LLVM changes.
+  if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage)
+    return true;
+
   // Check if we have it already.
   StringRef MangledName = getMangledName(AliasDecl);
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);

   // Instead of creating as alias to a linkonce_odr, replace all of the uses
   // of the aliasee.
   if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) &&
-     (TargetLinkage != llvm::GlobalValue::AvailableExternallyLinkage ||
-      !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>())) {
-    // FIXME: An extern template instantiation will create functions with
-    // linkage "AvailableExternally". In libc++, some classes also define
-    // members with attribute "AlwaysInline" and expect no reference to
-    // be generated. It is desirable to reenable this optimisation after
-    // corresponding LLVM changes.
+      !TargetDecl.getDecl()->hasAttr<AlwaysInlineAttr>()) {
     addReplacement(MangledName, Aliasee);
     return false;
   }

clang/test/CodeGenCXX/alias-available-externally.cpp

+// RUN: %clang_cc1 -O1 -std=c++11 -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s
+// Clang should not generate alias to available_externally definitions.
+// Check that the destructor of Foo is defined.
+// CHECK: define linkonce_odr void @_ZN3FooD2Ev
+template <class CharT>
+struct String {
+  String() {}
+  ~String();
+};
+
+template <class CharT>
+inline __attribute__((visibility("hidden"), always_inline))
+String<CharT>::~String() {}
+
+extern template struct String<char>;
+
+struct Foo : public String<char> { Foo() { String<char> s; } };
+
+Foo f;