[LinkerWrapper] Fix static library symbol resolution

[jhuber6]

Summary

The linker wrapper performs its own very basic symbol resolution for the
purpose of supporting standard static library semantics. We do this here
because the Nvidia nvlink wrapper does not support static linking and
we have some offloading specific extensions.

Currently, we always place symbols in the "table" even if they aren't
extracted. This caused the logic to fail when many files were used that
referenced the same undefined variable. This patch changes the pass to
only add the symbols to the global "table" if the file is actually
extracted.

[tra]

LGTM in general.

clang/test/Driver/linker-wrapper-libs.c

28

How does this test test the functionality of the undefined symbol? E.g. how does it fail now, before the patch?

Is there an explicit check we could to do to make sure things work as intended as opposed to "there's no obvious error" which may also mean "we forgot to process *undefined.bc".

[jhuber6]

clang/test/Driver/linker-wrapper-libs.c

28

Yeah, I wasn't sure how to define a good test for this. The problem I encountered before making this patch was that having another file that used an undefined symbol would override the NewSymbol check and then would prevent it from being extracted. So this checks that case.

[tra]

clang/test/Driver/linker-wrapper-libs.c

28

AFAICT, with -DUNDEFINED, the file would have only extern int sym;. CE says suggests that it produces an embty bitcode file: https://godbolt.org/z/EY9a8Pfeb

What exactly is supposed to be in the *.undefined.bc ? If it's intended to have an undefined reference to sym you need to add some sort of a reference to it.

[jhuber6]

Fix test

clang/test/Driver/linker-wrapper-libs.c

28

Good catch, forgot about that. It's why the other use of extern sym returns it.

clang/test/Driver/linker-wrapper-libs.c

 // REQUIRES: x86-registered-target
 // REQUIRES: nvptx-registered-target
 // REQUIRES: amdgpu-registered-target
 
 // RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.elf.o
 
 #if defined(RESOLVES)
 int __attribute__((visibility("hidden"))) sym;
 #elif defined(GLOBAL)
 int __attribute__((visibility("protected"))) global;
 #elif defined(WEAK)
 int __attribute__((visibility("hidden"))) weak;
 #elif defined(HIDDEN)
 int __attribute__((visibility("hidden"))) hidden;
+#elif defined(UNDEFINED)
+extern int sym;
+int baz() { return sym; }
 #else
 extern int sym;
 
 extern int __attribute__((weak)) weak;
 
 int foo() { return sym; }
 int bar() { return weak; }
 #endif
 
 //
 // Check that we extract a static library defining an undefined symbol.
 //
 // RUN: %clang -cc1 %s -triple nvptx64-nvidia-cuda -emit-llvm-bc -DRESOLVES -o %t.nvptx.resolves.bc
 // RUN: %clang -cc1 %s -triple amdgcn-amd-amdhsa -emit-llvm-bc -DRESOLVES -o %t.amdgpu.resolves.bc
+// RUN: %clang -cc1 %s -triple nvptx64-nvidia-cuda -emit-llvm-bc -DUNDEFINED -o %t.nvptx.undefined.bc
+// RUN: %clang -cc1 %s -triple amdgcn-amd-amdhsa -emit-llvm-bc -DUNDEFINED -o %t.amdgpu.undefined.bc
 // RUN: clang-offload-packager -o %t-lib.out \
+// RUN:   --image=file=%t.nvptx.undefined.bc,kind=openmp,triple=nvptx64-nvidia-cuda,arch=sm_70 \
+// RUN:   --image=file=%t.amdgpu.undefined.bc,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx1030 \
 // RUN:   --image=file=%t.nvptx.resolves.bc,kind=openmp,triple=nvptx64-nvidia-cuda,arch=sm_70 \
 // RUN:   --image=file=%t.amdgpu.resolves.bc,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx1030
 // RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.o -fembed-offload-object=%t-lib.out
 // RUN: llvm-ar rcs %t.a %t.o
 // RUN: clang-offload-packager -o %t.out \
 // RUN:   --image=file=%t.elf.o,kind=openmp,triple=nvptx64-nvidia-cuda,arch=sm_70 \
 // RUN:   --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx1030
 // RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t.o -fembed-offload-object=%t.out

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

