Skip to content
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

Compute layout of datatypes according to C ABI #181

Merged
merged 6 commits into from
Dec 20, 2019
Merged

Compute layout of datatypes according to C ABI #181

merged 6 commits into from
Dec 20, 2019

Conversation

pchickey
Copy link
Contributor

Based on #164

This is useful for validating pointers, enums, and flags in host code.

@pchickey
Copy link
Contributor Author

pchickey commented Dec 18, 2019

Dan pointed out the following spec exists to describe the C ABI: https://github.com/WebAssembly/tool-conventions/blob/master/BasicCABI.md#data-representation

I would still like to somehow test that the layout calculations match what llvm does. I tested the version of this algorithm that lived in lucet-idl against clang by randomly generating specs and rendering them to C, and adding static assertions about layout. That is probably too heavy-weight for this repo, because it would add a lot of testing deps, so I'll generate some weird types elsewhere and check in a small sample of tests that assert these calculations match how llvm lays them out.

@tlively
Copy link
Member

tlively commented Dec 18, 2019

Struct layout should be fine, but be aware that for calling conventions Rust only uses the official documented C ABI with its Emscripten targets and not with its WASI or wasm32-unknown-unknown targets. See WebAssembly/tool-conventions#88 for more information.

@bjorn3
Copy link

bjorn3 commented Dec 18, 2019

That incompatibility is only for the call ABI, not the memory ABI.

@sbc100
Copy link
Member

sbc100 commented Dec 18, 2019

Can this be used to re-instate the _Static_asserts that we lost when we transiation from wasi.h to api.h?

We used to have stuff like this that we no longer have:

 _Static_assert(offsetof(__wasi_dirent_t, d_next) == 0, "non-wasi data layout");  
 _Static_assert(offsetof(__wasi_dirent_t, d_ino) == 8, "non-wasi data layout");   
 _Static_assert(offsetof(__wasi_dirent_t, d_namlen) == 16, "non-wasi data layout");
 _Static_assert(offsetof(__wasi_dirent_t, d_type) == 20, "non-wasi data layout"); 
 _Static_assert(sizeof(__wasi_dirent_t) == 24, "non-wasi data layout");           
 _Static_assert(_Alignof(__wasi_dirent_t) == 8, "non-wasi data layout");  

@pchickey
Copy link
Contributor Author

@sbc100 I actually hadn't thought to do that, but its a great idea. I'll add those assertions to the wasi-libc code generator as part of my work to show that this code is actually correct.

@pchickey
Copy link
Contributor Author

I went and added those assertions back to wasi-libc and fixed bugs in the process.

Still working on adding some local tests to this repo that check whether the calculations are correct.

@pchickey pchickey marked this pull request as ready for review December 20, 2019 23:12
@pchickey
Copy link
Contributor Author

I added static assertions to the wasi-libc code generator to check if these calculations align with LLVM (WebAssembly/wasi-libc#149), and created a randomized witx spec generator to search for cases that the current wasi spec doesn't cover (https://github.com/pchickey/witx-test), and check those using the wasi-libc generator. That randomized search found a bug I had accidentally left in union size calculation (fixed in 3857728), and has increased my confidence that this code is correct.

I think we should try to move that sort of testing into this repo and run it as part of CI, but that is a bigger step than just adding this layout code and having some external evidence of its correctness. If that is enough for the rest of y'all, lets review and merge this PR as is.

I think the witx-test tool would be appropriate to have in this repo, or maybe even as a cargo-flagged feature in this crate. It does incur a dependency on proptest, which is pretty heavy weight compared to the existing dependencies.

I also think that, if we're going to move wasi-libc into the WebAssembly github org, that perhaps we should also move its wasi-header tool into the witx crate as well, since it should be useful to create a C header corresponding to any witx spec.

However, I don't want to make either of those questions part of this PR. So, I propose we merge this PR and then discuss those questions afterwards.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan sounds good; let's discuss those other steps separately. This is a really cool development!

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.

5 participants