Page MenuHomePhabricator

[lld-macho] Define __mh_*_header synthetic symbols.
ClosedPublic

Authored by oontvoo on Feb 18 2021, 3:46 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Did you intend to mark this diff as ready for review?

Not yet - i'm adding a test

int3 added a comment.Mar 10 2021, 1:32 PM

Testing aside, the most recent changes in the diff don't seem to have addressed the concerns in my earlier comment:

It really should have most of the semantics of a Defined symbol, and having it as a separate case means it's easy to forget to handle it. I've been thinking of having Defined symbols contain a SectionPointerUnion instead of an InputSection field, which should be all that's needed to model these synthetic symbols.

Testing aside, the most recent changes in the diff don't seem to have addressed the concerns in my earlier comment:

It really should have most of the semantics of a Defined symbol, and having it as a separate case means it's easy to forget to handle it. I've been thinking of having Defined symbols contain a SectionPointerUnion instead of an InputSection field, which should be all that's needed to model these synthetic symbols.

Yup, looking ๐Ÿ‘€

oontvoo updated this revision to Diff 331149.Mar 16 2021, 8:10 PM

updated diff

oontvoo retitled this revision from [lld-macho] Define __mh_bundle_header as symbol (similar to ___dso_handle) to [lld-macho] Define __mh_*_header synthetic symbols..Mar 16 2021, 8:10 PM
oontvoo edited the summary of this revision. (Show Details)
oontvoo edited the summary of this revision. (Show Details)Mar 16 2021, 8:14 PM
oontvoo added inline comments.Mar 17 2021, 12:31 PM
lld/MachO/Driver.cpp
1060

Unlike the __dso_handle, these __mh_*_header's are implemented as undefined. (can see this by looking at the header section of a binary produced by ld64)

carlokok added inline comments.Mar 17 2021, 12:36 PM
lld/MachO/Driver.cpp
1061

Would there be a chance of having a mh_*_header that works for all targets, while this gets added anyway? something like mh_header?

int3 added a comment.Mar 17 2021, 1:05 PM

Hm, why is __mh_execute_header an undefined?

It seems like it's a defined symbol to me:

(base) ~/tmp: cat test.cpp
int main() { return 0; }
(base) ~/tmp: clang -c test.cpp
(base) ~/tmp: ld -lSystem test.o -o test
(base) ~/tmp: llvm-objdump --syms test

test:	file format mach-o 64-bit x86-64

SYMBOL TABLE:
0000000100000000 g     F __TEXT,__text __mh_execute_header
0000000100003fa0 g     F __TEXT,__text _main
0000000000000000         *UND* dyld_stub_binder

Weirdly enough, when I swap out LLD for ld64 in our unit tests, it seems like load-command.s generates __mh_execute_header as an absolute symbol. Aha, turns out that it's an absolute symbol only if we aren't linking a PIE.

Also, ld64 doesn't appear to emit __mh_bundle_header:

--- a/lld/test/MachO/lit.local.cfg
+++ b/lld/test/MachO/lit.local.cfg
@@ -8,7 +8,7 @@ import os
 #
 # Note however that this does not apply to `-syslibroot`: each instance of that
 # flag will append to the set of library roots.
