Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

write/macho: ensure Mach-O subsections are not zero size #676

Merged
merged 1 commit into from
May 4, 2024

Conversation

philipc
Copy link
Contributor

@philipc philipc commented May 1, 2024

For Mach-O, add_symbol_data now ensures that the symbol size is at least 1 when subsections via symbols are enabled. This change was made to support linking with ld-prime. It is also unclear how this previously managed to work with ld64.

write::Object::add_subsection no longer enables subsections via symbols for Mach-O. Use set_subsections_via_symbols instead. This change was made because Mach-O subsections are all or nothing, so this decision must be made before any symbols are added.

write::Object::add_subsection no longer adds data to the subsection. This change was made because it was done with append_section_data, but this is often not the correct way to add data to the subsection. Usually add_symbol_data is a better choice.

@philipc
Copy link
Contributor Author

philipc commented May 1, 2024

This is to fix rust-lang/rustc_codegen_cranelift#1456, along with the cranelift change in philipc/wasmtime@d851125

@philipc
Copy link
Contributor Author

philipc commented May 1, 2024

Note that rustc_codegen_cranelift isn't currently setting MH_SUBSECTIONS_VIA_SYMBOLS, so there still needs to be a change somewhere to enable that (maybe cranelift-object can always set it). Also note that ld64 still works even with that flag set and without the fix in this PR, so I'm not completely sure that this bug is related to MH_SUBSECTIONS_VIA_SYMBOLS, although it does make sense to me that zero size symbols can't work with it set.

@philipc
Copy link
Contributor Author

philipc commented May 2, 2024

This code in llvm confirms that basing this on MH_SUBSECTIONS_VIA_SYMBOLS makes sense:
https://github.com/llvm/llvm-project/blob/176d6fbed3988032dbdabe7abfc5f54e1f0e2bbb/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L3812-L3816

llvm always sets MH_SUBSECTIONS_VIA_SYMBOLS, independent of -ffunction-sections:
https://github.com/llvm/llvm-project/blob/176d6fbed3988032dbdabe7abfc5f54e1f0e2bbb/llvm/lib/Target/X86/X86AsmPrinter.cpp#L990-L995

@philipc
Copy link
Contributor Author

philipc commented May 2, 2024

For zero length functions, llvm emits a noop instead:
https://github.com/llvm/llvm-project/blob/176d6fbed3988032dbdabe7abfc5f54e1f0e2bbb/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L1950-L1960

I'm not keen on handling this here. I think that's better done in cranelift if needed.

@philipc
Copy link
Contributor Author

philipc commented May 2, 2024

@bjorn3 Are you happy with this change?

@bjorn3
Copy link
Contributor

bjorn3 commented May 3, 2024

This PR looks fine to me.

For Mach-O, `add_symbol_data` now ensures that the symbol size
is at least 1 when subsections via symbols are enabled.
This change was made to support linking with ld-prime. It is also
unclear how this previously managed to work with ld64.

`write::Object::add_subsection` no longer enables subsections
via symbols for Mach-O. Use `set_subsections_via_symbols` instead.
This change was made because Mach-O subsections are all or nothing,
so this decision must be made before any symbols are added.

`write::Object::add_subsection` no longer adds data to the subsection.
This change was made because it was done with `append_section_data`,
but this is often not the correct way to add data to the subsection.
Usually `add_symbol_data` is a better choice.
@philipc philipc merged commit b77473f into gimli-rs:master May 4, 2024
10 checks passed
@philipc philipc deleted the macho-subsection branch May 4, 2024 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants