[PATCH] D139010: [clang][WebAssembly] Implement support for table types and builtins

[Paulo Matos via Phabricator via cfe-commits]

pmatos marked 2 inline comments as done.
pmatos added inline comments.


================
Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:205-210
+TARGET_BUILTIN(__builtin_wasm_table_set,  "viii", "t", "reference-types")
+TARGET_BUILTIN(__builtin_wasm_table_get,  "iii", "t", "reference-types")
+TARGET_BUILTIN(__builtin_wasm_table_size, "ii", "nt", "reference-types")
+TARGET_BUILTIN(__builtin_wasm_table_grow, "iiii", "nt", "reference-types")
+TARGET_BUILTIN(__builtin_wasm_table_fill, "viiii", "t", "reference-types")
+TARGET_BUILTIN(__builtin_wasm_table_copy, "viiiii", "t", "reference-types")
----------------
aaron.ballman wrote:
> All of these should be documented in `docs/LanguageExtensions.rst` (you can make a Web Assembly section if one doesn't already exist; we've historically been bad about documenting our builtins).
Working on this - thanks for the reminder.


================
Comment at: clang/test/SemaCXX/wasm-refs-and-tables.cpp:6-7
+
+// Using c++11 to test dynamic exception specifications (which are not 
+// allowed in c++17).
+
----------------
aaron.ballman wrote:
> Much of this file is the same test coverage as in the C case; I'd recommend combining the .c and .cpp files into one test with two RUN lines, and use `-verify=whatever` to distinguish between C vs C++ vs both diagnostic behaviors. The C++ specific code can then be split into a much smaller .cpp-specific file.
I have don't this know. I was not aware we could pass an argument to verify - thanks.


================
Comment at: clang/test/SemaCXX/wasm-refs-and-tables.cpp:14
+
+__externref_t *t1;               // expected-error {{pointer to WebAssembly reference type is not allowed}}
+__externref_t **t2;              // expected-error {{pointer to WebAssembly reference type is not allowed}}
----------------
aaron.ballman wrote:
> Anywhere you'd testing pointer behavior for C++, you should have a test with an lvalue reference as well. I presume those will behave the same as pointers? It'd probably be wise to have tests for rvalue references as well.
Here I guess you're talking about testing __externref_t &. However, this might be outside the scope of the patch which deals only with tables.

I could add further lvalue and rvalue reference tests to externref and funcref in another patch. What do you think?


================
Comment at: clang/test/SemaCXX/wasm-refs-and-tables.cpp:16-17
+__externref_t **t2;              // expected-error {{pointer to WebAssembly reference type is not allowed}}
+__externref_t ******t3;          // expected-error {{pointer to WebAssembly reference type is not allowed}}
+static __externref_t t4[3];      // expected-error {{only zero-length WebAssembly tables are currently supported}}
+static __externref_t t5[];       // expected-error {{only zero-length WebAssembly tables are currently supported}}
----------------
aaron.ballman wrote:
> pmatos wrote:
> > aaron.ballman wrote:
> > > This seems really... confused. We can't form a pointer to the type, but we can form an array of the type (which decays into a pointer when you sneeze near it, and it's not clear whether that should be allowed or not) so long as it's a zero-length array (which is an extension in C and C++, so do we need to silence pedantic warnings in WASM for this?).
> > As it stands, tables are declared as static arrays of size zero. Then to access them we need to use builtins. No subscripting, no comparison, no pointer decay, etc. No passing into functions, returning from functions, etc. Nothing besides using them as arguments to wasm_table... builtins.
> Okay, that's less confused now, thank you! What should the `-pedantic` behavior be for this:
> ```
> static __externref_t table[0];
> ```
> I presume you don't want the usual "zero size arrays are an extension" warning?
I have fixed this but unsure how to best merge it with the remainder of the tests, so created a new test file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139010/new/

https://reviews.llvm.org/D139010