Page MenuHomePhabricator

[clang][WebAssembly] Implement support for table types and appropriate semantic restrictions
Needs ReviewPublic

Authored by asb on Apr 11 2022, 6:40 AM.

Details

Summary

This patch isn't yet ready for merging, but I think is ready for high-level review. Note that it assumes something like this has been merged into D122215.

Todo:

  • funcref support and test coverage

WebAssembly tables are modeled at the IR level as arrays in address space 1. As the instructions accessing tables take the table index as an immediate, use of tables is very restricted (i.e. it isn't possible to pass as an argument etc). This patch models tables in C as arrays of reference types and adds various semantic restrictions to avoid illegal uses. Some of these restrictions overlap with those for sizeless types, while of course the limitations on passing in parameters and so on go above and beyond that.

I'd really appreciate any high-level feedback / questions / concerns about this approach at this point.

Diff Detail

Event Timeline

asb created this revision.Apr 11 2022, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 6:40 AM
asb requested review of this revision.Apr 11 2022, 6:40 AM
asb edited the summary of this revision. (Show Details)Apr 11 2022, 6:42 AM
asb updated this revision to Diff 422779.Apr 14 2022, 2:14 AM

Addresses some potential null pointer dereferences.

asb updated this revision to Diff 424164.Apr 21 2022, 5:39 AM
asb retitled this revision from [clang][WebAssembly][WIP] Implement support for table types and appropriate semantic restrictions to [clang][WebAssembly] Implement support for table types and appropriate semantic restrictions.
asb edited the summary of this revision. (Show Details)
asb added reviewers: rjmccall, tlively, sbc100, pmatos.

Update to latest version and update summary. As I say there, it would be really helpful to get some feedback at this point.

Sounds like you need an RFC thread on the forums describing the underlying WebAssembly feature and your proposed language design for exposing it in C.

tlively added inline comments.Apr 21 2022, 4:25 PM
clang/test/CodeGen/WebAssembly/table.c
5 ↗(On Diff #424164)

Why must tables be declared with 0 size? I would expect declaring any concrete size to work, since the underlying module can declare tables of any size. Also, how will all this work with table growth? Is there some danger of static analysis thinking that a table has an out-of-bound access when in fact it has been grown beyond its original size? Would declaring a sizeless table help there?

clang/test/Sema/wasm-refs-and-tables.c
10

This error message is confusing because it's not really the element type that is the problem, but rather the lack of size declaration on the array itself. Also, as a matter of UX, it would be nice if we could avoid printing __attribute__((address_space(1))) as part of the externref type.

11

Is this a temporary restriction? The underlying Wasm format supports initializers.

12

Why is this?

35

Are we going to be able to work around this to provide a builtin for table.grow?

75

Does this need a separate error message since it isn't a binary expression?

asb updated this revision to Diff 424467.Apr 22 2022, 7:20 AM

Update to address review comments.

@rjmccall: That makes sense - I'll write up an RFC for this. Thanks for taking a look.

clang/test/CodeGen/WebAssembly/table.c
5 ↗(On Diff #424164)

That's a limitation in this initial support for tables but needn't be a limitation going forward. We don't currently support generating table initialisers, but I think tables can still be useful without that (so that functionality can be split out to some future patch).

Regarding static analysis: potentially a static analyser will complain until it's taught to ignore tables. Using something other than an array type (either just a pointer or new sizeless type) is definitely an alternative, but in the case of a pointer there's not good story for adding syntax to support initialisation. Introducing a new sizeless type would need a story on indicating the type of the table's contents. Modelling as an array at least gives us those things (and array-like syntax for getting/setting elements, though I'm not sure that's a big factor one way or another). Which all to say I'm totally open to alternatives, but when I started on this I'd understood that arrays were the current preferred approach and when I considered those other options, it didn't feel like the others were clearly better (just different trade-offs!).

clang/test/Sema/wasm-refs-and-tables.c
10

This error message is inherited from the code to support sizeless types. But it's easy enough to cover this case alongside the one for non-zero size, so I've done that. That also covers the initialisation case for the time being.

11

Yes that's right, I've changed the message now to be clearer this is a current restriction rather than a fundamental one.

12

I understood that's what had been agreed as the initial way to expose them, and going beyond that, I don't think there's a clear story on sharing tables between compilation units. So I'd again put this down to the "MVP"-ness of the patch.

35

Yep, D124162 provides the builtins that don't use element segments. Using custom type-checking for those builtins neatly sidesteps this being an issue.

75

Good point, that would be more precise. I've added one now.

asb updated this revision to Diff 424469.Apr 22 2022, 7:28 AM

Re-add full context.

Nice! code looks good to me, but we'll need community signoff on an RFC as @rjmccall mentioned and it would be nice to have a clang maintainer sign off as well.

clang/test/CodeGen/WebAssembly/table.c
5 ↗(On Diff #424164)

By "sizeless table" I meant using an array type without a size (not sure if there's a more precise term for that), so something like static __externref_t table[]. I wonder if that would help avoid false positives from static analyzers because they wouldn't be able to reason about sizes.

But starting out with many restrictions and incrementally relaxing them as necessary sounds good 👍

clang/test/Sema/wasm-refs-and-tables.c
12

Sounds good for the MVP. I expect that it will be useful to share tables across compilation units in the future, and I think we might even have support for the necessary table relocations in the backend and linker thanks to @pmatos's work.

asb edited the summary of this revision. (Show Details)Apr 25 2022, 3:00 AM
asb updated this revision to Diff 424863.Apr 25 2022, 3:59 AM

Rebase, and fix+test missed error case (taking the address of table).

asb added a comment.Apr 26 2022, 8:05 AM

Sounds like you need an RFC thread on the forums describing the underlying WebAssembly feature and your proposed language design for exposing it in C.

Now posted as https://discourse.llvm.org/t/rfc-webassembly-tables-in-clang/62049

asb updated this revision to Diff 429721.EditedMay 16 2022, 8:13 AM
asb edited the summary of this revision. (Show Details)
asb edited the summary of this revision. (Show Details)

Added semantic restrictions for C++ specific constructs.

It would likely make sense to introduce a isWebAssemblyTable method and refacctor to use that.

asb updated this revision to Diff 430039.May 17 2022, 6:51 AM

Add a isWebAssemblyTable helper and refactor to use it.

asb updated this revision to Diff 446069.Jul 20 2022, 1:17 AM

Rebase (partial - rebases my patch stack on top of latest LLVM, but still needs a rebase on to Paulo's patch introducing funcref support).

asb updated this revision to Diff 446164.Jul 20 2022, 8:13 AM

Rebase on top of D128440

asb updated this revision to Diff 446500.Jul 21 2022, 7:48 AM

Rebase again.

koplas added a subscriber: koplas.Wed, Nov 9, 1:13 AM