Skip to content

[Swift+WASM] [IRGen] Disable COMDAT for reflection metadata #24133

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

Closed
wants to merge 1 commit into from

Conversation

zhuowei
Copy link
Contributor

@zhuowei zhuowei commented Apr 18, 2019

LLVM's Wasm COMDAT code assumes each COMDAT will be in its own section. This works for C++ on Clang, since each COMDAT is emitted into its own .rodata. section, but not for Swift metadata, which are all placed in the same section.

Disable COMDAT support when emitting reflection metadata.

This matches the behaviour on ELF (where COMDATs are disabled) and MachO (which doesn't support COMDAT)

This shouldn't affect existing platforms.


@ddunbar @MaxDesiatov This avoids an LLVM assert when building stdlib for WebAssembly: should be helpful for SR-9307.

@compnerd: You added the COMDAT support originally for Windows/COFF, right? Should the check for COMDAT support use a whitelist instead of a blacklist, since ELF, MachO, and now WASM all disabled it?

LLVM's Wasm COMDAT code assumes each COMDAT will be in its own section.
This works for C++ on Clang, since each COMDAT is emitted into its own
.rodata.<symbol name> section,
but not for Swift metadata, which are all placed in the same section.

Disable COMDAT support when emitting reflection metadata.

This matches the behaviour on ELF (where COMDATs are disabled) and MachO
(which doesn't support COMDAT)

This shouldn't affect existing platforms.
@compnerd
Copy link
Member

This feels wrong. The reason that ELF is disabling it is because of a bug in gold, it should be enabled otherwise. This is a size issue.

Why does this not work on WASM? It should - the sections are coalesced (.rodata.* -> .rodata). The same should occur with the metadata.

@zhuowei
Copy link
Contributor Author

zhuowei commented Apr 19, 2019

@compnerd As I mentioned, WASM only supports comdats with section coalescing: Swift emits all the metadata of a certain type together in the same section, not in individual sections, so LLVM asserts with a cannot rename symbol name error because the comdat code always tries to start a new section when encountering comdat.

You can get this to happen in C++ as well: https://gist.github.com/zhuowei/597b77698634aac9b302d43b5c464838

Edit: Would emitting each comdat in its own section work? I haven't tried that; is it worth trying?

@compnerd
Copy link
Member

Yes, they should be in unique section names (see how -ffunction-sections -fdata-sections work in ELF). That should work just fine, and would be better IMO.

@CodaFi
Copy link
Contributor

CodaFi commented Apr 14, 2020

@zhuowei Are you still pursuing this?

@MaxDesiatov
Copy link
Contributor

He doesn't, at least to my knowledge. I can pick it up, but it would make sense to do that in a separate PR probably, so feel free to close this one.

@CodaFi CodaFi closed this Jul 27, 2020
@MaxDesiatov MaxDesiatov deleted the wasm-disable-comdat branch July 27, 2020 17:08
@MaxDesiatov MaxDesiatov restored the wasm-disable-comdat branch July 27, 2020 17:08
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.

4 participants