Details
- Reviewers
int3 - Group Reviewers
Restricted Project - Commits
- rG66f340051ac2: [lld-macho] Define __mh_*_header synthetic symbols.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
So the reason why I hadn't implemented __mh_*_header symbols is that I'm not happy with the current implementation of DSOHandle. 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. The one thing I'm concerned about is the performance implications; I think it will be fine, but it would be good to run some benchmarks, e.g. by linking a large project like LLVM or Chromium.
Btw, I've just filed a bunch of tasks on the LLVM bug tracker for known missing features, and I've assigned this one to you :)
Hopefully we can coordinate there in the future to avoid wasted work -- I'd have given you more context beforehand if I knew you were going to work on this.
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.
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) |
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? |
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?) |
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? Or are you asking/suggesting that in this implementation, we would make an mh_header class and move this code there? | |
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 |
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. |
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? |
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:
- 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.
- 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 |
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. |
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 |
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 :)
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.
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)