This is an archive of the discontinued LLVM Phabricator instance.

[LLD] Support for --unresolved-symbols option in llvm lld for ELF file format
Needs ReviewPublic

Authored by joshishr on Aug 20 2015, 10:32 PM.

Details

Summary

Adding support for --unresolved-symbols option for ELF file format in llvm linker lld.
Details of this option can be found in gnu ld(1) man page.

Implemented all four values of this option: report-all, ignore-all, ignore-in-object-files and ignore-in-shared-libs
Implemented behavior of all the values as standalone as well as in combination with options --allow-shlib-undefined, --no-allow-shlib-undefined, -shared, -static

Bug# 24524
https://llvm.org/bugs/show_bug.cgi?id=24524

Patch by Shridhar Joshi

Diff Detail

Event Timeline

joshishr retitled this revision from to [LLD] Support for --unresolved-symbols option in llvm lld for ELF file format.
joshishr updated this object.
joshishr added a project: lld.
joshishr added a reviewer: llvm-commits.
joshishr edited reviewers, added: soumitra, supra; removed: llvm-commits.Aug 21 2015, 2:14 AM
joshishr added subscribers: soumitra, supra.
joshishr edited subscribers, added: llvm-commits; removed: supra, soumitra.
joshishr added a subscriber: lld.
joshishr added a reviewer: llvm-commits.

Added Test case.

ruiu added a subscriber: ruiu.Sep 2 2015, 12:14 AM
ruiu added inline comments.
lib/Driver/GnuLdDriver.cpp
439–442

It's better to define enums for these strings and use them in Resolver.cpp instead of strings.

458

I'm wondering if this is correct. Does --unresolved-symbols really negate --allow-shlib-undefines?

atanasyan added inline comments.
test/elf/unresolved-symbols.test
9

Is it possible to escape using Clang in the test? Usually build-bots do not build Clang with LLD.

It's better to define enums for these strings and use them in Resolver.cpp

instead of strings.
Yes, enum will be good

I'm wondering if this is correct. Does --unresolved-symbols really negate

--allow-shlib-undefines?
Not all values of --unresolved-symbols option negate
--allow-shlib-undefines.
But --allow-shlib-undefines option is only for shared libraries
whereas --unresolved-symbols is for relocatable object files, shared
libraries as well as executable.
I have tested behavior of all values of --unresolved-symbols in
combination with --allow-shlib-undefines and --no-allow-shlib-undefines
with gnu ld and gold.

Implementation of --unresolved-symbols option submitted in lld is in
compliance with gnu ld and gold.

@simon Atanasyan,

Is it possible to escape using Clang in the test? Usually build-bots do

not build Clang with LLD
Sure, I will update test case by avoiding clang usage.

joshishr added inline comments.Sep 2 2015, 2:23 AM
lib/Driver/GnuLdDriver.cpp
439–442

Yes, enum will be good. Will update the patch.

458

Not all values of --unresolved-symbols option negate --allow-shlib-undefines.
But --allow-shlib-undefines option is only for shared libraries whereas --unresolved-symbols is for relocatable object files, shared libraries as well as executable.
I have tested behavior of all values of --unresolved-symbols in combination with --allow-shlib-undefines and --no-allow-shlib-undefines with gnu ld and gold.

Implementation of --unresolved-symbols option submitted in lld is in compliance with gnu ld and gold.

test/elf/unresolved-symbols.test
9

Sure, I will update test case by avoiding clang usage.

joerg added a subscriber: joerg.Sep 2 2015, 2:24 AM

I'm missing test cases for undefined symbols in shared libraries linked against. That's the area where old lld was seriously misbehaving. In short: we should not complain about undefined symbols in shared libraries used as dependencies.

I'm missing test cases for undefined symbols in shared libraries linked against. That's the area where old lld was seriously misbehaving. In short: we should not complain about undefined symbols in shared libraries used as dependencies.

Test case i have added also tests the behavior of undefined symbols in shared libraries linked against. Default behavior is not complain about undefined symbols in shared libraries used as dependencies. --unresolved-symbols option is not changing this default behavior.

I might not have completely understood your query. Please elaborate more, if this does not answer your query.

joerg added a comment.Sep 2 2015, 4:24 AM

The undefined-shlib case makes no sense. That's exactly the case I am talking about.

Incorporated Review comments:

  1. Use enums instead of strings and use it in Resolver.cpp
  2. Avoid using clang in lld test case

The undefined-shlib case makes no sense. That's exactly the case I am talking about.

--[no]-allow-shlib-undefined option is shared library specific. --unresolved-symbols=ignore-in-object-files option may look equivalent to --allow-shlib-undefined but it is not exactly equivalent since it will report undefined symbols in object files while allowing undefined symbols from dependent shared libraries linked against. So --[no]-allow-shlib-undefined and --unresolved-symbols options serves different unique purpose. Comparatively later option has lot more use case as it allows unresolved symbols in executable as well. This adds use case to defer symbol resolution till run time by pre-loading shared libraries or through dlopen().

Some of the use cases in my test case using --[no]-allow-shlib-undefined may look making no sense but I have added them just for completeness of testing all combinations.

soumitra added inline comments.Sep 3 2015, 3:56 AM
include/lld/Core/LinkingContext.h
164

Return type should be the enumeration type, and not the size specifier.

214

Parameter type should be the enumeration type and not the size specifier.

358

Member should have the enumeration type, not the size specifier.

lib/Core/LinkingContext.cpp
29

I would recommend using elf::DEFAULT for the initialization.

lib/Core/Resolver.cpp
469

This should ideally be a fall-through case along with the case for 'IGNORE_ALL'.

Updated source code with

  1. Use enum type for set() and get() APIs
  2. Use default enum value for initialization
  3. Common switch cases mixed together
joshishr marked 5 inline comments as done.Sep 3 2015, 6:26 AM