write: Create symbols for sections

[afonso360]

👋 Hey,

It looks like we don't automatically create a symbol when adding a section, this PR changes that.

See rust-lang/rustc_codegen_cranelift#1249 (comment) for some context.

[philipc]

Section symbols aren't required for every section. Is there a reason the caller can't do this for the sections that it knows need the symbols?

[afonso360]

Not really, but I thought it would make sense to have that as a default and perhaps add an opt out version of add_symbol. But it might be better to do that as a user of the crate.

[philipc]

I prefer not to have it as the default because generally they aren't required (at least for ELF). If there's something special about COFF that requires it, then maybe we can add something to detect those cases. Have you tested whether this change fixes anything in the linked issue, or did you only check that section symbols were added? It's not clear to me that this will actually fix anything.

[afonso360]

This is not a full fix for the issue above. I've only tested with both the section symbols and the read only initializer sections, and that does fix it although I was planning on doing that as a separate PR.

Although testing it again it seems that just marking the sections as readonly seems sufficient to fix the issue. Given that I think we can probably close this.

Do you think marking the .CRT$XC* subsections as read only would make sense to include as part of this crate, or would it be better to fix as a user of the crate?

[philipc]

Do you think marking the .CRT$XC* subsections as read only would make sense to include as part of this crate

No, the user determines that by specifying the SectionKind for add_section. This crate shouldn't be second guessing what the user requested.

[bjorn3]

I think the .CRT$XC* issue is that https://github.com/bytecodealliance/wasmtime/blob/e4dc9c79443259e40f3e93b9c7815b0645ebd5c4/cranelift/object/src/backend.rs#L460 needs to be ReadOnlyDataWithRel rather than Data.

[philipc]

Yes that looks like the issue. However, currently there isn't a SectionKind::ReadOnlyDataWithRel. Maybe we need to add one, since the current behaviour for StandardSection::ReadOnlyDataWithRel varies between formats. That is, SectionKind::Data might be correct for ELF so it shouldn't be changed.