[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 ↗

(On Diff #71692)

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 ↗

(On Diff #71692)

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 ↗

(On Diff #71827)

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 ↗

(On Diff #71827)

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

142–143 ↗

(On Diff #71827)

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

170 ↗

(On Diff #71827)

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 ↗

(On Diff #71827)

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

162 ↗

(On Diff #71827)

... from here ...

170 ↗

(On Diff #71827)

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 ↗

(On Diff #72827)

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 ↗

(On Diff #72898)

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.

cfe/trunk/lib/CodeGen/CGCXX.cpp

bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl,
 
   // We can't use an alias if the linkage is not valid for one.
   if (!llvm::GlobalAlias::isValidLinkage(Linkage))
     return true;
 
   llvm::GlobalValue::LinkageTypes TargetLinkage =
       getFunctionLinkage(TargetDecl);
 
+  // available_externally definitions aren't real definitions, so we cannot
+  // create an alias to one.
+  if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage)
+    return true;
+
   // Check if we have it already.
   StringRef MangledName = getMangledName(AliasDecl);
   llvm::GlobalValue *Entry = GetGlobalValue(MangledName);
   if (Entry && !Entry->isDeclaration())
     return false;
   if (Replacements.count(MangledName))
     return false;
 
   // Derive the type for the alias.
   llvm::Type *AliasValueType = getTypes().GetFunctionType(AliasDecl);
   llvm::PointerType *AliasType = AliasValueType->getPointerTo();
 
   // Find the referent.  Some aliases might require a bitcast, in
   // which case the caller is responsible for ensuring the soundness
   // of these semantics.
   auto *Ref = cast<llvm::GlobalValue>(GetAddrOfGlobal(TargetDecl));
   llvm::Constant *Aliasee = Ref;
   if (Ref->getType() != AliasType)
     Aliasee = llvm::ConstantExpr::getBitCast(Ref, AliasType);
 
   // 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.
+  if (llvm::GlobalValue::isDiscardableIfUnused(Linkage)) {
     addReplacement(MangledName, Aliasee);
     return false;
   }
 
   // If we have a weak, non-discardable alias (weak, weak_odr), like an extern
   // template instantiation or a dllexported class, avoid forming it on COFF.
   // A COFF weak external alias cannot satisfy a normal undefined symbol
   // reference from another TU. The other TU must also mark the referenced

cfe/trunk/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;