<div dir="ltr">Thanks!<div><br></div><div>I have some suggestion inline.<br><div class="gmail_extra"><br><div class="gmail_quote">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>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><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>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><div><br></div><div>-- Sean Silva</div><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">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></div><br></div></div></div>