This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Enable main-function signature rewriting for WASI
ClosedPublic

Authored by sunfish on Jan 28 2019, 5:19 AM.

Details

Summary

Enable the functionality to rewrite the signature of "int main(void)" functions to "int main(int argc, char *argv[])", so that they can be called by the C startup code, since WebAssembly requires caller and callee signatures to match.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish created this revision.Jan 28 2019, 5:19 AM

I tried to enable this by default recently but had to revert: https://reviews.llvm.org/D54117.

I'm fine for this to land in the mean time but you might want to be aware of this issue.

I tried to enable this by default recently but had to revert: https://reviews.llvm.org/D54117.

I'm fine for this to land in the mean time but you might want to be aware of this issue.

The patch here avoids the envp issue by limiting the transformation to the (standard-approved) int main(void) case, and not arbitrary signatures. So if it sees int main(int argc, char *argv[], char *envp[]), it won't do anything.

I tried to enable this by default recently but had to revert: https://reviews.llvm.org/D54117.

I'm fine for this to land in the mean time but you might want to be aware of this issue.

The patch here avoids the envp issue by limiting the transformation to the (standard-approved) int main(void) case, and not arbitrary signatures. So if it sees int main(int argc, char *argv[], char *envp[]), it won't do anything.

In that case can you just remove the TemporaryWorkarounds flag and always do this transform? That is the behaviour we want and I only reverted my change because of the envp issue.

I mean with your fix we don't need this to be wasi specific do we?

I mean with your fix we don't need this to be wasi specific do we?

It needs to align with what crt1.o does though, which is specific to the OS. For WASI's crt1.o, my current working theory is to just support the two standard-required cases and not the envp case. But for the unknown OS, we don't know what people might want to link to.

I mean with your fix we don't need this to be wasi specific do we?

It needs to align with what crt1.o does though, which is specific to the OS. For WASI's crt1.o, my current working theory is to just support the two standard-required cases and not the envp case. But for the unknown OS, we don't know what people might want to link to.

But we already decided we wanted this behaviour and my change only got reverted because it was demoting the 3-arg form to the 2-arg form. Musl's crt0 currently expects the 2-arg form so this would align exactly with what you have done here: promote the 0-arg to 2-arg form but leave the 3-arg form (and all other forms) alone.

sunfish updated this revision to Diff 183988.Jan 28 2019, 4:40 PM
sunfish edited the summary of this revision. (Show Details)
  • Remove the target constraint, allowing the main rewrite logic to work on all wasm32 targets.
  • Remove the now-unneeded "wasm-temporary-workarounds" flag altogether.
  • This uncovered some bugs with wrappers for main when main is only a declaration; fix those as well.

But we already decided we wanted this behaviour and my change only got reverted because it was demoting the 3-arg form to the 2-arg form. Musl's crt0 currently expects the 2-arg form so this would align exactly with what you have done here: promote the 0-arg to 2-arg form but leave the 3-arg form (and all other forms) alone.

Ok, that works for me. And this way we can get rid of the wasm-temporary-workarounds flag completely, which is a nice bonus. Patch updated.

sbc100 accepted this revision.Jan 28 2019, 8:26 PM
sbc100 added inline comments.
test/CodeGen/WebAssembly/main-three-args.ll
4

Drop the WASI

This revision is now accepted and ready to land.Jan 28 2019, 8:26 PM
This revision was automatically updated to reflect the committed changes.