Page MenuHomePhabricator

[LLD] Add support for --unique option
ClosedPublic

Authored by davidb on Mar 3 2020, 8:55 AM.

Details

Summary

Places orphan sections into a unique output section. This prevents the merging of orphan sections of the same name.
Matches behaviour of GNU ld --unique. --unique=pattern is not implemented.

Motivated user case shown in the test has 2 local symbols as they would appear if C++ source has been compiled with -ffunction-sections. The merging of these sections in the case of a partial link (-r) may limit the effectiveness of -gc-sections of a subsequent link.

Diff Detail

Event Timeline

davidb created this revision.Mar 3 2020, 8:55 AM
davidb retitled this revision from Add support for --unique option to [LLD] Add support for --unique option.Mar 3 2020, 8:59 AM
davidb updated this revision to Diff 247932.Mar 3 2020, 9:22 AM

Fix lint checks

Harbormaster completed remote builds in B47926: Diff 247932.
MaskRay added a comment.EditedMar 3 2020, 11:38 AM

Do you have a plan to implement --unique=pattern (overrides output section descriptions)? It is different from --unique (only applies to orphan sections).

The merging of these sections in the case of a partial link (-r) inhibits deadstripping.

Dead stripping is a Mach-O term. Non --unique does not inhibit --gc-sections (without -r).
Without --unique, some orphan sections may be unnecessarily kept (as a whole their "liveness" can be larger than the composing parts).

Note that -r --gc-sections is not implemented in lld.

lld/ELF/LinkerScript.cpp
691

Use else if

lld/ELF/Options.td
378

No period.

Add an item to lld/docs/ld.lld.1 (manpage)

lld/test/ELF/Inputs/unique-orphans.s
1

Delete the file.

lld/test/ELF/unique-orphans.s
2

Delete -unknown-linux

3

Keep one REQUIRES

6

The instructions are verbose and unnecessary.

.section .foo,"a",unique,1
.byte 1

.section .foo,"a",unique,2
.byte 2

.section .foo,"a",unique,3
.byte 3

Check both ld.lld %t.o -o %t and ld.lld -r %t.o -o %t.ro

7

Test an output section description suppresses --unique.

