[lld] r259475 - ELF: Include archive names in error messages.
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 2 13:29:59 PST 2016
On Tue, Feb 2, 2016 at 12:17 PM, Rui Ueyama <ruiu at google.com> wrote:
> On Tue, Feb 2, 2016 at 12:13 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>> Thanks!
>>
>> I have some suggestion inline.
>>
>> On Tue, Feb 2, 2016 at 12:22 AM, Rui Ueyama via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: ruiu
>>> Date: Tue Feb 2 02:22:41 2016
>>> New Revision: 259475
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=259475&view=rev
>>> Log:
>>> ELF: Include archive names in error messages.
>>>
>>> If object files are drawn from archive files, the error message should
>>> be something like "conflict symbols in foo.a(bar.o) and baz.o" instead
>>> of "conflict symbols in bar.o and baz.o". This patch implements that.
>>>
>>> Added:
>>> lld/trunk/test/ELF/Inputs/conflict.s
>>> Modified:
>>> lld/trunk/ELF/Driver.cpp
>>> lld/trunk/ELF/InputFiles.cpp
>>> lld/trunk/ELF/InputFiles.h
>>> lld/trunk/ELF/SymbolTable.cpp
>>> lld/trunk/ELF/Symbols.cpp
>>> lld/trunk/test/ELF/conflict.s
>>>
>>> Modified: lld/trunk/ELF/Driver.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=259475&r1=259474&r2=259475&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/Driver.cpp (original)
>>> +++ lld/trunk/ELF/Driver.cpp Tue Feb 2 02:22:41 2016
>>> @@ -95,8 +95,9 @@ void LinkerDriver::addFile(StringRef Pat
>>> return;
>>> case file_magic::archive:
>>> if (WholeArchive) {
>>> + StringRef S = MBRef.getBufferIdentifier();
>>> for (MemoryBufferRef MB : getArchiveMembers(MBRef))
>>> - Files.push_back(createObjectFile(MB));
>>> + Files.push_back(createObjectFile(MB, S));
>>> return;
>>> }
>>> Files.push_back(make_unique<ArchiveFile>(MBRef));
>>>
>>> Modified: lld/trunk/ELF/InputFiles.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.cpp?rev=259475&r1=259474&r2=259475&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/InputFiles.cpp (original)
>>> +++ lld/trunk/ELF/InputFiles.cpp Tue Feb 2 02:22:41 2016
>>> @@ -452,8 +452,11 @@ static std::unique_ptr<InputFile> create
>>> fatal("Invalid file class: " + MB.getBufferIdentifier());
>>> }
>>>
>>> -std::unique_ptr<InputFile> elf2::createObjectFile(MemoryBufferRef MB) {
>>> - return createELFFile<ObjectFile>(MB);
>>> +std::unique_ptr<InputFile> elf2::createObjectFile(MemoryBufferRef MB,
>>> + StringRef
>>> ArchiveName) {
>>> + std::unique_ptr<InputFile> F = createELFFile<ObjectFile>(MB);
>>> + F->ArchiveName = ArchiveName;
>>> + return F;
>>> }
>>>
>>> std::unique_ptr<InputFile> elf2::createSharedFile(MemoryBufferRef MB) {
>>>
>>> Modified: lld/trunk/ELF/InputFiles.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputFiles.h?rev=259475&r1=259474&r2=259475&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/InputFiles.h (original)
>>> +++ lld/trunk/ELF/InputFiles.h Tue Feb 2 02:22:41 2016
>>> @@ -38,6 +38,11 @@ public:
>>>
>>> StringRef getName() const { return MB.getBufferIdentifier(); }
>>>
>>> + // Filename of .a which contained this file. If this file was
>>> + // not in an archive file, it is the empty string. We use this
>>> + // string for creating error messages.
>>> + std::string ArchiveName;
>>> +
>>>
>>
>> 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.
>> 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.
>> Or perhaps simpler would be to do `Archive *ParentArchive = nullptr;`
>> instead of storing ArchiveName. Then you can do
>> ParentArchive->getFileName().
>>
>
> Yeah, I think this can be StringRef.
>
> 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.
>
Makes sense. Thanks for the explanation!
-- Sean Silva
>
> protected:
>>> InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}
>>> MemoryBufferRef MB;
>>> @@ -208,7 +213,8 @@ public:
>>> bool isNeeded() const { return !AsNeeded || IsUsed; }
>>> };
>>>
>>> -std::unique_ptr<InputFile> createObjectFile(MemoryBufferRef MB);
>>> +std::unique_ptr<InputFile> createObjectFile(MemoryBufferRef MB,
>>> + StringRef ArchiveName = "");
>>> std::unique_ptr<InputFile> createSharedFile(MemoryBufferRef MB);
>>>
>>> } // namespace elf2
>>>
>>> Modified: lld/trunk/ELF/SymbolTable.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SymbolTable.cpp?rev=259475&r1=259474&r2=259475&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/SymbolTable.cpp (original)
>>> +++ lld/trunk/ELF/SymbolTable.cpp Tue Feb 2 02:22:41 2016
>>> @@ -149,17 +149,23 @@ ELFFileBase<ELFT> *SymbolTable<ELFT>::fi
>>> return nullptr;
>>> }
>>>
>>> +// Returns "(internal)", "foo.a(bar.o)" or "baz.o".
>>> +template <class ELFT> static std::string getFilename(ELFFileBase<ELFT>
>>> *F) {
>>> + if (!F)
>>> + return "(internal)";
>>> + if (!F->ArchiveName.empty())
>>> + return (F->ArchiveName + "(" + F->getName() + ")").str();
>>> + return F->getName();
>>> +}
>>> +
>>> // Construct a string in the form of "Sym in File1 and File2".
>>> // Used to construct an error message.
>>> template <class ELFT>
>>> std::string SymbolTable<ELFT>::conflictMsg(SymbolBody *Old, SymbolBody
>>> *New) {
>>> - ELFFileBase<ELFT> *OldFile = findFile(Old);
>>> - ELFFileBase<ELFT> *NewFile = findFile(New);
>>> -
>>> + ELFFileBase<ELFT> *F1 = findFile(Old);
>>> + ELFFileBase<ELFT> *F2 = findFile(New);
>>> StringRef Sym = Old->getName();
>>> - StringRef F1 = OldFile ? OldFile->getName() : "(internal)";
>>> - StringRef F2 = NewFile ? NewFile->getName() : "(internal)";
>>> - return (demangle(Sym) + " in " + F1 + " and " + F2).str();
>>> + return demangle(Sym) + " in " + getFilename(F1) + " and " +
>>> getFilename(F2);
>>> }
>>>
>>> // This function resolves conflicts if there's an existing symbol with
>>>
>>> Modified: lld/trunk/ELF/Symbols.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Symbols.cpp?rev=259475&r1=259474&r2=259475&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/ELF/Symbols.cpp (original)
>>> +++ lld/trunk/ELF/Symbols.cpp Tue Feb 2 02:22:41 2016
>>> @@ -187,7 +187,7 @@ std::unique_ptr<InputFile> Lazy::getMemb
>>> // read from the library.
>>> if (MBRef.getBuffer().empty())
>>> return std::unique_ptr<InputFile>(nullptr);
>>> - return createObjectFile(MBRef);
>>> + return createObjectFile(MBRef, File->getName());
>>> }
>>>
>>> template <class ELFT> static void doInitSymbols() {
>>>
>>> Added: lld/trunk/test/ELF/Inputs/conflict.s
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/Inputs/conflict.s?rev=259475&view=auto
>>>
>>> ==============================================================================
>>> --- lld/trunk/test/ELF/Inputs/conflict.s (added)
>>> +++ lld/trunk/test/ELF/Inputs/conflict.s Tue Feb 2 02:22:41 2016
>>> @@ -0,0 +1,7 @@
>>> +.globl _Z3muldd, foo, baz
>>> +_Z3muldd:
>>> +foo:
>>> +baz:
>>> + mov $60, %rax
>>> + mov $42, %rdi
>>> + syscall
>>>
>>> Modified: lld/trunk/test/ELF/conflict.s
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/conflict.s?rev=259475&r1=259474&r2=259475&view=diff
>>>
>>> ==============================================================================
>>> --- lld/trunk/test/ELF/conflict.s (original)
>>> +++ lld/trunk/test/ELF/conflict.s Tue Feb 2 02:22:41 2016
>>> @@ -3,15 +3,21 @@
>>> # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
>>> # RUN: not ld.lld %t %t -o %t2 2>&1 | FileCheck -check-prefix=DEMANGLE
>>> %s
>>>
>>> -# RUN: not ld.lld %t %t -o %t2 --no-demangle 2>&1 | \
>>> -# RUN: FileCheck -check-prefix=NO_DEMANGLE %s
>>> -
>>> # DEMANGLE: duplicate symbol: {{mul\(double, double\)|_Z3muldd}} in
>>> # DEMANGLE: duplicate symbol: foo in
>>>
>>> +# RUN: not ld.lld %t %t -o %t2 --no-demangle 2>&1 | \
>>> +# RUN: FileCheck -check-prefix=NO_DEMANGLE %s
>>> +
>>> # NO_DEMANGLE: duplicate symbol: _Z3muldd in
>>> # NO_DEMANGLE: duplicate symbol: foo in
>>>
>>> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux
>>> %S/Inputs/conflict.s -o %t2
>>> +# RUN: llvm-ar rcs %t.a %t2
>>> +# RUN: not ld.lld %t %t.a -u baz -o %t2 2>&1 | FileCheck
>>> -check-prefix=ARCHIVE %s
>>> +
>>> +# ARCHIVE: duplicate symbol: foo in {{.*}}conflict.s.tmp and
>>> {{.*}}.a(conflict.s.tmp2)
>>>
>>
>> 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) ?
>>
>> -- Sean Silva
>>
>>
>>> +
>>> .globl _Z3muldd, foo
>>> _Z3muldd:
>>> foo:
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160202/409c8802/attachment.html>
More information about the llvm-commits
mailing list