ThinLTO: Fix inline assembly references to static functions with CFI

[samitolvanen]

Summary

Create an internal alias with the original name for static functions
that are renamed in promoteInternals to avoid breaking inline
assembly references to them.

Link: https://github.com/ClangBuiltLinux/linux/issues/1354

[samitolvanen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

93

Note that I'm adding the alias to llvm.compiler.used because it's otherwise removed during optimization. Is there are better way to accomplish this?

[pcc]

LGTM

clang/test/CodeGen/thinlto-cfi-icall-static-inline-asm.c

3 ↗

(On Diff #351253)

Can the test be moved to llvm/test/Transforms/ThinLTOBitcodeWriter and made to use opt -thinlto-bc?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

93

Not as far as I know, that's what I'd recommend.

[samitolvanen]

Moved the test to llvm/test/Transforms/ThinLTOBitcodeWriter.

[thakis]

Looks like this breaks check-llvm on mac, see http://45.33.8.238/mac/32814/summary.html

Please take a look and revert for now if it takes a while to fix.

[samitolvanen]

Fix a use-of-uninitialized-value error.

[nickdesaulniers]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

71–93

Can you avoid making a copy of the OldName by doing the appendToCompilerUsed BEFORE making the dangling reference via ExportGV.setName(NewName);?

[samitolvanen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

71–93

No, I have to rename the existing function before I can create an alias with the same name, and as ExportGV.setName() invalidates Name, I need to create a copy first.

[zequanwu]

Hi, this caused compiler crash: "Assertion `materialized_use_empty() && "Uses remain when a value is destroyed!"'" on chromium build https://ci.chromium.org/ui/p/chromium/builders/try/linux-official/151/overview.
Here is the error message:

While deleting: void ()* %
Use still stuck around after Def is destroyed:i8* bitcast (void ()* <badref> to i8*)
While deleting: void ()* %
Use still stuck around after Def is destroyed:i8* bitcast (voidld.lld: /usr/local/google/home/zequanwu/llvm-project/llvm/lib/IR/Value.cpp:103: llvm::Value::~Value(): Assertion `materialized_use_empty() && "Uses remain when a value is destroyed!"' failed.
 ()* <badref> to i8*)

[samitolvanen]

Thanks for the revert, I'll take a look.

[samitolvanen]

Moved the alias creation to module level inline assembly to avoid issues with LowerTypeTestsModule, based on pcc's suggestion.

[nickdesaulniers]

Change LGTM, but I don't understand why the following tests are modified:

• llvm/test/ThinLTO/X86/devirt2.ll
• llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
• llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll

[samitolvanen]

In D104058#2877631, @nickdesaulniers wrote:

Change LGTM, but I don't understand why the following tests are modified:

• llvm/test/ThinLTO/X86/devirt2.ll

This is needed to fix two missing symbol resolution errors that are caused by the aliases we added.

• llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
• llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll

And for these, we need to specify a target triple to use module inline assembly. According to pcc, there shouldn't be a real-world situation where the triple is missing, but these two tests don't currently specify one.

[nickdesaulniers]

In D104058#2878083, @samitolvanen wrote:

In D104058#2877631, @nickdesaulniers wrote:

Change LGTM, but I don't understand why the following tests are modified:

• llvm/test/ThinLTO/X86/devirt2.ll

This is needed to fix two missing symbol resolution errors that are caused by the aliases we added.

I'm curious if this will lead to breakages with LTO in general? I suppose not, since it's llvm-lto2 that needs the explicit list of symbols that can be linked against.

• llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
• llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll

And for these, we need to specify a target triple to use module inline assembly. According to pcc, there shouldn't be a real-world situation where the triple is missing, but these two tests don't currently specify one.

Neither of these tests use module inline assembly though, AFAICT?

[samitolvanen]

In D104058#2883804, @nickdesaulniers wrote:

In D104058#2878083, @samitolvanen wrote:

In D104058#2877631, @nickdesaulniers wrote:

Change LGTM, but I don't understand why the following tests are modified:

• llvm/test/ThinLTO/X86/devirt2.ll

This is needed to fix two missing symbol resolution errors that are caused by the aliases we added.

I'm curious if this will lead to breakages with LTO in general? I suppose not, since it's llvm-lto2 that needs the explicit list of symbols that can be linked against.

No, this won't break LTO.

• llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll
• llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll

And for these, we need to specify a target triple to use module inline assembly. According to pcc, there shouldn't be a real-world situation where the triple is missing, but these two tests don't currently specify one.

Neither of these tests use module inline assembly though, AFAICT?

The tests don't, but we add module inline assembly to create the aliases, which means that IR with type metadata will need a target triple for ThinLTO.

[pcc]

LGTM

[samitolvanen]

The tests that now specify a target triple also need ; REQUIRES: x86-registered-target or they will obviously fail. I'll upload another revision.

[samitolvanen]

Added REQUIRES: x86-registered-target to tests.

[nickdesaulniers]

Sorry, I always forget when these are necessary.

[morehouse]

This patch broke the sanitizer-windows bot: https://lab.llvm.org/buildbot/#/builders/127/builds/14257

Failed Tests (4):
  cfi-devirt-lld-thinlto-x86_64 :: anon-namespace.cpp
  cfi-devirt-lld-thinlto-x86_64 :: simple-pass.cpp
  cfi-standalone-lld-thinlto-x86_64 :: anon-namespace.cpp
  cfi-standalone-lld-thinlto-x86_64 :: simple-pass.cpp

Please revert or fix.

[samitolvanen]

In D104058#2891551, @morehouse wrote:

This patch broke the sanitizer-windows bot: https://lab.llvm.org/buildbot/#/builders/127/builds/14257

Failed Tests (4):
  cfi-devirt-lld-thinlto-x86_64 :: anon-namespace.cpp
  cfi-devirt-lld-thinlto-x86_64 :: simple-pass.cpp
  cfi-standalone-lld-thinlto-x86_64 :: anon-namespace.cpp
  cfi-standalone-lld-thinlto-x86_64 :: simple-pass.cpp

Please revert or fix.

This should fix the -msvc targets: https://reviews.llvm.org/D106392

[samitolvanen]

Folded in D106392.

[nickdesaulniers]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

90

Is there more information about "promotion aliases with x86_64-pc-windows-msvc" from D106392? Can we quote these only for msvc target triples? Can we add a comment about the quoting be necessary for those targets?

It's still not clear to me how tests using explicit -linux-gnu triples could fail on -mscv hosts.

[nickdesaulniers]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

90

Also, I think the quoting hurts the readability of the generated asm. Maybe that doesn't matter for LTO, but I'd be curious if we could do such escaping only when necessary? Perhaps that's only when targeting -msvc triples?

[rnk]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

90

I was going to say, the proper way to do this is what MCSymbol::print does:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCSymbol.cpp#L59

That code doesn't seem exhaustive, but at least it escapes quotes. We can't call MC from here due to library layering.

The LLVM IR readability is poor, but inline asm in IR is already hard to read. I wouldn't worry about that, I'd mainly worry about the output of -S.

Quoting always seems fine to me, I guess.

[samitolvanen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

90

Is there more information about "promotion aliases with x86_64-pc-windows-msvc" from D106392?

Yes, the -msvc targets use Visual C++ compatible name mangling, which requires quotes when referred to in inline assembly. Here's a trivial reproducer:

$ cat test.cpp 
static void a(void) {}
void* b(void) { return (void *)a; }
$ clang -flto=thin -fvisibility=default -fsanitize=cfi -c test.cpp
$ clang -flto=thin -fvisibility=default -fsanitize=cfi -target x86_64-pc-windows-msvc -c test.cpp
Either SourceMgr should be available
UNREACHABLE executed at llvm-project/llvm/lib/MC/MCContext.cpp:913!
...

Here's the inline assembly generated when we compile the above example for the -msvc target:

.set ?a@@YAXXZ,?a@@YAXXZ.d7b56b39ccc53bc7515ae1b2533f1e3d

Can we quote these only for msvc target triples? Can we add a comment about the quoting be necessary for those targets?

I assume we could limit this only to -msvc targets, but I feel like that would unnecessarily complicate the code as there's no harm in quoting the names always.

It's still not clear to me how tests using explicit -linux-gnu triples could fail on -mscv hosts.

These tests don't specify a -linux-gnu triple. They are C++ and end up using the default target, which in this case is -msvc.

[samitolvanen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

90

I was going to say, the proper way to do this is what MCSymbol::print does:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCSymbol.cpp#L59

That code doesn't seem exhaustive, but at least it escapes quotes. We can't call MC from here due to library layering.

Is it actually possible to have function names that contain quotes? If so, I suppose we need to do something similar here and escape any quotes in the names.

The LLVM IR readability is poor, but inline asm in IR is already hard to read. I wouldn't worry about that, I'd mainly worry about the output of -S.

The promotion happens only when we write bitcode, so the aliases won't yet exist in the -S output.

[samitolvanen]

As we only care about fixing inline assembly references, mangled names are
not that important in the first place. This version skips any functions
that have unusual characters in their names that would otherwise require
quotes, which includes any functions with MSVC compatible name mangling.

[nickdesaulniers]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

39

What about MCAsmInfoXCOFF::isAcceptableChar? I guess we could be targeting a XCOFF object file format with LTO?

45

Can llvm::any_of or llvm::none_of be used here?
llvm/ADT/STLExtras.h

[samitolvanen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

39

Sure, I'll drop '$' and '@' from the list to play nice with XCOFF.

45

Maybe, but I don't see how they would make this function any cleaner. Did you have something specific in mind?

[samitolvanen]

Also skip functions with names incompatible with XCOFF.

[nickdesaulniers]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

45

Something like?

return any_of(Name, [](const char &C) { return isAlnum(C) || C == '_' || C == '.'; }

or maybe we need !none_of(...)? (not sure if characters of a string can be enumerated this way)

[samitolvanen]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

45

Since we want to ensure all the characters in the string match the predicate, I believe it would make sense to use all_of() instead.

However, I don't see the point of introducing additional complexity to such a trivial function to shave off a couple of lines of code. While it might not actually matter here, using all_of() also seems to generate ~5x as many instructions to execute:

https://godbolt.org/z/Pndfxj6rM

If you feel like the current version is too long, I can drop one line by changing the loop to use:

if (!isAlnum(C) && C != '_' && C != '.')
  return false;

I initially wanted to keep the test identical to MCAsmInfo::isAcceptableChar() to make it easier to see it actually matches, but since it's no longer identical, I suppose that doesn't matter.

Thoughts?

[rnk]

I think the new approach of skipping non C-ish identifier names is reasonable. Looks good to me, but wait for the more active reviewers to stamp it.

[nickdesaulniers]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

45

using all_of() also seems to generate ~5x as many instructions to execute:

Hard to argue with that.

Might be nice to add some test coverage of this code with an mscv target triple though.

[thakis]

Mentioning it here in case others run into the same thing: We bisected a 7x (!) binary size regression to this. Details at https://bugs.chromium.org/p/chromium/issues/detail?id=1261715

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

 #include "llvm/Transforms/IPO/FunctionImport.h"
 #include "llvm/Transforms/IPO/LowerTypeTests.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 using namespace llvm;
 
 namespace {
 
+// Determine if a promotion alias should be created for a symbol name.
+static bool allowPromotionAlias(const std::string &Name) {
+  // Promotion aliases are used only in inline assembly. It's safe to
+  // simply skip unusual names. Subset of MCAsmInfo::isAcceptableChar()
+  // and MCAsmInfoXCOFF::isAcceptableChar().
+  for (const char &C : Name) {
+    if (isAlnum(C) || C == '_' || C == '.')
+      continue;
+    return false;
+  }
+  return true;
+}
+
 // Promote each local-linkage entity defined by ExportM and used by ImportM by
 // changing visibility and appending the given ModuleId.
 void promoteInternals(Module &ExportM, Module &ImportM, StringRef ModuleId,
                       SetVector<GlobalValue *> &PromoteExtra) {
   DenseMap<const Comdat *, Comdat *> RenamedComdats;
   for (auto &ExportGV : ExportM.global_values()) {
     if (!ExportGV.hasLocalLinkage())
       continue;
 
     auto Name = ExportGV.getName();
     GlobalValue *ImportGV = nullptr;
     if (!PromoteExtra.count(&ExportGV)) {
       ImportGV = ImportM.getNamedValue(Name);
       if (!ImportGV)
         continue;
       ImportGV->removeDeadConstantUsers();
       if (ImportGV->use_empty()) {
         ImportGV->eraseFromParent();
         continue;
       }
     }
 
+    std::string OldName = Name.str();
     std::string NewName = (Name + ModuleId).str();
 
     if (const auto *C = ExportGV.getComdat())
       if (C->getName() == Name)
         RenamedComdats.try_emplace(C, ExportM.getOrInsertComdat(NewName));
 
     ExportGV.setName(NewName);
     ExportGV.setLinkage(GlobalValue::ExternalLinkage);
     ExportGV.setVisibility(GlobalValue::HiddenVisibility);
 
     if (ImportGV) {
       ImportGV->setName(NewName);
       ImportGV->setVisibility(GlobalValue::HiddenVisibility);
     }
+
+    if (isa<Function>(&ExportGV) && allowPromotionAlias(OldName)) {
+      // Create a local alias with the original name to avoid breaking
+      // references from inline assembly.
+      std::string Alias = ".set " + OldName + "," + NewName + "\n";
+      ExportM.appendModuleInlineAsm(Alias);
+    }
   }
 
   if (!RenamedComdats.empty())
     for (auto &GO : ExportM.global_objects())
       if (auto *C = GO.getComdat()) {
         auto Replacement = RenamedComdats.find(C);
         if (Replacement != RenamedComdats.end())
           GO.setComdat(Replacement->second);
       }

llvm/test/ThinLTO/X86/devirt2.ll

 ; RUN:   -r=%t1.o,_ZTV1B, \
 ; RUN:   -r=%t1.o,_ZTV1C, \
 ; RUN:   -r=%t1.o,_ZTV1D, \
 ; RUN:   -r=%t1.o,_ZTV1D, \
 ; RUN:   -r=%t1.o,_ZN1D1mEi, \
 ; RUN:   -r=%t1.o,_ZN1D1mEi, \
 ; RUN:   -r=%t1.o,test2, \
 ; RUN:   -r=%t2.o,_ZN1A1nEi,p \
+; RUN:   -r=%t2.o,_ZN1A1nEi, \
 ; RUN:   -r=%t2.o,_ZN1B1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1C1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1D1mEi,p \
 ; RUN:   -r=%t2.o,_ZN1E1mEi,p \
+; RUN:   -r=%t2.o,_ZN1E1mEi, \
 ; RUN:   -r=%t2.o,_ZTV1B, \
 ; RUN:   -r=%t2.o,_ZTV1C, \
 ; RUN:   -r=%t2.o,_ZTV1D, \
 ; RUN:   -r=%t2.o,_ZTV1E, \
 ; RUN:   -r=%t2.o,test2,px \
 ; RUN:   -r=%t2.o,_ZN1A1nEi, \
 ; RUN:   -r=%t2.o,_ZN1B1fEi, \
 ; RUN:   -r=%t2.o,_ZN1C1fEi, \

 ; RUN:   -r=%t1.o,_ZTV1B, \
 ; RUN:   -r=%t1.o,_ZTV1C, \
 ; RUN:   -r=%t1.o,_ZTV1D, \
 ; RUN:   -r=%t1.o,_ZTV1D, \
 ; RUN:   -r=%t1.o,_ZN1D1mEi, \
 ; RUN:   -r=%t1.o,_ZN1D1mEi, \
 ; RUN:   -r=%t1.o,test2, \
 ; RUN:   -r=%t2.o,_ZN1A1nEi,p \
+; RUN:   -r=%t2.o,_ZN1A1nEi, \
 ; RUN:   -r=%t2.o,_ZN1B1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1C1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1D1mEi,p \
 ; RUN:   -r=%t2.o,_ZN1E1mEi,p \
+; RUN:   -r=%t2.o,_ZN1E1mEi, \
 ; RUN:   -r=%t2.o,_ZTV1B, \
 ; RUN:   -r=%t2.o,_ZTV1C, \
 ; RUN:   -r=%t2.o,_ZTV1D, \
 ; RUN:   -r=%t2.o,_ZTV1E, \
 ; RUN:   -r=%t2.o,test2,px \
 ; RUN:   -r=%t2.o,_ZN1A1nEi, \
 ; RUN:   -r=%t2.o,_ZN1B1fEi, \
 ; RUN:   -r=%t2.o,_ZN1C1fEi, \

llvm/test/Transforms/ThinLTOBitcodeWriter/cfi-icall-static-inline-asm.ll

+; REQUIRES: x86-registered-target
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o - %s | llvm-modextract -b -n 0 -o - | llvm-dis | FileCheck %s
+
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK: module asm ".set a,a.[[HASH:[0-9a-f]+]]"
+
+define void @b() {
+  %f = alloca void ()*, align 8
+  ; CHECK: store{{.*}} @a.[[HASH]],{{.*}} %f
+  store void ()* @a, void ()** %f, align 8
+  ; CHECK: %1 = call void ()* asm sideeffect "leaq a(%rip)
+  %1 = call void ()* asm sideeffect "leaq a(%rip), $0\0A\09", "=r,~{dirflag},~{fpsr},~{flags}"()
+  ret void
+}
+
+; CHECK: define{{.*}} @a.[[HASH]](){{.*}} !type
+define internal void @a() !type !0 {
+  ret void
+}
+
+!0 = !{i64 0, !"typeid1"}

llvm/test/Transforms/ThinLTOBitcodeWriter/split-internal2.ll

+; REQUIRES: x86-registered-target
 ; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
 ; RUN: llvm-modextract -b -n 0 -o %t0 %t
 ; RUN: llvm-modextract -b -n 1 -o %t1 %t
 ; RUN: not llvm-modextract -b -n 2 -o - %t 2>&1 | FileCheck --check-prefix=ERROR %s
 ; RUN: llvm-dis -o - %t0 | FileCheck --check-prefix=M0 %s
 ; RUN: llvm-dis -o - %t1 | FileCheck --check-prefix=M1 %s
 ; RUN: llvm-bcanalyzer -dump %t0 | FileCheck --check-prefix=BCA0 %s
 ; RUN: llvm-bcanalyzer -dump %t1 | FileCheck --check-prefix=BCA1 %s
 
+target triple = "x86_64-unknown-linux-gnu"
+
 ; ERROR: llvm-modextract: error: module index out of range; bitcode file contains 2 module(s)
 
 ; BCA0: <GLOBALVAL_SUMMARY_BLOCK
 ; BCA1-NOT: <GLOBALVAL_SUMMARY_BLOCK
 
 ; M0: @g = external global void ()*{{$}}
 ; M1: @g = global void ()* @f.13757e0fb71915e385efa4dc9d1e08fd, !type !0
 @g = global void ()* @f, !type !0

llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc-internal.ll

+; REQUIRES: x86-registered-target
 ; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
 ; RUN: llvm-modextract -b -n 0 -o - %t | llvm-dis | FileCheck --check-prefix=M0 %s
 ; RUN: llvm-modextract -b -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=M1 %s
 
+target triple = "x86_64-unknown-linux-gnu"
+
 define [1 x i8*]* @source() {
   ret [1 x i8*]* @g
 }
 
 ; M0: @g.84f59439b469192440047efc8de357fb = external hidden constant [1 x i8*]{{$}}
 ; M1: @g.84f59439b469192440047efc8de357fb = hidden constant [1 x i8*] [i8* bitcast (i64 (i8*)* @ok.84f59439b469192440047efc8de357fb to i8*)]
 @g = internal constant [1 x i8*] [
   i8* bitcast (i64 (i8*)* @ok to i8*)