Skip to content

🍒 [WebAssembly] Emit clangast in custom section aligned by 4 bytes #3451

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

Merged

Conversation

kateinoigakukun
Copy link
Member

@kateinoigakukun kateinoigakukun commented Oct 20, 2021

Emit __clangast in custom section instead of named data segment
to find it while iterating sections.
This could be avoided if all data segements (the wasm sense) were
represented as their own sections (in the llvm sense).
This can be resolved by WebAssembly/tool-conventions#138

And the on-disk hashtable in clangast needs to be aligned by 4 bytes,
so add paddings in name length field in custom section header.

The length of clangast section name can be represented in 1 byte
by leb128, and possible maximum pads are 3 bytes, so the section
name length won't be invalid in theory.

Fixes https://bugs.llvm.org/show_bug.cgi?id=35928

Differential Revision: https://reviews.llvm.org/D74531

Cherry-pick from llvm@1813fde to build Swift stdlib for WebAssembly importing SwiftShims module.

CC: @MaxDesiatov, @compnerd

Emit __clangast in custom section instead of named data segment
to find it while iterating sections.
This could be avoided if all data segements (the wasm sense) were
represented as their own sections (in the llvm sense).
This can be resolved by WebAssembly/tool-conventions#138

And the on-disk hashtable in clangast needs to be aligned by 4 bytes,
so add paddings in name length field in custom section header.

The length of clangast section name can be represented in 1 byte
by leb128, and possible maximum pads are 3 bytes, so the section
name length won't be invalid in theory.

Fixes https://bugs.llvm.org/show_bug.cgi?id=35928

Differential Revision: https://reviews.llvm.org/D74531
@kateinoigakukun
Copy link
Member Author

@swift-ci Please test

@MaxDesiatov
Copy link

@shahmishal would you be available for review and merge? Thanks!

@shahmishal
Copy link
Member

cc: @hyp

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Oct 25, 2021

Do we need to wait for @hyp 's approval? Or is it ok to merge this for now and revert if it breaks something?
Please let me know if this kind of fix should not be cherry-picked to the swift branch by some policy.

After this patch, swiftwasm can use apple/llvm-project instead of its fork and it would improve the maintainability of the project.

@compnerd
Copy link
Member

Isn't part of this path upstreamed? I don't think that this should be going here if it is. I think that waiting for @hyp is probably a good idea here.

@kateinoigakukun
Copy link
Member Author

kateinoigakukun commented Oct 25, 2021

@compnerd This patch has been already landed in the upstream LLVM https://reviews.llvm.org/rG1813fde9cc0b56cee42d9b82e6f22fa00a59cdf9

OK, let's wait @hyp 👍

@compnerd
Copy link
Member

Ah, I failed to read the truncated link as a cherry-pick. Thanks! This is a clean cherry-pick from upstream. The changes are isolated. I think that this change is fine to merge, but I'd still prefer to have a cross-repository test first. Have you run that yet?

@kateinoigakukun
Copy link
Member Author

OK, cross-repos test has been passed now

@compnerd
Copy link
Member

LGTM, I think that this should be safe to merge.

@compnerd compnerd merged commit 92dc250 into swiftlang:stable/20210726 Oct 27, 2021
@kateinoigakukun kateinoigakukun deleted the katei/cherry-pick-D74531 branch October 27, 2021 02:33
MaxDesiatov added a commit to swiftwasm/swift that referenced this pull request Nov 5, 2021
As previously announced by @kateinoigakukun:
>The last required commit for swiftwasm has been cherry-picked to the swift's llvm branch! swiftlang/llvm-project#3451
>We no longer need to maintain our llvm downstream fork
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