LowerTypeTests: Add limited support for aliases

[vlad.tsyrklevich]

Summary

LowerTypeTests moves some function definitions from individual object
files to the merged module, leaving a stub to be called in the merged
module's jump table. If an alias was pointing to such a function
definition LowerTypeTests would fail because the alias would be left
without a definition to point to.

This change 1) emits information about aliases to the ThinLTO summary,

2. replaces aliases pointing to function definitions that are moved to

the merged module with function declarations, and 3) re-emits those
aliases in the merged module pointing to the correct function
definitions.

The patch does not correctly fix all possible mis-uses of aliases in
LowerTypeTests. For example, it does not handle aliases with a different
type from the pointed to function.

The addition of alias data increases the size of Chrome build artifacts
by less than 1%.

[pcc]

Please add some negative tests, e.g. alias of function without jump table entry,

lib/Transforms/IPO/LowerTypeTests.cpp

963 ↗

(On Diff #128651)

if (auto *A = dyn_cast<GlobalAlias>(U.getUser())) {

966 ↗

(On Diff #128651)

I think you want to RAUW the alias with the newly created function declaration, otherwise this will fail if the alias has any uses. See line 775 in this file for an example.

1705 ↗

(On Diff #128651)

Is it possible for ExportedFunctions.count(AliasName) to be false in a case where we need to create an alias in the merged module? It seems possible, e.g. if the alias is referenced from a native object file or a translation unit compiled without cfi-icall.

1711 ↗

(On Diff #128651)

Similarly to my comment above, I think this will need to be a RAUW at the point where the alias is created in case the function declaration has any uses in the merged module.

[vlad.tsyrklevich]

Add negative tests and fix the bugs Peter pointed out.

lib/Transforms/IPO/LowerTypeTests.cpp

1711 ↗

(On Diff #128651)

I don't think it's possible for there to be any uses here since this function declaration is created in the code directly above and can not yet have any uses and none should be generated below (maybe I'm missing something about how such a use would be generated?)

[pcc]

lib/Transforms/IPO/LowerTypeTests.cpp

1711 ↗

(On Diff #128651)

The merged module may contain an ordinary reference to the function in user code (user code will appear in the merged module if it was compiled with full LTO or if it does not export any symbols). In that case we would fail the if condition on line 1654 above and reuse the existing function.

[vlad.tsyrklevich]

RAUW the alias we replace the function declaration with (as Peter mentioned in his comment, it's possible there are uses of it.)

[pcc]

The code looks good, but please add negative tests as well as tests for the functionality you are adding to ThinLTOBitcodeWriter.

[pcc]

Sorry disregard the negative tests comment, i see that you added some to the existing tests.

[vlad.tsyrklevich]

Add a ThinLTO bitcode writer test and fix a trivial UaF I found with an ASan test build.

[pcc]

LGTM

llvm/trunk/lib/Transforms/IPO/LowerTypeTests.cpp

if (F->isDeclarationForLinker() && !isDefinition) {
     FDecl->setVisibility(GlobalValue::HiddenVisibility);
   } else if (isDefinition) {
     F->setName(Name + ".cfi");
     F->setLinkage(GlobalValue::ExternalLinkage);
     F->setVisibility(GlobalValue::HiddenVisibility);
     FDecl = Function::Create(F->getFunctionType(), GlobalValue::ExternalLinkage,
                              Name, &M);
     FDecl->setVisibility(Visibility);
+
+    // Delete aliases pointing to this function, they'll be re-created in the
+    // merged output
+    SmallVector<GlobalAlias*, 4> ToErase;
+    for (auto &U : F->uses()) {
+      if (auto *A = dyn_cast<GlobalAlias>(U.getUser())) {
+        Function *AliasDecl = Function::Create(
+            F->getFunctionType(), GlobalValue::ExternalLinkage, "", &M);
+        AliasDecl->takeName(A);
+        A->replaceAllUsesWith(AliasDecl);
+        ToErase.push_back(A);
+      }
+    }
+    for (auto *A : ToErase)
+      A->eraseFromParent();
   } else {
     // Function definition without type metadata, where some other translation
     // unit contained a declaration with type metadata. This normally happens
     // during mixed CFI + non-CFI compilation. We do nothing with the function
     // so that it is treated the same way as a function defined outside of the
     // LTO unit.
     return;
   }
for (const auto &S : Sets) {
     });
 
     // Build bitsets for this disjoint set.
     buildBitSetsFromDisjointSet(TypeIds, Globals);
   }
 
   allocateByteArrays();
 
+  // Parse alias data to replace stand-in function declarations for aliases
+  // with an alias to the intended target.
+  if (ExportSummary) {
+    if (NamedMDNode *AliasesMD = M.getNamedMetadata("aliases")) {
+      for (auto AliasMD : AliasesMD->operands()) {
+        assert(AliasMD->getNumOperands() >= 4);
+        StringRef AliasName =
+            cast<MDString>(AliasMD->getOperand(0))->getString();
+        StringRef Aliasee = cast<MDString>(AliasMD->getOperand(1))->getString();
+
+        if (!ExportedFunctions.count(Aliasee) ||
+            ExportedFunctions[Aliasee].Linkage != CFL_Definition ||
+            !M.getNamedAlias(Aliasee))
+          continue;
+
+        GlobalValue::VisibilityTypes Visibility =
+            static_cast<GlobalValue::VisibilityTypes>(
+                cast<ConstantAsMetadata>(AliasMD->getOperand(2))
+                    ->getValue()
+                    ->getUniqueInteger()
+                    .getZExtValue());
+        bool Weak =
+            static_cast<bool>(cast<ConstantAsMetadata>(AliasMD->getOperand(3))
+                                  ->getValue()
+                                  ->getUniqueInteger()
+                                  .getZExtValue());
+
+        auto *Alias = GlobalAlias::create("", M.getNamedAlias(Aliasee));
+        Alias->setVisibility(Visibility);
+        if (Weak)
+          Alias->setLinkage(GlobalValue::WeakAnyLinkage);
+
+        if (auto *F = M.getFunction(AliasName)) {
+          Alias->takeName(F);
+          F->replaceAllUsesWith(Alias);
+          F->eraseFromParent();
+        } else {
+          Alias->setName(AliasName);
+        }
+      }
+    }
+  }
+
   return true;
 }
 
 PreservedAnalyses LowerTypeTestsPass::run(Module &M,
                                           ModuleAnalysisManager &AM) {
   bool Changed = LowerTypeTestsModule(M, /*ExportSummary=*/nullptr,
                                       /*ImportSummary=*/nullptr)
                      .lower();
   if (!Changed)
     return PreservedAnalyses::all();
   return PreservedAnalyses::none();
 }

llvm/trunk/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

void splitAndWriteThinLTOBitcode(
   }
 
   if(!CfiFunctionMDs.empty()) {
     NamedMDNode *NMD = MergedM->getOrInsertNamedMetadata("cfi.functions");
     for (auto MD : CfiFunctionMDs)
       NMD->addOperand(MD);
   }
 
+  SmallVector<MDNode *, 8> FunctionAliases;
+  for (auto &A : M.aliases()) {
+    if (!isa<Function>(A.getAliasee()))
+      continue;
+
+    auto *F = cast<Function>(A.getAliasee());
+    auto &Ctx = MergedM->getContext();
+    SmallVector<Metadata *, 4> Elts;
+
+    Elts.push_back(MDString::get(Ctx, A.getName()));
+    Elts.push_back(MDString::get(Ctx, F->getName()));
+    Elts.push_back(ConstantAsMetadata::get(
+        llvm::ConstantInt::get(Type::getInt8Ty(Ctx), A.getVisibility())));
+    Elts.push_back(ConstantAsMetadata::get(
+        llvm::ConstantInt::get(Type::getInt8Ty(Ctx), A.isWeakForLinker())));
+
+    FunctionAliases.push_back(MDTuple::get(Ctx, Elts));
+  }
+
+  if (!FunctionAliases.empty()) {
+    NamedMDNode *NMD = MergedM->getOrInsertNamedMetadata("aliases");
+    for (auto MD : FunctionAliases)
+      NMD->addOperand(MD);
+  }
+
   simplifyExternals(*MergedM);
 
   // FIXME: Try to re-use BSI and PFI from the original module here.
   ProfileSummaryInfo PSI(M);
   ModuleSummaryIndex Index = buildModuleSummaryIndex(M, nullptr, &PSI);
 
   // Mark the merged module as requiring full LTO. We still want an index for
   // it though, so that it can participate in summary-based dead stripping.

llvm/trunk/test/Transforms/LowerTypeTests/Inputs/import-alias.yaml

+---
+TypeIdMap:
+  typeid1:
+    TTRes:
+      Kind:            AllOnes
+      SizeM1BitWidth:  7
+WithGlobalValueDeadStripping: false
+CfiFunctionDefs:
+  - f
+CfiFunctionDecls:
+...

llvm/trunk/test/Transforms/LowerTypeTests/export-alias.ll

+; RUN: opt -S %s -lowertypetests -lowertypetests-summary-action=export -lowertypetests-read-summary=%S/Inputs/use-typeid1-typeid2.yaml | FileCheck %s
+;
+; CHECK: @alias1 = weak alias void (), void ()* @f
+; CHECK: @alias2 = hidden alias void (), void ()* @f
+; CHECK: declare !type !1 void @alias3()
+; CHECK-NOT: @alias3 = alias
+
+target triple = "x86_64-unknown-linux"
+
+!cfi.functions = !{!0, !2, !3}
+!aliases = !{!4, !5, !6}
+
+!0 = !{!"f", i8 0, !1}
+!1 = !{i64 0, !"typeid1"}
+!2 = !{!"alias1", i8 1, !1}
+; alias2 not included here, this could happen if the only reference to alias2
+; is in a module compiled without cfi-icall
+!3 = !{!"alias3", i8 1, !1}
+!4 = !{!"alias1", !"f", i8 0, i8 1}
+!5 = !{!"alias2", !"f", i8 1, i8 0}
+!6 = !{!"alias3", !"not_present", i8 0, i8 0}

llvm/trunk/test/Transforms/LowerTypeTests/import-alias.ll

+; RUN: opt -S %s -lowertypetests -lowertypetests-summary-action=import -lowertypetests-read-summary=%S/Inputs/import-alias.yaml | FileCheck %s
+;
+; Check that the definitions for @f and @f_alias are removed from this module
+; but @g_alias remains.
+;
+; CHECK: @g_alias = alias void (), void ()* @g
+; CHECK: define hidden void @f.cfi
+; CHECK: declare void @f()
+; CHECK: declare void @f_alias()
+
+target triple = "x86_64-unknown-linux"
+
+@f_alias = alias void (), void ()* @f
+@g_alias = alias void (), void ()* @g
+
+; Definition moved to the merged module
+define void @f() {
+  ret void
+}
+
+; Definition not moved to the merged module
+define void @g() {
+  ret void
+}
+
+define void @uses_aliases() {
+  call void @f_alias()
+  call void @g_alias()
+  ret void
+}

llvm/trunk/test/Transforms/ThinLTOBitcodeWriter/function-alias.ll

+; RUN: opt -thinlto-bc -o %t %s
+; RUN: llvm-modextract -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=CHECK1 %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define hidden void @Func() !type !0 {
+  ret void
+}
+
+; CHECK1: !aliases = !{![[A1:[0-9]+]], ![[A2:[0-9]+]], ![[A3:[0-9]+]]}
+
+; CHECK1: ![[A1]] = !{!"Alias", !"Func", i8 1, i8 0}
+; CHECK1: ![[A2]] = !{!"Hidden_Alias", !"Func", i8 1, i8 0}
+; CHECK1: ![[A3]] = !{!"Weak_Alias", !"Func", i8 0, i8 1}
+@Alias = hidden alias void (), void ()* @Func
+@Hidden_Alias = hidden alias void (), void ()* @Func
+@Weak_Alias = weak alias void (), void ()* @Func
+
+@Variable = global i32 0
+
+; Only generate summary alias information for aliases to functions
+; CHECK1-NOT: Variable_Alias
+@Variable_Alias = alias i32, i32* @Variable
+
+!0 = !{i64 0, !"_ZTSFvvE"}