Page MenuHomePhabricator

If --dynamic-list is given, only those symbols are preemptible
ClosedPublic

Authored by rafael on Aug 8 2017, 6:11 PM.

Details

Summary

This is PR34053.

The implementation is a bit of a hack, given the precise location where IsPreemtible is set, it cannot be used from SymbolTable::handleAnonymousVersion.

I could add another method to SymbolTable if you think that would be better.

Diff Detail

Repository
rL LLVM

Event Timeline

rafael created this revision.Aug 8 2017, 6:11 PM
smeenai edited edge metadata.Aug 8 2017, 10:42 PM

Thank you!

One issue I have is that this still conflates version scripts and dynamic lists, whereas they're orthogonal concepts in my mind; a version script controls dynamic symbol table population, while a dynamic list (for a shared library) controls preemptibility. The test case below demonstrates this; it passes with ld.bfd and ld.gold.

# REQUIRES: x86

# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
# RUN: echo "{ foo; };" > %t.list
# RUN: echo "{ global: foo; bar; };" > %t.vers
# RUN: ld.lld -fatal-warnings -dynamic-list %t.list -version-script %t.vers -shared %t.o -o %t.so
# RUN: llvm-readobj -r %t.so | FileCheck --check-prefix=RELOCS %s
# RUN: llvm-nm -DU %t.so | FileCheck --check-prefix=DYNSYMS %s

# RELOCS:      Relocations [
# RELOCS-NEXT:   Section ({{.*}}) .rela.plt {
# RELOCS-NEXT:     R_X86_64_JUMP_SLOT foo 0x0
# RELOCS-NEXT:   }
# RELOCS-NEXT: ]

# DYNSYMS:      bar
# DYNSYMS-NEXT: foo

	.globl	foo
foo:
	ret

	.globl	bar
bar:
	ret

	call	foo@PLT
	call	bar@PLT
grimar added a subscriber: grimar.Aug 8 2017, 11:36 PM
grimar added inline comments.
ELF/Writer.cpp
1188 ↗(On Diff #110318)

foo ? :) Looks there is no test for this error.

ruiu added inline comments.Aug 9 2017, 5:13 AM
ELF/Config.h
87 ↗(On Diff #110318)

These members are sorted by type and then by name, so move below.

ELF/Driver.cpp
748 ↗(On Diff #110318)

Maybe you want to set this only once by Config->HasDynamicList = Args.hasArg(OPT_dynamic_list)?

ELF/Writer.cpp
1185 ↗(On Diff #110318)

This needs a comment.

Fix iteration with version scripts.

It is now possible to use version scripts to decide which symbols are globals and --dynamic-list to pick which of those are also preemptible.

smeenai resigned from this revision.Aug 10 2017, 12:45 PM

This looks great. Thank you!

I'll leave the approval to Rui.

I am going on vacation. Please commit if OK.

ruiu accepted this revision.Aug 22 2017, 9:25 AM

LGTM

Since Rafael is on vacation, I'll submit this patch with the following fixes.

ELF/ScriptParser.cpp
182–183 ↗(On Diff #110623)

You want to return here.

185 ↗(On Diff #110623)

+ next() doesn't seem to make sense, so remove.

This revision is now accepted and ready to land.Aug 22 2017, 9:25 AM
This revision was automatically updated to reflect the committed changes.
smeenai reopened this revision.Sep 5 2017, 8:33 PM

This was committed in r311468 and reverted in r311497 because it broke bots. I'm re-opening now that @rafael is back from vacation.

This revision is now accepted and ready to land.Sep 5 2017, 8:33 PM
espindola closed this revision.Mar 14 2018, 3:46 PM
espindola added a subscriber: espindola.

312806