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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
