gimli-rs / object

A unified interface for reading and writing object file formats
https://docs.rs/object/
Apache License 2.0
673 stars 157 forks source link

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

Closed philipc closed 6 months ago

philipc commented 6 months ago

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 commented 6 months ago

This is to fix https://github.com/rust-lang/rustc_codegen_cranelift/issues/1456, along with the cranelift change in https://github.com/philipc/wasmtime/commit/d851125fab33b10ec51038bdbc323373b1ab07d6

philipc commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@bjorn3 Are you happy with this change?

bjorn3 commented 6 months ago

This PR looks fine to me.