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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

This will also need someone from the LLVM side to look at the LLVM-specific changes. Most of my comments were focused on the behavior of test cases, but there may be more comments coming for the code changes once I've got a better handle on the test behavior.



================
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")
----------------
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).


================
Comment at: clang/test/Sema/builtins-wasm.c:12-13
+  __builtin_wasm_table_size(table, table);            // expected-error {{too many arguments to function call, expected 1, have 2}}
+  void *a = __builtin_wasm_table_size(table);         // expected-error {{incompatible integer to pointer conversion initializing 'void *' with an expression of type 'int'}}
+  __externref_t b = __builtin_wasm_table_size(table); // expected-error {{initializing '__externref_t' with an expression of incompatible type 'int'}}
+  int c = __builtin_wasm_table_size(table);
----------------
Instead of relying on assignment diagnostics to test the return type of the call, why not use the language? (Same can be used for the other, similar tests.)
```
#define EXPR_HAS_TYPE(expr, type) _Generic((expr), type : 1, default : 0)

_Static_assert(EXPR_HAS_TYPE(__builtin_wasm_table_size(table), int), "");
```


================
Comment at: clang/test/Sema/builtins-wasm.c:21
+  __builtin_wasm_table_grow(ref, ref, size);             // expected-error {{1st argument must be a WebAssembly table}}
+  __builtin_wasm_table_grow(table, table, size);         // expected-error {{2nd argument must match the element type of the WebAssembly table in the 1st argument}}
+  __builtin_wasm_table_grow(table, ref, table);          // expected-error {{3rd argument must be an integer}}
----------------
I can't make sense of this diagnostic -- what is the element type of the web assembly table in the 1st argument? Why is the second argument needed at all if it needs to be derived from the type of the first argument and these things don't represent values (due to being zero-sized)?


================
Comment at: clang/test/Sema/builtins-wasm.c:32
+  __builtin_wasm_table_fill(table, table, ref, nelem);           // expected-error {{2nd argument must be an integer}}
+  __builtin_wasm_table_fill(table, index, index, ref);           // expected-error {{3rd argument must match the element type of the WebAssembly table in the 1st argument}}
+  __builtin_wasm_table_fill(table, index, ref, table);           // expected-error {{4th argument must be an integer}}
----------------
Similar question here about the third argument.


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:17
+static __externref_t t6[] = {0}; // expected-error {{only zero-length WebAssembly tables are currently supported}}
+__externref_t t7[0];             // expected-error {{WebAssembly table must be static}}
+static __externref_t t8[0][0];   // expected-error {{multi-dimensional arrays of WebAssembly references are not allowed}}
----------------
So why is `extern __externref_t r2;` allowed? Is it because it's not an array declaration?


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:21-23
+static __externref_t table[0];
+static __externref_t other_table[0] = {};
+
----------------
For completeness (should just work)


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:48
+void illegal_argument_4(__externref_t ***table);    // expected-error {{pointer to WebAssembly reference type is not allowed}}
+void illegal_argument_5(__externref_t (*table)[0]); // expected-error {{cannot form a pointer to a WebAssembly table}}
+
----------------
How about:
```
void foo(__externref_t table[0]);
```
I'd expect this to also not be allowed (that's just a fancy spelling for a weird pointer).


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:81
+
+  funcref_t func = __builtin_wasm_ref_null_func(0); // expected-error {{too many arguments to function call, expected 0, have 1}}
+
----------------
Shouldn't this be in builtins-wasm.c instead?


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:83-84
+
+  __externref_t lt1[0];           // expected-error {{WebAssembly table cannot be declared within a function}}
+  static __externref_t lt2[0];    // expected-error {{WebAssembly table cannot be declared within a function}}
+  static __externref_t lt3[0][0]; // expected-error {{multi-dimensional arrays of WebAssembly references are not allowed}}
----------------
This needs some further test cases for situations where the declaration isn't at the top level of the function. e.g.,
```
void foo() {
  static __externref_t t[0]; // error
  {
    static __externref_t t2[0]; // error
    for (;;) {
      static __externref_t t3[0]; // error
    }
  }
  int i = ({ static __externref_t t4[0]; /* error, I presume? */ 1;});
}
```
and also a C++ test involving lambdas (we should probably also cover blocks in C, I suppose).


================
Comment at: clang/test/Sema/wasm-refs-and-tables.c:104
+  table[0] = ref;                 // expected-error {{cannot subscript a WebAssembly table}}
+
+  return ref;
----------------
Please don't hate me, but... what about:
```
int i = 0;
__externref_t oh_no_vlas[i];
```



================
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).
+
----------------
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.


================
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}}
----------------
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.


================
Comment at: clang/test/SemaCXX/wasm-refs-and-tables.cpp:118
+
+  return table; // expected-error {{cannot return a WebAssembly table}}
+}
----------------
We should probably have a test for `co_return` behavior as well?


================
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}}
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139010