Generate the WASI interface from witx. by sunfishcode · Pull Request #136 · WebAssembly/wasi-libc

sunfishcode · 2019-11-19T01:32:03Z

This replaces the hand-maintained <wasi/core.h> header with a
<wasi/api.h> generated from witx.

Most of the churn here is caused by upstream WASI renamings; hopefully
in the future ABI updates will be less noisy.

@cjihrig This obviates #135, and I hope it'll be a more robust approach as the API continues to evolve.

This replaces the hand-maintained <wasi/core.h> header with a <wasi/api.h> generated from witx. Most of the churn here is caused by upstream WASI renamings; hopefully in the future ABI updates will be less noisy.

alexcrichton · 2019-11-19T15:14:37Z

Would it make sense to include a submodule and/or reference to the wasi repo, and then check that the generated code matches the reference on CI?

(to ensure that if the reference is changed and/or the generator the generated code is updated as well)

sbc100

In general looks great! Very excited to see this.

I'm not totally convinced the tool for generating the header should live in this repo. Although I don't have any great ideas for a better place for it. At least maybe we can discuses that separately? How would you feel about splitting this change such that tool is a separate PR that we can discuses. The actual libc change LGTM as is so no need to block that.

sbc100 · 2019-11-19T16:56:08Z

 #include <fcntl.h>
 #include <string.h>

+#ifdef __wasilibc_unmodified_upstream // fstat


This doesn't seem to make sense.. surly __WASI maros don't belong in the "unmodified" section?

It's a special case, and I agree it's confusing. The policy is documented at the bottom of this README.md. The idea was, we'd allow CLOUDABI_ -> __WASI_ renaming without #ifdef markers, because that's super common, and a mechanical change, so it's not interesting to track.

But, now that the witx file renamed a lot of things, it's becoming less obvious that this whole #ifdef scheme is still worthwhile. We dropped it for libpreopen; it may soon make sense to drop it for cloudlibc too.

