This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Added support for --unresolved-symbols option.
ClosedPublic

Authored by grimar on Jun 28 2016, 7:40 AM.

Details

Summary

Option has next description (http://linux.die.net/man/1/ld):

--unresolved-symbols=method
Determine how to handle unresolved symbols. There are four possible values for method
according to documentation:

ignore-all: Do not report any unresolved symbols.
report-all: Report all unresolved symbols. This is the default.
ignore-in-object-files: Report unresolved symbols that are contained in shared libraries, but ignore them if they come from regular object files.
ignore-in-shared-libs: Report unresolved symbols that come from regular object files, but ignore them if they come from shared libraries.

This is PR24524.
Since report-all is default and we do not report about undefined symbols in lld,
report-all does not report about undefines from DSO.
ignore-in-object-files also does not do that. Handling of that option differs from what gnu linkers do.

Option works in next way in lld:
ignore-all: Do not report any unresolved symbols.
report-all: Report all unresolved symbols except symbols from DSOs. This is the default.
ignore-in-object-files: The same as ignore-all.
gnore-in-shared-libs: The same as report-all.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 62091.Jun 28 2016, 7:40 AM
grimar retitled this revision from to [ELF] - Added support for --unresolved-symbols option..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu added inline comments.Jun 28 2016, 11:01 PM
ELF/Config.h
40 ↗(On Diff #62091)

We always ignore undefined symbols in DSOs, so this enum shouldn't have IgnoreInSharedLibs. This also overlaps with NoinhibitExec, so it needs reorganization.

I think you want to define this in Config.

enum { Error, Warn, Ignore };

and

  • set to Error by default or if --unresolved-symbols=IgnoreAll
  • set to Warn if --noinhibit-exec
  • set to Ignore if --unresolved-symbols={IgnoreAll,IgnoreInObjectFiles,IgnoreInSharedLibs}
ELF/Writer.cpp
734 ↗(On Diff #62091)

We don't want to do this -- we don't care whether undefined symbols in DSOs are resolved or not unlike GNU linkers.

grimar added inline comments.Jun 29 2016, 12:10 AM
ELF/Config.h
40 ↗(On Diff #62091)

This enum was for describing possible options for unresolved-symbols initially,
but if you want to remove reportUndefinedFromSharedLibs() that of cource
needs reorganization.

I would also add --no-undefined here then.

So can be:

Error by default or if --no-undefined or --unresolved-symbols={ReportAll, IgnoreInSharedLibs}
Warn if --noinhibit-exec
Ignore if --unresolved-symbols={IgnoreAll,IgnoreInObjectFiles}

ELF/Writer.cpp
734 ↗(On Diff #62091)

Generally yes, but I was thinking about this option as about the only option that can show this information in lld.
That was key feature of this option for me.

Shouldn't we show warning then ? For example when IgnoreInObjectFiles is used explicitly we probably want to inform user that
it does not do what is expected from documentation ? (does not show eerors for symbols from DSO).

ruiu added inline comments.Jun 29 2016, 12:15 AM
ELF/Writer.cpp
734 ↗(On Diff #62091)

We don't care about undefined symbols in DSOs because we think identifying them doesn't make sense. So, I think we don't want to provide an option to detect an "error" that doesn't make sense to us.

grimar updated this revision to Diff 62201.Jun 29 2016, 3:51 AM
  • Addressed review comments.
grimar added inline comments.Jun 29 2016, 3:51 AM
ELF/Config.h
40 ↗(On Diff #62091)

I had to add NoUndef enum value to handle --no-undefined.
When --no-undefined is set we produce error even when -shared and symbol visibility STV_DEFAULT,
otherwise it is ignored. That was original logic worked before this patch.

ruiu added inline comments.Jun 29 2016, 4:12 AM
ELF/Config.h
36 ↗(On Diff #62201)

You want to move this inside Config class so that these enum names don't pollute lld::elf namespace.

113 ↗(On Diff #62201)

I think you can make this enum nameless.

enum { NoUndef, Error, Warn, Ignore } UnresolvedSymbols = Error;
ELF/Driver.cpp
412 ↗(On Diff #62201)

Please make a new function for the new code. I'd define getUnresolvedSymbolOption.

ELF/LTO.cpp
242 ↗(On Diff #62201)

Move Error inside Config and revert this change.

ELF/Writer.cpp
12 ↗(On Diff #62201)

Do you need this?

282–284 ↗(On Diff #62201)

I'm not sure if this is what --no-undefined should do. Is this correct?

grimar marked 4 inline comments as done.Jun 29 2016, 4:42 AM
grimar added inline comments.
ELF/Config.h
36 ↗(On Diff #62201)

Please see below.

113 ↗(On Diff #62201)

I think I need named one to implement

HandlingRule getUnresolvedSymbolOption().

And because of that may be make it enum class and leave outside ?

ELF/Driver.cpp
412 ↗(On Diff #62201)

I don't think I can return the value of anonymous enum. I mean I can return unsigned,
but will not be able to assign it then:

Config->UnresolvedSymbols = getUnresolvedSymbolOption(); //error

as there is no type to cast to.

ELF/Writer.cpp
282–284 ↗(On Diff #62201)

Per manual "--no-undefined Report unresolved symbol references from regular object files. This is done even if the linker is creating a non-symbolic shared library."
So according to that description we should report here if --no-undefined is set. Btw I am not changing the logic here, that is how it worked initially and I think it is correct.

grimar updated this revision to Diff 62206.Jun 29 2016, 4:51 AM
grimar marked 2 inline comments as done.
  • Addressed comments in according to Rui's suggestions and my answers above.
ruiu accepted this revision.Jun 29 2016, 5:26 AM
ruiu edited edge metadata.

LGTM with the following changes.

ELF/Config.h
36 ↗(On Diff #62206)

This name is too generic. I'd rename UnresolvedPolicy.

ELF/Options.td
154 ↗(On Diff #62206)

Remove ..

ELF/Writer.cpp
281–282 ↗(On Diff #62206)
if (Config->Shared && Sym->symbol()->Visitility == STV_DEFAULT &&
    Config->UnresolvedSymbols != HandlingRule::NoUndef)
This revision is now accepted and ready to land.Jun 29 2016, 5:26 AM
grimar updated this object.Jun 29 2016, 5:39 AM
grimar edited edge metadata.
This revision was automatically updated to reflect the committed changes.
grimar marked 3 inline comments as done.