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

[asb]

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.

[asb]

Addresses some potential null pointer dereferences.

[asb]

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

[rjmccall]

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]

clang/test/CodeGen/WebAssembly/table.c

6

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

15

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.

16

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

17

Why is this?

44–45

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

89

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

[asb]

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

6

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

15

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.

16

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

17

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.

44–45

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

89

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

[asb]

Re-add full context.

[tlively]

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

6

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

17

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]

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

[asb]

In D123510#3465123, @rjmccall wrote:

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]

Added semantic restrictions for C++ specific constructs.

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

[asb]

Add a isWebAssemblyTable helper and refactor to use it.

[asb]

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]

Rebase on top of D128440

[asb]

Rebase again.

[pmatos]

@asb This can _also_ be closed since table support landed.

[asb]

Abandoning, as this was ultimately merged into D139010 which has now landed.