sbc100 · 2019-11-19T16:59:02Z

      if (event->error == __WASI_EBADF) {
+#else
+      if (event->error == __WASI_ERRNO_BADF) {
+#endif


Yeah I guess what happen is that these blocks must have be modified without being tagged. So mabye the "unmodified" blocmk should still have clound abi macros (presumably that is what the original had)?

Yeah, see my answer above.

sbc100 · 2019-11-19T17:02:10Z

@@ -0,0 +1,2569 @@
+/**
+ * THIS FILE IS AUTO-GENERATED!


By whom? Where I go if I want to modify it?

I've now added a patch to add more information here.

sunfishcode · 2019-11-20T00:10:14Z

I'm not totally convinced the tool for generating the header should live in this repo. Although I don't have any great ideas for a better place for it. At least maybe we can discuses that separately? How would you feel about splitting this change such that tool is a separate PR that we can discuses. The actual libc change LGTM as is so no need to block that.

I agree, but I also don't have great ideas for a better place for it in its current form. It's specific to wasi-libc, and still pretty experimental. What would you think of leaving it here for now, but re-evaluating it after it's settled into its role a little?

sbc100 · 2019-11-20T01:17:47Z

Isn't it C-specific rather then wasi-libc specific? i.e. wasi-libc is just one way to build something useful on C on top of this C header file? But I guess this is will at least the primary consumer.

I can maybe see this living in WebAssembly/wasi .. one can imagine generators for many different languages in there, as we already have a president of rust tooling in that repo.

sunfishcode · 2019-11-20T01:32:17Z

The <wasi/api.h> header is hard-wired into the tool right now, plus __WASI_ names, and some workarounds for WASI-specific issues. Witx will also actively evolving and will require this tool to change to better support eg. modularity, which will also require coordination with libc code. In the future it'll be a more general tool, but there's work to do before we get there.

pchickey · 2019-11-20T01:38:22Z

I agree that this repo seems like the best place for this tool to live for now. We will want a bit of flexibility about merging changes into the witx crate in the WebAssembly/wasi repo, versus consuming those changes over here.

I like Alex's suggestion to point to WebAssembly/wasi as a submodule, and have scripts that re-generate these headers and check that they are unchanged in CI. If we do that, it would make sense to depend on witx by path.

sunfishcode · 2019-11-20T01:42:53Z

@alexcrichton @pchickey git submodules are pinned to a specific revision, so I'm unclear on the purpose of setting them up with scripts to check that the generated files are unchanged.

sbc100 · 2019-11-20T01:45:03Z

I guess the idea would be that you would roll the wasi submodule and in the same PR see the updated headers files and also ensure the header generator is in sync the the witx parser.. although actually it looks like you are pinning that dependency via crates.io and via submodule.

sbc100 · 2019-11-20T01:48:16Z

I'm not gonna block this PR if you guys think this is the right place for it. We can revisit later if needed.

I'm not huge fan of having rust tooling spread everywhere like this (mostly because its not my personal lingua fanca, but also because its also not the lingua fanca of C toolchain maintainers in general). But I'm not offering any alternatives right now.

alexcrichton · 2019-11-20T02:10:25Z

@sunfishcode I was thinking of something like https://github.com/bytecodealliance/wasi/tree/master/crates/generate-raw where with a submodule here we'd get benefits like:

If you're only changing the generator, a PR shows that
If all you're doing is updating the submodule, a PR shows that
It's always clear where the header file is being generated from
CI can be used to ensure that the generated code is, well, always generated

This isn't really critical, though, just something I've seen that's a good way to head off future headaches. Not really a blocker at all though. I haven't though too much how this fits into the general versioning story for wasi in general (and its evolving interfaces over time).

Also, add the witx filenames to the generated output, and just have `cargo run` auto-generate the api.h header, rather than using clap.

sunfishcode · 2019-11-21T06:31:39Z

@alexcrichton That's a good idea. I've now added a WASI submodule, which isn't needed unless you want to re-generate the header, and running cargo run auto-regenerates the header`.

alexcrichton · 2019-11-21T15:29:45Z

👍

The only other thing I'd do here is to hook this up to CI to fail CI if the generated code in the repository is different than what the generator generates, but that's pretty minor.

pchickey · 2019-11-21T17:42:15Z

 [dependencies]
-clap = "2"
 heck = "0.3.1"
 witx = "0.5.0"


A path dependency to witx is appropriate now that we're tracking WASI as a submodule, imo.

Makes sense; done.

pchickey

Good to go once CI is passing

Most editors these days can deal with eof=lf files, even on Windows, and this avoids trouble with headers and other generated files differing in line endings.

sunfishcode · 2019-11-22T02:35:39Z

The CI is now passing, at the cost of making git checkout text files with unix-style line endings on Windows. It would also be possible to make the diff ignore line-ending differences, but with text=lf, we don't need that, we avoid trouble with people running the generator tool on Windows and getting crlf/lf diffs, and we eliminate some line-ending differences in the generated sysroot too.

* Add the WASI repo as a submodule. Also, add the witx filenames to the generated output, and just have `cargo run` auto-generate the api.h header, rather than using clap. * Switch witx to a path dependency. * Add a test. * Add a test that the generated file is in sync with the generator. * Enable CI testing with Github Actions. * Fix the name of the wasi-headers directory. * Enable submodules. * Add a diff mechanism to help explain failures. * Sort the inputs for display. * More debugging. * More debugging. * Add a .gitattributes file forcing text files to be eol=lf. Most editors these days can deal with eof=lf files, even on Windows, and this avoids trouble with headers and other generated files differing in line endings.

Generate the WASI interface from witx.

9d686e9

This replaces the hand-maintained <wasi/core.h> header with a <wasi/api.h> generated from witx. Most of the churn here is caused by upstream WASI renamings; hopefully in the future ABI updates will be less noisy.

sbc100 reviewed Nov 19, 2019

View reviewed changes

sunfishcode mentioned this pull request Nov 21, 2019

Update seek constants #106

Closed

Add the WASI repo as a submodule.

07355b4

Also, add the witx filenames to the generated output, and just have `cargo run` auto-generate the api.h header, rather than using clap.

sunfishcode force-pushed the sunfish_snapshot_1 branch from a156442 to 07355b4 Compare November 21, 2019 06:29

pchickey reviewed Nov 21, 2019

View reviewed changes

sunfishcode added 4 commits November 21, 2019 11:41

Switch witx to a path dependency.

60173a3

Add a test.

fa3c285

Add a test that the generated file is in sync with the generator.

40161c3

Enable CI testing with Github Actions.

0976c90

pchickey approved these changes Nov 22, 2019

View reviewed changes

sunfishcode added 2 commits November 21, 2019 16:38

Fix the name of the wasi-headers directory.

f69af9d

Enable submodules.

2afa831

pchickey mentioned this pull request Nov 22, 2019

Update wasi-headers to use reorganized witx ast #139

Merged

sunfishcode added 5 commits November 21, 2019 17:22

Add a diff mechanism to help explain failures.

ec98fa6

Sort the inputs for display.

c22ad93

More debugging.

a54231b

More debugging.

38ba731

Add a .gitattributes file forcing text files to be eol=lf.

e984164

Most editors these days can deal with eof=lf files, even on Windows, and this avoids trouble with headers and other generated files differing in line endings.

sunfishcode force-pushed the sunfish_snapshot_1 branch from 029de46 to e984164 Compare November 22, 2019 02:13

sunfishcode merged commit 31bea9c into wasi_snapshot_preview1 Nov 22, 2019

sunfishcode deleted the sunfish_snapshot_1 branch November 22, 2019 03:46

sunfishcode restored the sunfish_snapshot_1 branch November 22, 2019 04:38

sunfishcode mentioned this pull request Nov 22, 2019

Correct the version of #136 on master #141

Merged

Conversation

sunfishcode commented Nov 19, 2019

Uh oh!

alexcrichton commented Nov 19, 2019

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunfishcode commented Nov 20, 2019

Uh oh!

sbc100 commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunfishcode commented Nov 20, 2019

Uh oh!

pchickey commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunfishcode commented Nov 20, 2019

Uh oh!

sbc100 commented Nov 20, 2019

Uh oh!

sbc100 commented Nov 20, 2019

Uh oh!

alexcrichton commented Nov 20, 2019

Uh oh!

sunfishcode commented Nov 21, 2019

Uh oh!

alexcrichton commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchickey left a comment

Choose a reason for hiding this comment

Uh oh!

sunfishcode commented Nov 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sbc100 commented Nov 20, 2019 •

edited

Loading

pchickey commented Nov 20, 2019 •

edited

Loading