Page MenuHomePhabricator

[LLD] Do not allocate space for common symbols with -r
ClosedPublic

Authored by kettenis on Jan 21 2017, 3:37 PM.

Details

Reviewers
ruiu
rafael
Summary

Currently ld.lld -r allocates space for common symbols, whereas ld.bfd -r doesn't.
As a result the OpenBSD makefile bits for creating libraries fail as they use ld -X -r to strip local symbols,
which results in duplicate symbol errors because space for the common symbols has been allocated.

The diff also implements the --define-commons option such that allocation of commons can be forced even
if -r is used.

Diff Detail

Event Timeline

kettenis created this revision.Jan 21 2017, 3:37 PM
ruiu edited edge metadata.Jan 21 2017, 3:40 PM

Can you add a test?

lld/ELF/Config.h
102

Since you always set a value to this variable, please remove = false.

lld/ELF/Symbols.cpp
77

Returning an alignment as an address seems odd. What are you trying to do here?

h>>! In D28984#652498, @ruiu wrote:

Can you add a test?

Sure.

The alignment is returned as the VA because that's what's being stored in the st_value field of SHN_COMMON symbols.
Alternatively I could return 0 here, and fix up st_value in SymbolTableSection<ELFT>::writeGlobalSymbols(), but this way
seems a little bit cleaner.

ruiu added a comment.Jan 21 2017, 4:10 PM

I think I prefer returning 0 from getVA, because that function is supposed to return an address. Returning an alignment seems a bit confusing.

kettenis updated this revision to Diff 85302.Jan 22 2017, 3:13 PM

New diff that return zero as the VA but sets st_value to the alignment for SHN_COMMON symbols.
Now also includes a test case.

ruiu added inline comments.Jan 23 2017, 2:01 PM
lld/ELF/Driver.cpp
483

You added Config->Relocatable && !Config->DefineCommon in multiple places in this patch. I think doing that only once is a good idea.

// -r implies -no-define-common unless -define-common is specified.
if (Args.hasArg(OPT_relocatable) && !Args.hasArg(OPT_define_common))
  Config->DefineCommon = false;
lld/ELF/Options.td
48

Please also define "no-define-common" just like other command line options with -no- variants.

lld/ELF/SyntheticSections.cpp
1160–1168

nit: since you added {} to one else, please add {} to this if and else if.

kettenis updated this revision to Diff 85478.Jan 23 2017, 3:52 PM

New diff that addresses the issues raised by Rui.

ruiu requested changes to this revision.Jan 23 2017, 3:58 PM

Can you add a test? Otherwise this looks good.

lld/ELF/Driver.cpp
501–502

Nice use of getArg's default value!

This revision now requires changes to proceed.Jan 23 2017, 3:58 PM
kettenis updated this revision to Diff 85491.Jan 23 2017, 4:57 PM
kettenis edited edge metadata.

Apologies. Forgot to add a test. The test now also tests the --no-define-common option.

ruiu accepted this revision.Jan 23 2017, 4:58 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Jan 23 2017, 4:58 PM

Thanks. I don't have commit access, so I'd appreciate it if somebody could commit this for me.

ruiu closed this revision.Jan 23 2017, 7:52 PM

Submitted as r292878.