<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 2, 2016 at 12:13 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks!<div><br></div><div>I have some suggestion inline.<br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Tue, Feb 2, 2016 at 12:22 AM, Rui Ueyama via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ruiu<br>
Date: Tue Feb 2 02:22:41 2016<br>
New Revision: 259475<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=259475&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=259475&view=rev</a><br>
Log:<br>
ELF: Include archive names in error messages.<br>
<br>
If object files are drawn from archive files, the error message should<br>
be something like "conflict symbols in foo.a(bar.o) and baz.o" instead<br>
of "conflict symbols in bar.o and baz.o". This patch implements that.<br>
<br>
Added:<br>
lld/trunk/test/ELF/Inputs/conflict.s<br>
Modified:<br>
lld/trunk/ELF/Driver.cpp<br>
lld/trunk/ELF/InputFiles.cpp<br>
lld/trunk/ELF/InputFiles.h<br>
lld/trunk/ELF/SymbolTable.cpp<br>
lld/trunk/ELF/Symbols.cpp<br>
lld/trunk/test/ELF/conflict.s<br>
<br>
Modified: lld/trunk/ELF/Driver.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=259475&r1=259474&r2=259475&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=259475&r1=259474&r2=259475&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/Driver.cpp (original)<br>
+++ lld/trunk/ELF/Driver.cpp Tue Feb 2 02:22:41 2016<br>
@@ -95,8 +95,9 @@ void LinkerDriver::addFile(StringRef Pat<br>
return;<br>
case file_magic::archive:<br>
if (WholeArchive) {<br>
+ StringRef S = MBRef.getBufferIdentifier();<br>
for (MemoryBufferRef MB : getArchiveMembers(MBRef))<br>
- Files.push_back(createObjectFile(MB));<br>
+ Files.push_back(createObjectFile(MB, S));<br>
return;<br>
}<br>
Files.push_back(make_unique<ArchiveFile>(MBRef));<br>
<br>
Modified: lld/trunk/ELF/InputFiles.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.cpp?rev=259475&r1=259474&r2=259475&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.cpp?rev=259475&r1=259474&r2=259475&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/InputFiles.cpp (original)<br>
+++ lld/trunk/ELF/InputFiles.cpp Tue Feb 2 02:22:41 2016<br>
@@ -452,8 +452,11 @@ static std::unique_ptr<InputFile> create<br>
fatal("Invalid file class: " + MB.getBufferIdentifier());<br>
}<br>
<br>
-std::unique_ptr<InputFile> elf2::createObjectFile(MemoryBufferRef MB) {<br>
- return createELFFile<ObjectFile>(MB);<br>
+std::unique_ptr<InputFile> elf2::createObjectFile(MemoryBufferRef MB,<br>
+ StringRef ArchiveName) {<br>
+ std::unique_ptr<InputFile> F = createELFFile<ObjectFile>(MB);<br>
+ F->ArchiveName = ArchiveName;<br>
+ return F;<br>
}<br>
<br>
std::unique_ptr<InputFile> elf2::createSharedFile(MemoryBufferRef MB) {<br>
<br>
Modified: lld/trunk/ELF/InputFiles.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.h?rev=259475&r1=259474&r2=259475&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.h?rev=259475&r1=259474&r2=259475&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/InputFiles.h (original)<br>
+++ lld/trunk/ELF/InputFiles.h Tue Feb 2 02:22:41 2016<br>
@@ -38,6 +38,11 @@ public:<br>
<br>
StringRef getName() const { return MB.getBufferIdentifier(); }<br>
<br>
+ // Filename of .a which contained this file. If this file was<br>
+ // not in an archive file, it is the empty string. We use this<br>
+ // string for creating error messages.<br>
+ std::string ArchiveName;<br>
+<br></blockquote><div><br></div></div></div><div>Can this be StringRef? The parent archive's MemoryBufferRef should already have a StringRef to that same string, so I don't think there are any lifetime issues.</div><div>That will avoid O(#files) std::string heap allocations and (at least for me) clarify the code by avoiding to need to think about why the lifetime had to be preserved with std::string.</div><div>Or perhaps simpler would be to do `Archive *ParentArchive = nullptr;` instead of storing ArchiveName. Then you can do ParentArchive->getFileName().</div></div></div></div></div></blockquote><div><br></div><div>Yeah, I think this can be StringRef.</div><div><br></div><div>This however cannot be Archive* because for archives in --whole-archive --no-whole-archive, we do not create Archive objects. We just extract all files from archives and pass all of them to the symbol table, so there's no need to instantiate Archive for such archives.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
protected:<br>
InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}<br>
MemoryBufferRef MB;<br>
@@ -208,7 +213,8 @@ public:<br>
bool isNeeded() const { return !AsNeeded || IsUsed; }<br>
};<br>
<br>
-std::unique_ptr<InputFile> createObjectFile(MemoryBufferRef MB);<br>
+std::unique_ptr<InputFile> createObjectFile(MemoryBufferRef MB,<br>
+ StringRef ArchiveName = "");<br>
std::unique_ptr<InputFile> createSharedFile(MemoryBufferRef MB);<br>
<br>
} // namespace elf2<br>
<br>
Modified: lld/trunk/ELF/SymbolTable.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SymbolTable.cpp?rev=259475&r1=259474&r2=259475&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SymbolTable.cpp?rev=259475&r1=259474&r2=259475&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/SymbolTable.cpp (original)<br>
+++ lld/trunk/ELF/SymbolTable.cpp Tue Feb 2 02:22:41 2016<br>
@@ -149,17 +149,23 @@ ELFFileBase<ELFT> *SymbolTable<ELFT>::fi<br>
return nullptr;<br>
}<br>
<br>
+// Returns "(internal)", "foo.a(bar.o)" or "baz.o".<br>
+template <class ELFT> static std::string getFilename(ELFFileBase<ELFT> *F) {<br>
+ if (!F)<br>
+ return "(internal)";<br>
+ if (!F->ArchiveName.empty())<br>
+ return (F->ArchiveName + "(" + F->getName() + ")").str();<br>
+ return F->getName();<br>
+}<br>
+<br>
// Construct a string in the form of "Sym in File1 and File2".<br>
// Used to construct an error message.<br>
template <class ELFT><br>
std::string SymbolTable<ELFT>::conflictMsg(SymbolBody *Old, SymbolBody *New) {<br>
- ELFFileBase<ELFT> *OldFile = findFile(Old);<br>
- ELFFileBase<ELFT> *NewFile = findFile(New);<br>
-<br>
+ ELFFileBase<ELFT> *F1 = findFile(Old);<br>
+ ELFFileBase<ELFT> *F2 = findFile(New);<br>
StringRef Sym = Old->getName();<br>
- StringRef F1 = OldFile ? OldFile->getName() : "(internal)";<br>
- StringRef F2 = NewFile ? NewFile->getName() : "(internal)";<br>
- return (demangle(Sym) + " in " + F1 + " and " + F2).str();<br>
+ return demangle(Sym) + " in " + getFilename(F1) + " and " + getFilename(F2);<br>
}<br>
<br>
// This function resolves conflicts if there's an existing symbol with<br>
<br>
Modified: lld/trunk/ELF/Symbols.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Symbols.cpp?rev=259475&r1=259474&r2=259475&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Symbols.cpp?rev=259475&r1=259474&r2=259475&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/ELF/Symbols.cpp (original)<br>
+++ lld/trunk/ELF/Symbols.cpp Tue Feb 2 02:22:41 2016<br>
@@ -187,7 +187,7 @@ std::unique_ptr<InputFile> Lazy::getMemb<br>
// read from the library.<br>
if (MBRef.getBuffer().empty())<br>
return std::unique_ptr<InputFile>(nullptr);<br>
- return createObjectFile(MBRef);<br>
+ return createObjectFile(MBRef, File->getName());<br>
}<br>
<br>
template <class ELFT> static void doInitSymbols() {<br>
<br>
Added: lld/trunk/test/ELF/Inputs/conflict.s<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/Inputs/conflict.s?rev=259475&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/Inputs/conflict.s?rev=259475&view=auto</a><br>
==============================================================================<br>
--- lld/trunk/test/ELF/Inputs/conflict.s (added)<br>
+++ lld/trunk/test/ELF/Inputs/conflict.s Tue Feb 2 02:22:41 2016<br>
@@ -0,0 +1,7 @@<br>
+.globl _Z3muldd, foo, baz<br>
+_Z3muldd:<br>
+foo:<br>
+baz:<br>
+ mov $60, %rax<br>
+ mov $42, %rdi<br>
+ syscall<br>
<br>
Modified: lld/trunk/test/ELF/conflict.s<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/conflict.s?rev=259475&r1=259474&r2=259475&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/conflict.s?rev=259475&r1=259474&r2=259475&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/test/ELF/conflict.s (original)<br>
+++ lld/trunk/test/ELF/conflict.s Tue Feb 2 02:22:41 2016<br>
@@ -3,15 +3,21 @@<br>
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t<br>
# RUN: not ld.lld %t %t -o %t2 2>&1 | FileCheck -check-prefix=DEMANGLE %s<br>
<br>
-# RUN: not ld.lld %t %t -o %t2 --no-demangle 2>&1 | \<br>
-# RUN: FileCheck -check-prefix=NO_DEMANGLE %s<br>
-<br>
# DEMANGLE: duplicate symbol: {{mul\(double, double\)|_Z3muldd}} in<br>
# DEMANGLE: duplicate symbol: foo in<br>
<br>
+# RUN: not ld.lld %t %t -o %t2 --no-demangle 2>&1 | \<br>
+# RUN: FileCheck -check-prefix=NO_DEMANGLE %s<br>
+<br>
# NO_DEMANGLE: duplicate symbol: _Z3muldd in<br>
# NO_DEMANGLE: duplicate symbol: foo in<br>
<br>
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %S/Inputs/conflict.s -o %t2<br>
+# RUN: llvm-ar rcs %t.a %t2<br>
+# RUN: not ld.lld %t %t.a -u baz -o %t2 2>&1 | FileCheck -check-prefix=ARCHIVE %s<br>
+<br>
+# ARCHIVE: duplicate symbol: foo in {{.*}}conflict.s.tmp and {{.*}}.a(conflict.s.tmp2)<br></blockquote><div><br></div></div></div><div>I think this is hardcoding some dependence on the %t names that lit chooses (e.g. conflict.s.tmp). Could you use %t.o instead of %t2 and then check {{.*}}.a({{[^)]}}.o) ?</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
.globl _Z3muldd, foo<br>
_Z3muldd:<br>
foo:<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></span></div><br></div></div></div>
</blockquote></div><br></div></div>