Implement Garbage Collector type system#2607
Conversation
243ec44 to
68fe37e
Compare
| }; | ||
| using TypeMutVector = std::vector<TypeMut>; | ||
|
|
||
| // Garbage Collector specific type information |
There was a problem hiding this comment.
This is a core part of the patch. It contains the (sub ...) part of the type. It is declared as a structure, because it is not mandatory.
| virtual Result OnTypeCount(Index count) = 0; | ||
| virtual Result OnRecursiveRange(Index start_index, Index type_count) = 0; | ||
| virtual Result OnFuncType(Index index, | ||
| GCTypeExtension* gc_ext, |
There was a problem hiding this comment.
This structure is passed as a second argument, because it is a header, but it could go to the last argument since it is an extra (and optional) information. Which one you prefer?
| struct RecursiveRange { | ||
| Index start_index; | ||
| Index type_count; | ||
| }; |
There was a problem hiding this comment.
This is another core structure to encode (rec ...) constructs. It represents the range.
| std::vector<FuncType> func_types; | ||
| std::vector<StructType> struct_types; | ||
| std::vector<ArrayType> array_types; | ||
| std::vector<RecursiveRange> recursive_ranges; |
There was a problem hiding this comment.
It is stored in an ordered array. It cannot be stored as part of the types, because zero length (rec) range is allowed for whatever reason.
e0ce7f8 to
adcbdf7
Compare
|
I have reworked the type validation system of the patch. Now it is capable of detecting the first type index for all equal types. This first type index is called canonical index. If I have two types ( |
75bdfe5 to
890a316
Compare
|
The |
|
It sounds like you are canonicalising wrt type indices. But type indices are meaningless in any program that consists of more than one module. Type canonicalisation must happen globally, across module boundaries, based on the types' structure. I suspect that is the reason for the link/run-time tests failing. |
|
The link tests are not failing, although the interpreter do a slow type comparison for import/exports. As far as I understand the interpreter here is just a demonstration, so this is probably ok. The runtime tests fail because the operations (such as The global type canonicalisation sounds like a very good idea! A high performance engine should do that! |
cc8e21f to
097046d
Compare
|
@sbc100 there is a fuzzer issue in the code. The code is correct though. https://github.com/WebAssembly/wabt/blob/main/src/interp/binary-reader-interp.cc#L772 https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/fuzzer/FuzzerLoop.cpp What shall I do? |
We don't tend to have time to worry about fixing all the fuzz tests issues, unless they could conceivable show up in real world programs. i.e. we tend to assume trusted and save inputs, since we don't have the resources the harden wabt against other things. Having said that we obviously would be happy to accept fixes for such issues if folks come up with them. |
|
There is nothing to fix here, the code is correct (and not related to this patch). It is simply a limitation of the fuzzer, it assumes too much memory allocation is likely a bug. |
68c749c to
b7fc45c
Compare
| Result OnStructType(Index index, | ||
| Index field_count, | ||
| TypeMut* fields) override { | ||
| TypeMut* fields, |
There was a problem hiding this comment.
I wonder if we should be using C++ std::array rather then size + ptr in these API ? But I see its a pre-existing thing so no worries for this PR.
031fa07 to
9eeb962
Compare
|
@sbc100 may I ask whether you have time to review this patch? |
sbc100
left a comment
There was a problem hiding this comment.
There is a lot of code here. I've not yet had a chance to look at all of it but LGTM so far.
| : ExternType(ExternKind::Func), | ||
| params(params), | ||
| results(results), | ||
| func_types(nullptr) {} |
There was a problem hiding this comment.
This line looks like it did not change? Unless I'm missing something? Maybe revert this line?
There was a problem hiding this comment.
The line is longer than 80 columns, it is surprising for me that it is not a style error.
There was a problem hiding this comment.
Hmm.. i wonder if we should clang-format the codebase as a separate PR?
There was a problem hiding this comment.
I have no objection.
| }; | ||
|
|
||
| // To simplify the implementation, FuncType may also represent | ||
| // Struct and Array types. In the latter case, the mutability |
There was a problem hiding this comment.
Can you explain a little more? Why would FuncType represent struct or array?
There was a problem hiding this comment.
FuncType is a type, which is used in a lot of code in the interpreter. When I worked on this patch, I wanted to avoid adding a lot of new code to the interpreter (the patch is large enough), which primary purpose is just demonstration as far as I understood. This could be reworked later if we want the extra code. An option is just renaming FuncType to something generic.
There was a problem hiding this comment.
OK, maybe add a TODO to refactor or rename it?
|
|
||
| Result BinaryReaderInterp::OnFunctionCount(Index count) { | ||
| module_.funcs.reserve(count); | ||
| module_.funcs.reserve(std::min(count, 1024u)); |
There was a problem hiding this comment.
Why is this relevant to this change?
Surely this list will need to grow to count elements anyway? Why not allocate it all here?
8325b85 to
475c983
Compare
|
Thank you for the review. I have updated the patch. |
|
I have updated this patch. If there are follow-up works I should do (e.g changing the param/result types for functions from pointer to vector reference), please open issues for them and assign me. It is easy to forget these things. |
e1d4797 to
51a42dc
Compare
|
I made some small changes. I hope this patch is in good shape now. I will fix issues related to this patch. |
269171f to
9045213
Compare
6407976 to
b5c0c43
Compare
06132cb to
36a825a
Compare
22118e5 to
6395963
Compare
This patch supports parsing the new GC types - Abstract types - Recursive types - Composite types The patch also improves type comparison.
|
The patch is updated with |
This patch supports parsing the new GC types
The patch also improves type comparison.