-
Notifications
You must be signed in to change notification settings - Fork 746
Add support for function references proposal #2562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,10 @@ struct FuncType : ExternType { | |
|
||
ValueTypes params; | ||
ValueTypes results; | ||
// When params or results contain references, the referenced | ||
// types are also needed for type equality comparisons. | ||
// An example for these comparisons is import validation. | ||
std::vector<FuncType>* func_types; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this field used for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed for supporting a complex requirement: We need to match the import/export references across modules. To do that, the interpreter needs to see the whole type tree (where the references are expanded to other types). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment about it. This is a non-trivial connection. |
||
}; | ||
|
||
struct TableType : ExternType { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,24 +36,43 @@ namespace wabt { | |
|
||
struct Module; | ||
|
||
enum class VarType { | ||
// VarType (16 bit) and the opt_type_ (16 bit) | ||
// fields of Var forms a 32 bit field. | ||
enum class VarType : uint16_t { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not uint8_t? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I simply split the 32 bit (including padding) field to two 16 bit fields. I could also use bitfields, but I think 16 bit is more than enough for WebAssembly types. Shall I change that to 8 bit (this way another 8 bit padding will be added by the compiler)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, perhaps just add a comment here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment added |
||
Index, | ||
Name, | ||
}; | ||
|
||
struct Var { | ||
// Var can represent variables or types. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a major internal change. It does not increase the size of Var. I think it is better than constructing a Var/Type pair. |
||
|
||
// Represent a variable: | ||
// has_opt_type() is false | ||
// Only used by wast-parser | ||
|
||
// Represent a type: | ||
// has_opt_type() is true, is_index() is true | ||
// type can be get by to_type() | ||
// Binary reader only constructs this variant | ||
|
||
// Represent both a variable and a type: | ||
// has_opt_type() is true, is_name() is true | ||
// A reference, which index is unknown | ||
// Only used by wast-parser | ||
|
||
explicit Var(); | ||
explicit Var(Index index, const Location& loc); | ||
explicit Var(std::string_view name, const Location& loc); | ||
explicit Var(Type type, const Location& loc); | ||
Var(Var&&); | ||
Var(const Var&); | ||
Var& operator=(const Var&); | ||
Var& operator=(Var&&); | ||
~Var(); | ||
|
||
VarType type() const { return type_; } | ||
bool is_index() const { return type_ == VarType::Index; } | ||
bool is_name() const { return type_ == VarType::Name; } | ||
bool has_opt_type() const { return opt_type_ < 0; } | ||
|
||
Index index() const { | ||
assert(is_index()); | ||
|
@@ -63,17 +82,25 @@ struct Var { | |
assert(is_name()); | ||
return name_; | ||
} | ||
Type::Enum opt_type() const { | ||
assert(has_opt_type()); | ||
return static_cast<Type::Enum>(opt_type_); | ||
} | ||
|
||
void set_index(Index); | ||
void set_name(std::string&&); | ||
void set_name(std::string_view); | ||
void set_opt_type(Type::Enum); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. For most Vars (e.g. names of WebAssembly constructs) there is no need for any type. |
||
Type to_type() const; | ||
|
||
Location loc; | ||
|
||
private: | ||
void Destroy(); | ||
|
||
VarType type_; | ||
// Can be set to Type::Enum types, Type::Any represent no optional type. | ||
int16_t opt_type_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This range should be enough for a long time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some reason we can't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternatively if you do need a third type for Var why not add another element to VarType and add another element to the union below? Sorry, I think I must be misunderstanding something here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For indexed references, you need both a reference type (nullable / non-nullable) and an index or name (only for the text parser). The index or name is stored in the union. For non indexed types the type could go to the union. Another alternative could be:
But handling this seems far more complex than the current one, and we have more than enough bits (VarType is aligned to 32 bit, because the union is aligned to 32 bit) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the main part of this PR that I'm still finding confusing. @tlively WDYT about this part? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
union { | ||
Index index_; | ||
std::string name_; | ||
|
@@ -155,6 +182,7 @@ struct Const { | |
} | ||
void set_funcref() { From<uintptr_t>(Type::FuncRef, 0); } | ||
void set_externref(uintptr_t x) { From(Type::ExternRef, x); } | ||
void set_extern(uintptr_t x) { From(Type(Type::ExternRef, Type::ReferenceNonNull), x); } | ||
void set_null(Type type) { From<uintptr_t>(type, kRefNullBits); } | ||
|
||
bool is_expected_nan(int lane = 0) const { | ||
|
@@ -537,10 +565,10 @@ using MemoryCopyExpr = MemoryBinaryExpr<ExprType::MemoryCopy>; | |
template <ExprType TypeEnum> | ||
class RefTypeExpr : public ExprMixin<TypeEnum> { | ||
public: | ||
RefTypeExpr(Type type, const Location& loc = Location()) | ||
RefTypeExpr(Var type, const Location& loc = Location()) | ||
: ExprMixin<TypeEnum>(loc), type(type) {} | ||
|
||
Type type; | ||
Var type; | ||
}; | ||
|
||
using RefNullExpr = RefTypeExpr<ExprType::RefNull>; | ||
|
@@ -662,8 +690,8 @@ using MemoryInitExpr = MemoryVarExpr<ExprType::MemoryInit>; | |
|
||
class SelectExpr : public ExprMixin<ExprType::Select> { | ||
public: | ||
SelectExpr(TypeVector type, const Location& loc = Location()) | ||
: ExprMixin<ExprType::Select>(loc), result_type(type) {} | ||
SelectExpr(const Location& loc = Location()) | ||
: ExprMixin<ExprType::Select>(loc) {} | ||
TypeVector result_type; | ||
}; | ||
|
||
|
@@ -727,9 +755,7 @@ class CallRefExpr : public ExprMixin<ExprType::CallRef> { | |
explicit CallRefExpr(const Location& loc = Location()) | ||
: ExprMixin<ExprType::CallRef>(loc) {} | ||
|
||
// This field is setup only during Validate phase, | ||
// so keep that in mind when you use it. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment no longer valid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Call ref needs to explicitly specify the type, auto detection is not allowed anymore: |
||
Var function_type_index; | ||
Var sig_type; | ||
}; | ||
|
||
template <ExprType TypeEnum> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ struct ValidateOptions { | |
class SharedValidator { | ||
public: | ||
WABT_DISALLOW_COPY_AND_ASSIGN(SharedValidator); | ||
using FuncType = TypeChecker::FuncType; | ||
SharedValidator(Errors*, const ValidateOptions& options); | ||
|
||
// TODO: Move into SharedValidator? | ||
|
@@ -72,7 +73,6 @@ class SharedValidator { | |
Index type_index); | ||
Result OnStructType(const Location&, Index field_count, TypeMut* fields); | ||
Result OnArrayType(const Location&, TypeMut field); | ||
Result EndTypeSection(); | ||
|
||
Result OnFunction(const Location&, Var sig_var); | ||
Result OnTable(const Location&, Type elem_type, const Limits&); | ||
|
@@ -141,7 +141,7 @@ class SharedValidator { | |
Result EndBrTable(const Location&); | ||
Result OnCall(const Location&, Var func_var); | ||
Result OnCallIndirect(const Location&, Var sig_var, Var table_var); | ||
Result OnCallRef(const Location&, Index* function_type_index); | ||
Result OnCallRef(const Location&, Var function_type_var); | ||
Result OnCatch(const Location&, Var tag_var, bool is_catch_all); | ||
Result OnCompare(const Location&, Opcode); | ||
Result OnConst(const Location&, Type); | ||
|
@@ -178,7 +178,7 @@ class SharedValidator { | |
Result OnNop(const Location&); | ||
Result OnRefFunc(const Location&, Var func_var); | ||
Result OnRefIsNull(const Location&); | ||
Result OnRefNull(const Location&, Type type); | ||
Result OnRefNull(const Location&, Var func_type_var); | ||
Result OnRethrow(const Location&, Var depth); | ||
Result OnReturnCall(const Location&, Var func_var); | ||
Result OnReturnCallIndirect(const Location&, Var sig_var, Var table_var); | ||
|
@@ -221,18 +221,6 @@ class SharedValidator { | |
Result OnUnreachable(const Location&); | ||
|
||
private: | ||
struct FuncType { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is moved to type checker. |
||
FuncType() = default; | ||
FuncType(const TypeVector& params, | ||
const TypeVector& results, | ||
Index type_index) | ||
: params(params), results(results), type_index(type_index) {} | ||
|
||
TypeVector params; | ||
TypeVector results; | ||
Index type_index; | ||
}; | ||
|
||
struct StructType { | ||
StructType() = default; | ||
StructType(const TypeMutVector& fields) : fields(fields) {} | ||
|
@@ -289,6 +277,11 @@ class SharedValidator { | |
Index end; | ||
}; | ||
|
||
struct LocalReferenceMap { | ||
Type type; | ||
Index bit_index; | ||
}; | ||
|
||
bool ValidInitOpcode(Opcode opcode) const; | ||
Result CheckInstr(Opcode opcode, const Location& loc); | ||
Result CheckType(const Location&, | ||
|
@@ -336,6 +329,10 @@ class SharedValidator { | |
|
||
TypeVector ToTypeVector(Index count, const Type* types); | ||
|
||
void SaveLocalRefs(); | ||
void RestoreLocalRefs(Result result); | ||
void IgnoreLocalRefs(); | ||
|
||
ValidateOptions options_; | ||
Errors* errors_; | ||
TypeChecker typechecker_; // TODO: Move into SharedValidator. | ||
|
@@ -361,6 +358,8 @@ class SharedValidator { | |
// Includes parameters, since this is only used for validating | ||
// local.{get,set,tee} instructions. | ||
std::vector<LocalDecl> locals_; | ||
std::map<Index, LocalReferenceMap> local_refs_map_; | ||
std::vector<bool> local_ref_is_set_; | ||
|
||
std::set<std::string> export_names_; // Used to check for duplicates. | ||
std::set<Index> declared_funcs_; // TODO: optimize? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
#include <functional> | ||
#include <type_traits> | ||
#include <vector> | ||
#include <map> | ||
|
||
#include "wabt/common.h" | ||
#include "wabt/feature.h" | ||
|
@@ -31,6 +32,18 @@ class TypeChecker { | |
public: | ||
using ErrorCallback = std::function<void(const char* msg)>; | ||
|
||
struct FuncType { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is something that could be improved. We could create a separate comparison class, which contains only the |
||
FuncType() = default; | ||
FuncType(const TypeVector& params, | ||
const TypeVector& results, | ||
Index type_index) | ||
: params(params), results(results), type_index(type_index) {} | ||
|
||
TypeVector params; | ||
TypeVector results; | ||
Index type_index; | ||
}; | ||
|
||
struct Label { | ||
Label(LabelType, | ||
const TypeVector& param_types, | ||
|
@@ -46,9 +59,11 @@ class TypeChecker { | |
TypeVector result_types; | ||
size_t type_stack_limit; | ||
bool unreachable; | ||
std::vector<bool> local_ref_is_set_; | ||
}; | ||
|
||
explicit TypeChecker(const Features& features) : features_(features) {} | ||
explicit TypeChecker(const Features& features, std::map<Index, FuncType>& func_types) | ||
: features_(features), func_types_(func_types) {} | ||
|
||
void set_error_callback(const ErrorCallback& error_callback) { | ||
error_callback_ = error_callback; | ||
|
@@ -80,7 +95,7 @@ class TypeChecker { | |
Result OnCallIndirect(const TypeVector& param_types, | ||
const TypeVector& result_types, | ||
const Limits& table_limits); | ||
Result OnIndexedFuncRef(Index* out_index); | ||
Result OnCallRef(Type); | ||
Result OnReturnCall(const TypeVector& param_types, | ||
const TypeVector& result_types); | ||
Result OnReturnCallIndirect(const TypeVector& param_types, | ||
|
@@ -115,7 +130,7 @@ class TypeChecker { | |
Result OnTableGrow(Type elem_type, const Limits& limits); | ||
Result OnTableSize(const Limits& limits); | ||
Result OnTableFill(Type elem_type, const Limits& limits); | ||
Result OnRefFuncExpr(Index func_type, bool force_generic_funcref); | ||
Result OnRefFuncExpr(Index func_type); | ||
Result OnRefNullExpr(Type type); | ||
Result OnRefIsNullExpr(); | ||
Result OnRethrow(Index depth); | ||
|
@@ -141,7 +156,7 @@ class TypeChecker { | |
Result BeginInitExpr(Type type); | ||
Result EndInitExpr(); | ||
|
||
static Result CheckType(Type actual, Type expected); | ||
Result CheckType(Type actual, Type expected); | ||
|
||
private: | ||
void WABT_PRINTF_FORMAT(2, 3) PrintError(const char* fmt, ...); | ||
|
@@ -210,6 +225,7 @@ class TypeChecker { | |
// to represent "any". | ||
TypeVector* br_table_sig_ = nullptr; | ||
Features features_; | ||
std::map<Index, FuncType>& func_types_; | ||
}; | ||
|
||
} // namespace wabt | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think type sounds better than Index, although it could be an Index.