This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Add support for emitting dylibs with a single symbol
AbandonedPublic

Authored by int3 on Mar 27 2020, 1:04 AM.

Details

Summary

Add logic for emitting the correct set of load commands and segments when -dylib is passed.

I haven't gotten to implementing a real export trie yet, so we can only emit a single symbol, but it's enough to replace the YAML test files introduced in D76252.

Depends on D76742: [lld-macho] Add basic symbol table output.

Diff Detail

Event Timeline

int3 created this revision.Mar 27 2020, 1:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 1:04 AM
int3 edited the summary of this revision. (Show Details)Mar 27 2020, 1:12 AM
int3 added a reviewer: Ktwu.Mar 28 2020, 1:38 AM
int3 updated this revision to Diff 253675.Mar 30 2020, 1:05 PM

make error messages lowercase

int3 updated this revision to Diff 253761.Mar 30 2020, 6:28 PM

update test to use symtab addresses

ruiu added inline comments.Mar 31 2020, 1:30 AM
lld/MachO/Driver.cpp
146–148

Does this have to be in the for loop?

149–151

Do you have to process this option in a for loop? It looks like you can do somethin glike

config->outputFile = args.getLastArgValue(OPT_o, "a.out");
config->installName = args.getLastArgValue(OPT_install_name, config->outputFile);
lld/MachO/SyntheticSections.h
99

I'd write a class comment.

int3 updated this revision to Diff 254066.Mar 31 2020, 6:04 PM
int3 marked 4 inline comments as done.

address comments

lld/MachO/Driver.cpp
149–151

Oh, that's indeed nicer. Thanks!

ruiu accepted this revision.Mar 31 2020, 9:47 PM

LGTM

This revision is now accepted and ready to land.Mar 31 2020, 9:47 PM
int3 updated this revision to Diff 254339.Apr 1 2020, 4:29 PM

rebase

int3 updated this revision to Diff 254341.Apr 1 2020, 4:31 PM

rebase

Nice!

lld/MachO/SyntheticSections.cpp
142

This can be done in a follow-up, but we should be checking if the symbol has hidden visibility before exporting it, right?

lld/MachO/SyntheticSections.h
24

Nit: why the trailing underscore?

lld/include/lld/Common/Sort.h
1 ↗(On Diff #254341)

Was this meant to be part of a previous diff?

lld/test/MachO/dylib.s
7

Where's the corresponding FileCheck for these?

11

Which part of the test is checking this?

13

Is it possible to test these via objdump or readobj instead of obj2yaml? That's more conventional in LLD tests, and their output is probably a bit more stable.

int3 updated this revision to Diff 256482.Apr 9 2020, 7:11 PM

rebase

int3 updated this revision to Diff 256493.Apr 9 2020, 8:06 PM
int3 marked 7 inline comments as done.

address comments

int3 marked 4 inline comments as done.Apr 9 2020, 8:06 PM
int3 added inline comments.
lld/MachO/SyntheticSections.cpp
142

yeah probably. I'll add a TODO

lld/MachO/SyntheticSections.h
24

export is a reserved keyword in C++ :)

lld/include/lld/Common/Sort.h
1 ↗(On Diff #254341)

ugh, rebasing is hard

lld/test/MachO/dylib.s
7

oops

11

The line directly below -- it's checking that lld doesn't error out even when passed -e missing_entry. I'll elaborate the comment.

13

Yeah we can use llvm-objdump --macho --dylib-id. While checking that out I realize we can look for load commands in general using llvm-objdump --macho --all-headers, so load-commands.s can use that too and the test suite can be entirely free of obj2yaml

int3 updated this revision to Diff 256494.Apr 9 2020, 8:11 PM
int3 marked 4 inline comments as done.

update

int3 edited the summary of this revision. (Show Details)Apr 10 2020, 12:21 AM
ruiu accepted this revision.Apr 12 2020, 9:33 PM

LGTM

lld/MachO/SyntheticSections.cpp
154

auto * -> const Defined *

lld/MachO/Writer.cpp
356

When we reach here, it's an internal error. So this line should be llvm_unreachable rather than error.

396

Ditto

MaskRay added inline comments.Apr 12 2020, 10:57 PM
lld/test/MachO/symtab.s
18–19

It seems that the previous diff should not add the symbols which will be deleted by this diff...

int3 updated this revision to Diff 257901.Apr 15 2020, 4:59 PM

order section name declarations by output order

smeenai added inline comments.Apr 21 2020, 1:49 PM
lld/MachO/Writer.cpp
356

This one still needs addressing.

396

Same here.

lld/test/MachO/symtab.s
18–19

The export trie diff will restore these symbols, so I think this is okay. (It's because this diff only supports exporting a single symbol.)

int3 updated this revision to Diff 260432.Apr 27 2020, 1:08 PM

address comments

This revision was automatically updated to reflect the committed changes.

Reverted in rGb52bc2653bbc8da852a804afbb34836bb7f1e58a because of a UBSan failure in D76742.

smeenai reopened this revision.Apr 28 2020, 11:44 AM
This revision is now accepted and ready to land.Apr 28 2020, 11:44 AM
int3 abandoned this revision.Apr 28 2020, 8:43 PM

Huh, didn't realize that reverting a diff reopens it in LLVM's Phabricator. This has been relanded as rG62b8f32f7699: [lld-macho][reland] Add support for emitting dylibs with a single symbol.

lld/MachO/Writer.cpp