[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa26bd95325f1: [LinkerWrapper] Fix static library symbol
resolution (authored by jhuber6).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151839/new/
https://reviews.llvm.org/D151839
Files:
clang/test/Driver/linker-wrapper-libs.c
clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -1163,20 +1163,21 @@
/// Scan the symbols from a BitcodeFile \p Buffer and record if we need to
/// extract any symbols from it.
Expected getSymbolsFromBitcode(MemoryBufferRef Buffer, OffloadKind Kind,
- StringSaver &Saver,
+ bool IsArchive, StringSaver &Saver,
DenseMap &Syms) {
Expected IRSymtabOrErr = readIRSymtab(Buffer);
if (!IRSymtabOrErr)
return IRSymtabOrErr.takeError();
- bool ShouldExtract = false;
+ bool ShouldExtract = !IsArchive;
+ DenseMap 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 =
@@ -1192,23 +1193,31 @@
// Update this symbol in the "table" with the new information.
if (OldSym & Sym_Undefined && !Sym.isUndefined())
-OldSym = static_cast(OldSym & ~Sym_Undefined);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(OldSym & ~Sym_Undefined);
if (Sym.isUndefined() && NewSymbol)
-OldSym = static_cast(OldSym | Sym_Undefined);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(OldSym | Sym_Undefined);
if (Sym.isWeak())
-OldSym = static_cast(OldSym | Sym_Weak);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(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 getSymbolsFromObject(const ObjectFile &Obj, OffloadKind Kind,
-StringSaver &Saver,
+bool IsArchive, StringSaver &Saver,
DenseMap &Syms) {
- bool ShouldExtract = false;
+ bool ShouldExtract = !IsArchive;
+ DenseMap TmpSyms;
for (SymbolRef Sym : Obj.symbols()) {
auto FlagsOrErr = Sym.getFlags();
if (!FlagsOrErr)
@@ -1223,7 +1232,7 @@
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) &&
@@ -1240,12 +1249,19 @@
// Update this symbol in the "table" with the new information.
if (OldSym & Sym_Undefined && !(*FlagsOrErr & SymbolRef::SF_Undefined))
- OldSym = static_cast(OldSym & ~Sym_Undefined);
+ TmpSyms[Saver.save(*NameOrErr)] =
+ static_cast(OldSym & ~Sym_Undefined);
if (*FlagsOrErr & SymbolRef::SF_Undefined && NewSymbol)
- OldSym = static_cast(OldSym | Sym_Undefined);
+ TmpSyms[Saver.save(*NameOrErr)] =
+ static_cast(OldSym | Sym_Undefined);
if (*FlagsOrErr & SymbolRef::SF_Weak)
- OldSym = static_cast(OldSym | Sym_Weak);
+ TmpSyms[Saver.save(*NameOrErr)] = static_cast(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;
}
@@ -1255,18 +1271,19 @@
/// 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 getSymbols(StringRef Image, OffloadKind Kind, StringSaver &Saver,
+Expected getSymbols(StringRef Image, OffloadKind Kind, bool IsArchive,
+ StringSaver &Saver,
DenseMap &Syms) {
MemoryBufferRef Buffer = MemoryBufferRef(Image, "");
switch (identify_magic(Image)) {
case file_magic:
[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
jhuber6 added inline comments. Comment at: clang/test/Driver/linker-wrapper-libs.c:27 // // Check that we extract a static library defining an undefined symbol. // tra wrote: > jhuber6 wrote: > > tra wrote: > > > 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". > > 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. > 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. > Good catch, forgot about that. It's why the other use of `extern sym` returns it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151839/new/ https://reviews.llvm.org/D151839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
jhuber6 updated this revision to Diff 527240.
jhuber6 added a comment.
Fix test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151839/new/
https://reviews.llvm.org/D151839
Files:
clang/test/Driver/linker-wrapper-libs.c
clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -1163,20 +1163,21 @@
/// Scan the symbols from a BitcodeFile \p Buffer and record if we need to
/// extract any symbols from it.
Expected getSymbolsFromBitcode(MemoryBufferRef Buffer, OffloadKind Kind,
- StringSaver &Saver,
+ bool IsArchive, StringSaver &Saver,
DenseMap &Syms) {
Expected IRSymtabOrErr = readIRSymtab(Buffer);
if (!IRSymtabOrErr)
return IRSymtabOrErr.takeError();
- bool ShouldExtract = false;
+ bool ShouldExtract = !IsArchive;
+ DenseMap 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 =
@@ -1192,23 +1193,31 @@
// Update this symbol in the "table" with the new information.
if (OldSym & Sym_Undefined && !Sym.isUndefined())
-OldSym = static_cast(OldSym & ~Sym_Undefined);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(OldSym & ~Sym_Undefined);
if (Sym.isUndefined() && NewSymbol)
-OldSym = static_cast(OldSym | Sym_Undefined);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(OldSym | Sym_Undefined);
if (Sym.isWeak())
-OldSym = static_cast(OldSym | Sym_Weak);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(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 getSymbolsFromObject(const ObjectFile &Obj, OffloadKind Kind,
-StringSaver &Saver,
+bool IsArchive, StringSaver &Saver,
DenseMap &Syms) {
- bool ShouldExtract = false;
+ bool ShouldExtract = !IsArchive;
+ DenseMap TmpSyms;
for (SymbolRef Sym : Obj.symbols()) {
auto FlagsOrErr = Sym.getFlags();
if (!FlagsOrErr)
@@ -1223,7 +1232,7 @@
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) &&
@@ -1240,12 +1249,19 @@
// Update this symbol in the "table" with the new information.
if (OldSym & Sym_Undefined && !(*FlagsOrErr & SymbolRef::SF_Undefined))
- OldSym = static_cast(OldSym & ~Sym_Undefined);
+ TmpSyms[Saver.save(*NameOrErr)] =
+ static_cast(OldSym & ~Sym_Undefined);
if (*FlagsOrErr & SymbolRef::SF_Undefined && NewSymbol)
- OldSym = static_cast(OldSym | Sym_Undefined);
+ TmpSyms[Saver.save(*NameOrErr)] =
+ static_cast(OldSym | Sym_Undefined);
if (*FlagsOrErr & SymbolRef::SF_Weak)
- OldSym = static_cast(OldSym | Sym_Weak);
+ TmpSyms[Saver.save(*NameOrErr)] = static_cast(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;
}
@@ -1255,18 +1271,19 @@
/// 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 getSymbols(StringRef Image, OffloadKind Kind, StringSaver &Saver,
+Expected getSymbols(StringRef Image, OffloadKind Kind, bool IsArchive,
+ StringSaver &Saver,
DenseMap &Syms) {
MemoryBufferRef Buffer = MemoryBufferRef(Image, "");
switch (identify_magic(Image)) {
case file_magic::bitcode:
-return getSymbolsFromBitcode(Buffer, Kind, Saver, Syms);
+return getSymbolsFromBitc
[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
tra added inline comments. Comment at: clang/test/Driver/linker-wrapper-libs.c:27 // // Check that we extract a static library defining an undefined symbol. // jhuber6 wrote: > tra wrote: > > 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". > 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. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151839/new/ https://reviews.llvm.org/D151839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
jhuber6 added inline comments. Comment at: clang/test/Driver/linker-wrapper-libs.c:27 // // Check that we extract a static library defining an undefined symbol. // tra wrote: > 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". 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151839/new/ https://reviews.llvm.org/D151839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
tra added a comment. LGTM in general. Comment at: clang/test/Driver/linker-wrapper-libs.c:27 // // Check that we extract a static library defining an undefined symbol. // 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". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151839/new/ https://reviews.llvm.org/D151839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D151839: [LinkerWrapper] Fix static library symbol resolution
jhuber6 created this revision.
jhuber6 added reviewers: jdoerfert, tianshilei1992, JonChesterfield, tra,
yaxunl.
Herald added a project: All.
jhuber6 requested review of this revision.
Herald added subscribers: cfe-commits, jplehr, sstefan1.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D151839
Files:
clang/test/Driver/linker-wrapper-libs.c
clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
Index: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
===
--- clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
+++ clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp
@@ -1163,20 +1163,21 @@
/// Scan the symbols from a BitcodeFile \p Buffer and record if we need to
/// extract any symbols from it.
Expected getSymbolsFromBitcode(MemoryBufferRef Buffer, OffloadKind Kind,
- StringSaver &Saver,
+ bool IsArchive, StringSaver &Saver,
DenseMap &Syms) {
Expected IRSymtabOrErr = readIRSymtab(Buffer);
if (!IRSymtabOrErr)
return IRSymtabOrErr.takeError();
- bool ShouldExtract = false;
+ bool ShouldExtract = !IsArchive;
+ DenseMap 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 =
@@ -1192,23 +1193,31 @@
// Update this symbol in the "table" with the new information.
if (OldSym & Sym_Undefined && !Sym.isUndefined())
-OldSym = static_cast(OldSym & ~Sym_Undefined);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(OldSym & ~Sym_Undefined);
if (Sym.isUndefined() && NewSymbol)
-OldSym = static_cast(OldSym | Sym_Undefined);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(OldSym | Sym_Undefined);
if (Sym.isWeak())
-OldSym = static_cast(OldSym | Sym_Weak);
+TmpSyms[Saver.save(Sym.getName())] =
+static_cast(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 getSymbolsFromObject(const ObjectFile &Obj, OffloadKind Kind,
-StringSaver &Saver,
+bool IsArchive, StringSaver &Saver,
DenseMap &Syms) {
- bool ShouldExtract = false;
+ bool ShouldExtract = !IsArchive;
+ DenseMap TmpSyms;
for (SymbolRef Sym : Obj.symbols()) {
auto FlagsOrErr = Sym.getFlags();
if (!FlagsOrErr)
@@ -1223,7 +1232,7 @@
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) &&
@@ -1240,12 +1249,19 @@
// Update this symbol in the "table" with the new information.
if (OldSym & Sym_Undefined && !(*FlagsOrErr & SymbolRef::SF_Undefined))
- OldSym = static_cast(OldSym & ~Sym_Undefined);
+ TmpSyms[Saver.save(*NameOrErr)] =
+ static_cast(OldSym & ~Sym_Undefined);
if (*FlagsOrErr & SymbolRef::SF_Undefined && NewSymbol)
- OldSym = static_cast(OldSym | Sym_Undefined);
+ TmpSyms[Saver.save(*NameOrErr)] =
+ static_cast(OldSym | Sym_Undefined);
if (*FlagsOrErr & SymbolRef::SF_Weak)
- OldSym = static_cast(OldSym | Sym_Weak);
+ TmpSyms[Saver.save(*NameOrErr)] = static_cast(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));
