This is an archive of the discontinued LLVM Phabricator instance.

[elf2] Add basic archive file support.
AbandonedPublic

Authored by Bigcheese on Sep 1 2015, 4:36 PM.

Details

Reviewers
espindola

Diff Detail

Event Timeline

Bigcheese updated this revision to Diff 33753.Sep 1 2015, 4:36 PM
Bigcheese retitled this revision from to [elf2] Add basic archive file support..
Bigcheese updated this object.
Bigcheese added a reviewer: rafael.
rafael edited edge metadata.Sep 2 2015, 8:20 AM
rafael added a subscriber: rafael.

The patch is not git-clang-format clean

building I get these warnings:

/home/espindola/llvm/llvm/tools/lld/ELF/Driver.cpp:83:11: warning:
enumeration value 'ArchiveKind' not handled in switch [-Wswitch]

switch (FirstObj.kind()) {
        ^

1 warning generated.
[6/8] Building CXX object tools/lld/ELF/CMakeFiles/lldELF2.dir/SymbolTable.cpp.o
/home/espindola/llvm/llvm/tools/lld/ELF/SymbolTable.cpp:46:13:
warning: enumeration value 'ArchiveKind' not handled in switch
[-Wswitch]

switch (File->kind()) {
        ^

/home/espindola/llvm/llvm/tools/lld/ELF/SymbolTable.cpp:64:13:
warning: enumeration value 'ArchiveKind' not handled in switch
[-Wswitch]

switch (File->kind()) {
        ^

2 warnings generated.
[6/8] Building CXX object tools/lld/ELF/CMakeFiles/lldELF2.dir/Writer.cpp.o
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]

switch (Body->kind()) {
        ^

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]

The test seems to do a bit more than what is needed:

  • Why do you need to disassemble?
  • _start is already undefined at the start of the link, you shouldn't

need to introduce a new undefined reference to it just to fetch the
member.

rafael added inline comments.Sep 2 2015, 8:33 AM
ELF/InputFiles.cpp
15

Not used.

Why use a llvm::MallocAllocator? It seems better to not have an
allocator and use a smart pointer or use a BumpPtrAllocator.

Given that the allocator is used for exactly one allocation per file,
wouldn't it be the same to replace

std::vector<Lazy *> LazySymbols;
llvm::MallocAllocator Alloc;

With

std::vector<Lazy> LazySymbols

?

The warnings with the file kind were really an issue with the existing
code. I have fixed that and rebased your patch. I also include the
change to use std::vector<Lazy> LazySymbols;.

The remaining warnings look like new issues:

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]

switch (Body->kind()) {
        ^

/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch]
/home/espindola/llvm/llvm/tools/lld/ELF/Writer.cpp:317:13: warning:
enumeration value 'LazyKind' not handled in switch [-Wswitch

Please fix them and upload a new patch.

Another thing: I see that you moved createFile out of Driver.cpp.

That is not what COFF does and I think I agree with COFF on this one.
The files we will support at the top level are not the ones we support
inside a .a. It is better to error early if we are given a .so or a
linker script inside an archive.

Cheers,
Rafael

I also noticed that we can statically know if getSymbols is available.

Attached.

Bigcheese updated this revision to Diff 33976.Sep 3 2015, 2:48 PM
Bigcheese edited edge metadata.

Address comments.

+ case SymbolBody::LazyKind:
+ assert("Lazy symbols shouldn't get here.");

You want an llvm_unreachable, no? That assert never fails.

+ auto ItOrErr = Sym->getMember();

Non obvious type. Please don't use auto.

Can you extend the test to cover us not adding a member twice? Should
be as simple as adding a second undefined symbol defined by the same
member.

+ llvm::DenseMap<uint64_t, bool> Seen;

Can this be a DenseSet?

+// Returns a buffer pointing to a member file containing a given symbol.
+MemoryBufferRef ArchiveFile::getMember(const Archive::Symbol *Sym) {

The comment is effectively duplicated from the header.

A few more nits:

+ if (Existing->isDefined() || isa<Lazy>(Existing))
+ return;
+ Sym->Body = New;
+ if (Existing->isUndefined())
+ addMemberFile(New);

The second if should be an assert. On ELF we have

bool isDefined() const { return !isUndefined(); }

+ getMember returns an empty buffer if the member was already
+
read from the library.
+ if (!File)
+ return;

Returns nullptr, no?

+static std::unique_ptr<InputFile> createFile(MemoryBufferRef MB) {
+ // FIXME: duplicated

Please fix it :-)
We need a basic function that only handles objects. It is used in here
and Driver.cpp uses it if not given an archive or shared library.

So only nits. LGTM with all of them fixed.

Cheers,
Rafael

espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 11:00 AM
Bigcheese abandoned this revision.Nov 14 2019, 1:10 PM