-lld = ('ld64.lld -arch x86_64 -platform_version macos 10.0 11.0 -syslibroot ' +
+lld = ('ld -syslibroot ' +
     os.path.join(config.test_source_root, "MachO", "Inputs", "MacOSX.sdk"))
 config.substitutions.append(('%lld', lld + ' -fatal_warnings'))
 config.substitutions.append(('%no_fatal_warnings_lld', lld))
(base) Output/load-commands.s.tmp: pwd
/Users/jezng/src/llvm-project/build/release/tools/lld/test/MachO/Output/load-commands.s.tmp
(base) Output/load-commands.s.tmp: llvm-objdump --syms executable

executable:	file format mach-o 64-bit x86-64

SYMBOL TABLE:
0000000100000000 g       *ABS* __mh_execute_header
0000000100003fb0 g     F __TEXT,__text _main
0000000000000000         *UND* dyld_stub_binder
(base) Output/load-commands.s.tmp: llvm-objdump --syms bundle

bundle:	file format mach-o 64-bit x86-64

SYMBOL TABLE:
0000000000003fb0 g     F __TEXT,__text _main

I figured it might be interesting to test if our input object files can link against these symbols, and it appears to be the case:

(base) ~/tmp: cat test.s
.text
.globl _main
_main:
  mov __mh_execute_header@GOTPCREL(%rip), %rax
  ret
(base) ~/tmp: llvm-mc test.s -triple=x86_64-apple-macos11.0 -filetype=obj -o test.o
(base) ~/tmp: ld test.o -lSystem -o test
(base) ~/tmp: ld test.o -lSystem -o test -dylib
Undefined symbols for architecture x86_64:
  "__mh_execute_header", referenced from:
      _main in test.o
ld: symbol(s) not found for architecture x86_64

We should add a test for that.

lld/MachO/Symbols.h
84โ€“86

hm why the need to replace linkerInternal with this? (Do we still need it if the __mh_* symbols are Defineds?)

oontvoo planned changes to this revision.Mar 17 2021, 10:24 PM

Hm, why is __mh_execute_header an undefined?

It seems like it's a defined symbol to me:

Yes, my bad - I thought I was testing against ld64, but ended up using the older darwin port, which seemed to incorrectly implemented it as UNDEF.

Weirdly enough, when I swap out LLD for ld64 in our unit tests, it seems like load-command.s generates __mh_execute_header as an absolute symbol. Aha, turns out that it's an absolute symbol only if we aren't linking a PIE.

No that's not weird - they said the _execute_ is an absolute symbol.
The rest is N_SECT.

Also, ld64 doesn't appear to emit __mh_bundle_header:

but it does ๐Ÿค” ... at least, the code looks like should. (it's not included in the symbol table though)

lld/MachO/Driver.cpp
1061

I'm not sure I understand this question.

Are you asking if they would introduce a new symbol to replace all this?
(if so, I don't know :) probably not given each of these symbol is unique to each file type )

Or are you asking/suggesting that in this implementation, we would make an mh_header class and move this code there?
If so, sure. I've moved the synthetic-symbols generations out-of-line for readability.

lld/MachO/Symbols.h
84โ€“86

How about calling it presentInSymbolTable?

Calling it linkerInternal is weird because all of these are linker internal ... it's just some of them are not to be included in the symbol table

oontvoo updated this revision to Diff 331463.Mar 17 2021, 10:26 PM

updated diff

oontvoo updated this revision to Diff 331464.Mar 17 2021, 10:28 PM
oontvoo marked an inline comment as not done.

format

int3 added a comment.Mar 17 2021, 10:53 PM

Will do a proper review tomorrow... just some quick comments before I go to sleep.

No that's not weird - they said the _execute_ is an absolute symbol.

I said it was weird because in my first test.cpp example it isn't an absolute symbol, but in the load-command.s test it was. I realized that was because our lit.local.cfg file specifies an early macOS platform version, whereas by default ld64 will target a modern macOS (>= 10.6) where PIEs are the default (see our isPie() function). We should probably mimic this behavior.

lld/MachO/Symbols.h
84โ€“86

Let me sleep on this... but naming aside, I'm also trying to understand if we need a tristate enum rather than a boolean

lld/MachO/SyntheticSections.cpp
72โ€“73
-preload    Produces a mach-o file in which the mach_header, load commands, and sym-
            bol table are not in any segment.  This output type is used for firmware
            or embedded development where the segments are copied out of the mach-o
            into ROM/Flash.

I think it's unlikely that we'll add real support for -preload any time soon (due to lack of use cases), so I wouldn't bother even partially supporting the MH_PRELOAD case.

int3 added a comment.Mar 18 2021, 12:35 PM

but it does ๐Ÿค” ... at least, the code looks like should. (it's not included in the symbol table though)

Ah I see -- we can link against it, but it doesn't appear in the final symbol table...

I'm trying to understand the significance of __mh_execute_header being defined while the others are undefined though. Why not make all of them Defined?

Also I think the tests still need updating :)

lld/MachO/Symbols.h
84โ€“86

Well we have "extern" to indicate external symbols that appear in the symbol table -- c.f. privateExtern. So the "internal" part is meant to contrast with that. but I agree that having "linker" in any variable name that's part of a linker is criminally redundant :P

How about syntheticIntern, or even just internal? I'm not a fan of LongJavaNamesThatSayWhatTheyDo, but if you think those are too cryptic, I'd also be fine with includeInSymtab ("include" seems to make more sense than "present" since nothing is present until we write out the binary.)

</bikeshed>

lld/MachO/SyntheticSections.cpp
1018โ€“1020

this could be a lambda. We should avoid macros if possible

1033

not sure I follow this last part of the sentence. How are they private to a TU, when they only exist in the output binary?

oontvoo planned changes to this revision.Mar 18 2021, 12:40 PM
oontvoo updated this revision to Diff 331711.Mar 18 2021, 3:50 PM
oontvoo marked 6 inline comments as done.

updated diff

oontvoo updated this revision to Diff 331732.Mar 18 2021, 5:29 PM
oontvoo marked an inline comment as not done.

updated tests

int3 added a comment.Mar 18 2021, 6:41 PM

So if I understand correctly, we need to make __mh_execute_header look like it's part of __text when we emit the symbol table. I have two ideas:

  1. This is kinda sneaky but ... if all we care about is the symbol table entry matching the right section, that means we just need to get the n_sect value to match, so we could have MachHeaderSection pretend to be the text section by giving it a section index of 1.
  2. We could make MachHeaderSection inherit from an InputSection rather than a OutputSection (SyntheticSection), and then have it be part of the __TEXT MergedOutputSection. This is more architecturally consistent, but would probably require a good deal of refactoring.

My inclination is to go with #1; I think it's a simple enough hack, and unlikely to bite us in the ass down the road (fingers crossed)

lld/MachO/SyntheticSections.cpp
1003

addSynSymMachHeader is kind of a mouthful... how about addHeaderSymbol?

1010โ€“1011

I would drop this comment... the default assumption is that we're emulating ld64's behavior.

1013

instead of passing isPie, we could just check if config->isPic here. MH_EXECUTE + isPic => PIE.

1018

From ld64's output, it looks like the absolute symbol will still have the address of the header. I guess we could support it by putting in.header->addr here, but we would need to move createSyntheticSymbols after assignAddresses.

(If it turns out to be uglier than that, I think it's fine that we don't have 100% parity; modern macOS binaries are all PIE, so this will be a very rare use case.)

1041

can rm

oontvoo updated this revision to Diff 331753.Mar 18 2021, 8:59 PM
oontvoo marked 6 inline comments as done.

Updated diff:

  • give MachHeaderSection a section index of 1.
  • added tests
lld/MachO/Symbols.h
84โ€“86

went with includeSymtab (because it's less cryptic than internal) Thanks!

lld/MachO/SyntheticSections.cpp
1003

well no-one reads it out loud, but ok. :-)

1018

I've added a FIXME instead. (assignAddresses() is in finalize....(), which is way later )

1033

Updated.

oontvoo updated this revision to Diff 331759.Mar 18 2021, 9:56 PM

fixed map-file test

int3 accepted this revision.Mar 19 2021, 9:15 AM

Thanks for getting through this surprisingly hairy feature (and putting up with my nits ;) )

lld/MachO/SymbolTable.h
55

the return type can still be Defined

lld/MachO/SyntheticSections.cpp
1010
1027โ€“1029

nit: formatting (line length)

1043

would be nice to add a llvm_unreachable("unexpected output type") here just to be safe

lld/MachO/Writer.cpp
753โ€“754

could just assign the index = 1 in the constructor of the MachHeaderSection, no need to check for it in a loop here

lld/test/MachO/mh_dylib_header.s
1โ€“2 โ†—(On Diff #331759)

file naming nit: mh_dylib_header.s is a bit misleading since we're doing more than just testing that symbol. How about mh-header-link.s or mh-header-resolution.s?

4 โ†—(On Diff #331759)

nit: REQUIRES should be the first line in the file

7โ€“8 โ†—(On Diff #331759)

(no need to repeat "in a dylib")

11 โ†—(On Diff #331759)

I mean I guess both flags work but one-char flags typically use one dash

13 โ†—(On Diff #331759)

we typically add -o /dev/null so that if it unexpectedly passes we're not creating an a.out in a random directory. ditto for ERR-EXEC

20 โ†—(On Diff #331759)
22 โ†—(On Diff #331759)

The "-arch x86_64 -platform_version macOS 10 11" shouldn't be necessary since we are already defining that in lit.local.cfg

(same for other %lld invocations)

33โ€“35 โ†—(On Diff #331759)

I think this is happening because we are linking a non-PIE, and due to the FIXME in SyntheticSections.cpp, LLD thinks that it is an absolute symbol at address 0, so the jump is too large (because it needs to cross __PAGEZERO). Whereas if it were actually pointing to the header, then it would be a much shorter jump.

I think we should just invoke %lld -pie and use GOTPCREL here, and skip testing the non-PIE case for now.

It would also be nice to check that the relocation resolves to the right address via llvm-objdump -m -d

lld/test/MachO/mh_execute_header.s
1 โ†—(On Diff #331759)

nit: mh-execute-header.s

This revision is now accepted and ready to land.Mar 19 2021, 9:15 AM
oontvoo updated this revision to Diff 331947.Mar 19 2021, 11:15 AM
oontvoo marked 13 inline comments as done.

addressed review comments + rebase

This revision was landed with ongoing or failed builds.Mar 19 2021, 11:15 AM
This revision was automatically updated to reflect the committed changes.
  1. We could make MachHeaderSection inherit from an InputSection rather than a OutputSection (SyntheticSection), and then have it be part of the __TEXT MergedOutputSection. This is more architecturally consistent, but would probably require a good deal of refactoring.

I don't have the full context here, but does this tie into the larger questions of whether SyntheticSections should be InputSections or OutputSections? https://bugs.llvm.org/show_bug.cgi?id=49780

To be clear, I'm not saying we're at the point where we should consider that refactoring yet; I'm just wondering if it's another data point that should be included when we eventually make that decision :)

int3 added a comment.Mar 30 2021, 7:06 PM

After D98545: [lld-macho][nfc] Give every SyntheticSection a fake InputSection and this diff, I feel like having SyntheticSections be OutputSections is clean enough. Not sure how thunks might be sped up if synthetics were InputSections though, I guess we'll see. But yeah, this diff is indeed another data point to consider.