enum Symbol : uint32_t {
   Sym_None = 0,
   Sym_Undefined = 1U << 1,
   Sym_Weak = 1U << 2,
 };
 
 /// Scan the symbols from a BitcodeFile \p Buffer and record if we need to
 /// extract any symbols from it.
 Expected<bool> getSymbolsFromBitcode(MemoryBufferRef Buffer, OffloadKind Kind,
-                                     StringSaver &Saver,
+                                     bool IsArchive, StringSaver &Saver,
                                      DenseMap<StringRef, Symbol> &Syms) {
   Expected<IRSymtabFile> IRSymtabOrErr = readIRSymtab(Buffer);
   if (!IRSymtabOrErr)
     return IRSymtabOrErr.takeError();
 
-  bool ShouldExtract = false;
+  bool ShouldExtract = !IsArchive;
+  DenseMap<StringRef, Symbol> TmpSyms;
   for (unsigned I = 0; I != IRSymtabOrErr->Mods.size(); ++I) {
     for (const auto &Sym : IRSymtabOrErr->TheReader.module_symbols(I)) {
       if (Sym.isFormatSpecific() || !Sym.isGlobal())
         continue;
 
       bool NewSymbol = Syms.count(Sym.getName()) == 0;
-      auto &OldSym = Syms[Saver.save(Sym.getName())];
+      auto OldSym = NewSymbol ? Sym_None : Syms[Sym.getName()];
 
       // We will extract if it defines a currenlty undefined non-weak symbol.
       bool ResolvesStrongReference =
           ((OldSym & Sym_Undefined && !(OldSym & Sym_Weak)) &&
            !Sym.isUndefined());
       // We will extract if it defines a new global symbol visible to the host.
       // This is only necessary for code targeting an offloading language.
       bool NewGlobalSymbol =
           ((NewSymbol || (OldSym & Sym_Undefined)) && !Sym.isUndefined() &&
            !Sym.canBeOmittedFromSymbolTable() && Kind != object::OFK_None &&
            (Sym.getVisibility() != GlobalValue::HiddenVisibility));
       ShouldExtract |= ResolvesStrongReference | NewGlobalSymbol;
 
       // Update this symbol in the "table" with the new information.
       if (OldSym & Sym_Undefined && !Sym.isUndefined())
-        OldSym = static_cast<Symbol>(OldSym & ~Sym_Undefined);
+        TmpSyms[Saver.save(Sym.getName())] =
+            static_cast<Symbol>(OldSym & ~Sym_Undefined);
       if (Sym.isUndefined() && NewSymbol)
-        OldSym = static_cast<Symbol>(OldSym | Sym_Undefined);
+        TmpSyms[Saver.save(Sym.getName())] =
+            static_cast<Symbol>(OldSym | Sym_Undefined);
       if (Sym.isWeak())
-        OldSym = static_cast<Symbol>(OldSym | Sym_Weak);
+        TmpSyms[Saver.save(Sym.getName())] =
+            static_cast<Symbol>(OldSym | Sym_Weak);
     }
   }
 
+  // If the file gets extracted we update the table with the new symbols.
+  if (ShouldExtract)
+    Syms.insert(std::begin(TmpSyms), std::end(TmpSyms));
+
   return ShouldExtract;
 }
 
 /// Scan the symbols from an ObjectFile \p Obj and record if we need to extract
 /// any symbols from it.
 Expected<bool> getSymbolsFromObject(const ObjectFile &Obj, OffloadKind Kind,
-                                    StringSaver &Saver,
+                                    bool IsArchive, StringSaver &Saver,
                                     DenseMap<StringRef, Symbol> &Syms) {
-  bool ShouldExtract = false;
+  bool ShouldExtract = !IsArchive;
+  DenseMap<StringRef, Symbol> TmpSyms;
   for (SymbolRef Sym : Obj.symbols()) {
     auto FlagsOrErr = Sym.getFlags();
     if (!FlagsOrErr)
       return FlagsOrErr.takeError();
 
     if (!(*FlagsOrErr & SymbolRef::SF_Global) ||
         (*FlagsOrErr & SymbolRef::SF_FormatSpecific))
       continue;
 
     auto NameOrErr = Sym.getName();
     if (!NameOrErr)
       return NameOrErr.takeError();
 
     bool NewSymbol = Syms.count(*NameOrErr) == 0;
-    auto &OldSym = Syms[Saver.save(*NameOrErr)];
+    auto OldSym = NewSymbol ? Sym_None : Syms[*NameOrErr];
 
     // We will extract if it defines a currenlty undefined non-weak symbol.
     bool ResolvesStrongReference = (OldSym & Sym_Undefined) &&
                                    !(OldSym & Sym_Weak) &&
                                    !(*FlagsOrErr & SymbolRef::SF_Undefined);
 
     // We will extract if it defines a new global symbol visible to the host.
     // This is only necessary for code targeting an offloading language.
     bool NewGlobalSymbol =
         ((NewSymbol || (OldSym & Sym_Undefined)) &&
          !(*FlagsOrErr & SymbolRef::SF_Undefined) && Kind != object::OFK_None &&
          !(*FlagsOrErr & SymbolRef::SF_Hidden));
     ShouldExtract |= ResolvesStrongReference | NewGlobalSymbol;
 
     // Update this symbol in the "table" with the new information.
     if (OldSym & Sym_Undefined && !(*FlagsOrErr & SymbolRef::SF_Undefined))
-      OldSym = static_cast<Symbol>(OldSym & ~Sym_Undefined);
+      TmpSyms[Saver.save(*NameOrErr)] =
+          static_cast<Symbol>(OldSym & ~Sym_Undefined);
     if (*FlagsOrErr & SymbolRef::SF_Undefined && NewSymbol)
-      OldSym = static_cast<Symbol>(OldSym | Sym_Undefined);
+      TmpSyms[Saver.save(*NameOrErr)] =
+          static_cast<Symbol>(OldSym | Sym_Undefined);
     if (*FlagsOrErr & SymbolRef::SF_Weak)
-      OldSym = static_cast<Symbol>(OldSym | Sym_Weak);
+      TmpSyms[Saver.save(*NameOrErr)] = static_cast<Symbol>(OldSym | Sym_Weak);
   }
+
+  // If the file gets extracted we update the table with the new symbols.
+  if (ShouldExtract)
+    Syms.insert(std::begin(TmpSyms), std::end(TmpSyms));
+
   return ShouldExtract;
 }
 
 /// Attempt to 'resolve' symbols found in input files. We use this to
 /// determine if an archive member needs to be extracted. An archive member
 /// will be extracted if any of the following is true.
 ///   1) It defines an undefined symbol in a regular object filie.
 ///   2) It defines a global symbol without hidden visibility that has not
 ///      yet been defined.