echo 'SECTIONS { .foo : { *(.foo) }}' > %t.script`

See test/ELF/linkerscript/input-archive.s for a recent example.

25

Check sections with llvm-readelf -S, instead of objdump (may not even be installed)

29

Not necessary. There are plenty of tests for the non --unique case.

MaskRay edited reviewers, added: psmith; removed: peter.smith.Mar 3 2020, 11:39 AM
davidb added a comment.EditedMar 3 2020, 12:54 PM

Do you have a plan to implement --unique=pattern (overrides output section descriptions)? It is different from --unique (only applies to orphan sections).

I don't have a plan to implement --unique=pattern, as the default is good enough for my needs. However I would consider getting back and doing it when I next have some free cycles.

The merging of these sections in the case of a partial link (-r) inhibits deadstripping.

Dead stripping is a Mach-O term. Non --unique does not inhibit --gc-sections (without -r).
Without --unique, some orphan sections may be unnecessarily kept (as a whole their "liveness" can be larger than the composing parts).

Note that -r --gc-sections is not implemented in lld.

Sorry, force of habit. PlayStation toolchains also use the term dead-stripping for ELF binaries. I did indeed mean gc-sections.

The current workflow involves multiple link stages, involving a partial link and then an executable link. It is the second link that performs -gc-sections. We would like to keep all functions in their own individual sections after the partial link.

I could update the test to demonstrate this if it is at all useful.

Do you have a plan to implement --unique=pattern (overrides output section descriptions)? It is different from --unique (only applies to orphan sections).

I don't have a plan to implement --unique=pattern, as the default is good enough for my needs. However I would consider getting back and doing it when I next have some free cycles.

The merging of these sections in the case of a partial link (-r) inhibits deadstripping.

Dead stripping is a Mach-O term. Non --unique does not inhibit --gc-sections (without -r).
Without --unique, some orphan sections may be unnecessarily kept (as a whole their "liveness" can be larger than the composing parts).

Note that -r --gc-sections is not implemented in lld.

Sorry, force of habit. PlayStation toolchains also use the term dead-stripping for ELF binaries. I did indeed mean gc-sections.

The current workflow involves multiple link stages, involving a partial link and then an executable link. It is the second link that performs -gc-sections. We would like to keep all functions in their own individual sections after the partial link.

I could update the test to demonstrate this if it is at all useful.

A test is not needed. Just make the test minimal (for example, as I suggested).

Please update SUMMARY by mentioning the --gc-sections scenario and delete the mention of deadstripping.

davidb updated this revision to Diff 248045.Mar 3 2020, 2:42 PM
  • review comments
davidb marked 6 inline comments as done.Mar 3 2020, 2:45 PM
davidb edited the summary of this revision. (Show Details)Mar 3 2020, 2:48 PM

Do you have a plan to implement --unique=pattern (overrides output section descriptions)? It is different from --unique (only applies to orphan sections).

I don't have a plan to implement --unique=pattern, as the default is good enough for my needs. However I would consider getting back and doing it when I next have some free cycles.

The merging of these sections in the case of a partial link (-r) inhibits deadstripping.

Dead stripping is a Mach-O term. Non --unique does not inhibit --gc-sections (without -r).
Without --unique, some orphan sections may be unnecessarily kept (as a whole their "liveness" can be larger than the composing parts).

Note that -r --gc-sections is not implemented in lld.

Sorry, force of habit. PlayStation toolchains also use the term dead-stripping for ELF binaries. I did indeed mean gc-sections.

The current workflow involves multiple link stages, involving a partial link and then an executable link. It is the second link that performs -gc-sections. We would like to keep all functions in their own individual sections after the partial link.

I could update the test to demonstrate this if it is at all useful.

A test is not needed. Just make the test minimal (for example, as I suggested).

Please update SUMMARY by mentioning the --gc-sections scenario and delete the mention of deadstripping.

Thank you for the review comments, especially wrt the tests. I have updated the summary, just need to add a comment to the manpage, which I'll try to get round to in the morning.

jhenderson added inline comments.Mar 4 2020, 12:50 AM
lld/ELF/Options.td
377

Nit: too many blank lines here.

lld/test/ELF/unique-orphans.s
14

instance -> instances (here and below)

34

Nit: no newline at EOF.

grimar added inline comments.Mar 4 2020, 1:04 AM
lld/test/ELF/unique-orphans.s
14

Consider using ## for comments. It is a modern common way.

18

Seems you can have a single chack block:

# CHECK-3:   .foo
# CHECK-NOT: .foo

And remove --check-prefix from everywhere?

20

.text.foo -> .foo? (and above)

MaskRay added inline comments.Mar 4 2020, 8:28 AM
lld/test/ELF/unique-orphans.s
25

Not done. Use llvm-readelf -S

26

Add a comment, along the lines of

## Test that --unique does not affect sections specified in output section descriptions.

davidb marked an inline comment as done.Mar 5 2020, 4:30 AM
davidb added inline comments.
lld/test/ELF/unique-orphans.s
25

I added the llvm-prefix so it should pick the right objdump. There are 2x more uses of llvm-objdump than readelf in the lld test suite, 4x more in LLVM as a whole. What is the motivation for readelf?

davidb updated this revision to Diff 248441.Mar 5 2020, 4:43 AM
  • review comments
davidb updated this revision to Diff 248443.Mar 5 2020, 4:45 AM
  • fix formatting from clang-format
davidb marked 5 inline comments as done.Mar 5 2020, 4:51 AM
davidb updated this revision to Diff 248452.Mar 5 2020, 5:27 AM
  • review comments
davidb marked an inline comment as done.Mar 5 2020, 5:27 AM
davidb updated this revision to Diff 248456.Mar 5 2020, 5:41 AM
  • replace objdump with readelf
davidb marked an inline comment as done.Mar 5 2020, 5:41 AM
davidb updated this revision to Diff 248489.Mar 5 2020, 8:44 AM
  • rebase
davidb marked 3 inline comments as done.Mar 5 2020, 8:45 AM
MaskRay added inline comments.Mar 5 2020, 8:45 AM
lld/ELF/Config.h
197

Delete = false

lld/ELF/Driver.cpp
970

lexicographical order

lld/test/ELF/unique-orphans.s
15

The comment ## Test with -r is redundant and should be deleted.

davidb updated this revision to Diff 248525.Mar 5 2020, 9:51 AM
  • review comments
davidb marked 3 inline comments as done.Mar 5 2020, 9:55 AM
davidb added inline comments.
lld/ELF/Config.h
197

Why do you not want me to be explicit here? Many other options above are

lld/ELF/Driver.cpp
970

was result of rebase

davidb marked 2 inline comments as done.Mar 5 2020, 9:58 AM
davidb added inline comments.
lld/test/ELF/unique-orphans.s
15

shame.

Places orphan sections into a unique output section. This prevents the merging of orphan sections of the same name. Matches behaviour of gnu LD option --unique[=SECTION], with the exception I haven't added the wildcard SECTION support.

gnu LD -> GNU ld

Matches behaviour of GNU ld --unique. --unique=pattern is not implemented.

lld/test/ELF/unique-orphans.s
25

Nit: --check-prefix=SCRIPT

UNIQUE_ is not necessary in the context.

grimar added inline comments.Mar 5 2020, 11:49 PM
lld/ELF/Config.h
197

There is a difference. Look at sysvHash for example whitch is explicitly set to false.
sysvHash is not always set to something in the code. Hence we have to initialize it.

You always assing unique: config->unique = args.hasArg(OPT_unique);. So no need to initialize it explicitly.

davidb updated this revision to Diff 249094.Mar 9 2020, 7:29 AM
  • review comments
davidb updated this revision to Diff 249095.Mar 9 2020, 7:30 AM
  • rebase
davidb marked 4 inline comments as done.Mar 9 2020, 7:31 AM
davidb edited the summary of this revision. (Show Details)Mar 9 2020, 7:32 AM
davidb updated this revision to Diff 249119.Mar 9 2020, 8:43 AM
  • formatting
MaskRay accepted this revision.Mar 9 2020, 9:46 AM

LGTM. Please wait for @grimar's opinions.

This revision is now accepted and ready to land.Mar 9 2020, 9:46 AM
grimar accepted this revision.Mar 10 2020, 3:21 AM

LGTM

This revision was automatically updated to reflect the committed changes.