-Expected<bool> getSymbols(StringRef Image, OffloadKind Kind, StringSaver &Saver,
+Expected<bool> getSymbols(StringRef Image, OffloadKind Kind, bool IsArchive,
+                          StringSaver &Saver,
                           DenseMap<StringRef, Symbol> &Syms) {
   MemoryBufferRef Buffer = MemoryBufferRef(Image, "");
   switch (identify_magic(Image)) {
   case file_magic::bitcode:
-    return getSymbolsFromBitcode(Buffer, Kind, Saver, Syms);
+    return getSymbolsFromBitcode(Buffer, Kind, IsArchive, Saver, Syms);
   case file_magic::elf_relocatable: {
     Expected<std::unique_ptr<ObjectFile>> ObjFile =
         ObjectFile::createObjectFile(Buffer);
     if (!ObjFile)
       return ObjFile.takeError();
-    return getSymbolsFromObject(**ObjFile, Kind, Saver, Syms);
+    return getSymbolsFromObject(**ObjFile, Kind, IsArchive, Saver, Syms);
   }
   default:
     return false;
   }
 }
 
 /// Search the input files and libraries for embedded device offloading code
 /// and add it to the list of files to be linked. Files coming from static
while (Extracted) {
         if (!Binary.getBinary())
           continue;
 
         // If we don't have an object file for this architecture do not
         // extract.
         if (IsArchive && !WholeArchive && !Syms.count(Binary))
           continue;
 
-        Expected<bool> ExtractOrErr = getSymbols(
-            Binary.getBinary()->getImage(),
-            Binary.getBinary()->getOffloadKind(), Saver, Syms[Binary]);
+        Expected<bool> ExtractOrErr =
+            getSymbols(Binary.getBinary()->getImage(),
+                       Binary.getBinary()->getOffloadKind(), IsArchive, Saver,
+                       Syms[Binary]);
         if (!ExtractOrErr)
           return ExtractOrErr.takeError();
 
-        Extracted = IsArchive && !WholeArchive && *ExtractOrErr;
+        Extracted = !WholeArchive && *ExtractOrErr;
 
         if (!IsArchive || WholeArchive || Extracted)
           InputFiles.emplace_back(std::move(Binary));
 
         // If we extracted any files we need to check all the symbols again.
         if (Extracted)
           break;
       }