Fix reference type processing #2571

zherczeg · 2025-03-26T09:51:45Z

Parse (ref index) form, not just (ref $name) form
Resolve references in types and locals
Display location when a named reference is not found

zherczeg · 2025-03-26T09:54:06Z

This is another bugfix patch extracted from #2562. The list of changes are above, these constructs should work even in the main branch.

zherczeg · 2025-03-26T09:56:04Z

src/wast-parser.cc

@@ -973,11 +1008,16 @@ Result WastParser::ParseValueTypeList(
    CHECK_RESULT(ParseValueType(&type));

    if (type.is_index()) {
-      out_type_list->push_back(Type(type.index()));
+      // TODO: Temporary fix.


The full fix is in #2562. It is unlikely that close to UINT32_MAX number of types are defined, so this is good as a temporary change.

zherczeg · 2025-03-26T09:57:55Z

src/wast-parser.cc

+                                  func->GetNumParams(),
+                                  decl->sig.param_type_names, errors);
+
+        func->local_types.Set(func->local_type_list);


This is a delayed processing of locals. The reasion is that otherwise we cannot tell that (ref $t) and (ref 0) refers to the same or different locals, so they can be merged or not.

zherczeg · 2025-03-26T09:59:07Z

src/ir.cc

@@ -181,7 +181,7 @@ void LocalTypes::Set(const TypeVector& types) {
  Type type = types[0];
  Index count = 1;
  for (Index i = 1; i < types.size(); ++i) {
-    if (types[i] != type) {
+    if (!types[i].IsSame(type)) {


The name might not be the best. Anyway, the purpose is that == only compares the enum, while the index is also important for references.

sbc100

Thanks for working on this.

Mostly LGTM, just trying to page in how this stuff all works.

sbc100 · 2025-03-26T15:55:25Z

include/wabt/ir.h

@@ -259,8 +259,8 @@ struct FuncSignature {
  // So to use this type we need to translate its name into
  // a proper index from the module type section.
  // This is the mapping from parameter/result index to its name.


Comment needs updating maybe?

sbc100 · 2025-03-26T15:56:59Z

include/wabt/type.h

@@ -79,6 +79,11 @@ class Type {
    return IsRef();
  }

+  // The == comparison only compares the enum_ member.
+  bool IsSame(const Type& rhs) const {


Should we instead update operator==?

Unfortunately I get errors such as error: ambiguous overload for ‘operator==’ (operand types are ‘const wabt::Type’ and ‘wabt::Type::Enum’)

The reason is the Type has a constexpr operator Enum() const { return enum_; }, and this confuses the compiler.

I am open to any ideas.

how much would break if you made that explicit...

This seems to work for me locally:

constexpr operator Enum() const { return enum_; } + // TODO(sbc) Use =default here once we have C++20 + bool operator==(const Type& other) const { + return enum_ == other.enum_ && type_index_ == other.type_index_; + } + + bool operator==(const Enum& other) const { return enum_ == other; }

I tried the explicit route too and that seems to break a bunch of places.

Ok, I change it. Thank you

Btw it might fix another bug. Currently if we have two types which are the same except the ref part:

(type $t0 (func (param (ref $t0)))) (type $t1 (func (param (ref $t1)))) (func $f (param (ref $t1)) )

$f gets type 0, instead of type 1. The index is simply ignored.

It seems it does not work :(

if (types[i] != type) { still uses the enum auto conversion.
if (!types[i].operator==(type)) { works but does not look nice.

What shall I do?

I guess removing the implicit cast is the way to go.. although as you say it might require a bunch of downstream changes.

I tried to move the operator== before the Enum casting, and the auto casting still takes precedence. I will never understand c++.

Removing the casting might have many side effects which is too big risk for this patch, so I move back to the original code for now, and we can try it later.

sbc100 · 2025-03-26T15:58:18Z

include/wabt/wast-parser.h

@@ -137,7 +137,8 @@ class WastParser {
  Result ParseValueType(Var* out_type);
  Result ParseValueTypeList(
      TypeVector* out_type_list,
-      std::unordered_map<uint32_t, std::string>* type_names);
+      std::unordered_map<uint32_t, Var>* type_names,


Maybe should be renamed? type_vars?

All ...type_names are renamed to ...type_vars.

sbc100 · 2025-03-26T16:09:44Z

test/typecheck/missing-ref.txt

+out/test/typecheck/missing-ref.txt:5:24: error: Unknown reference: $first_type
+  (func $f (param (ref $first_type))
+                       ^^^^^^^^^^^
+out/test/typecheck/missing-ref.txt:6:25: error: Unknown reference: $second_type


Looking at the errors that come from resolve-names.cc It looks like this should be undefined reference type variable \"%s\"".. or something like that?

I changed it to undefined reference type name %s

sbc100 · 2025-03-26T16:10:48Z

src/wast-parser.cc

@@ -457,7 +474,7 @@ class ResolveFuncTypesExprVisitorDelegate : public ExprVisitor::DelegateNop {
      : module_(module), errors_(errors) {}

  void ResolveBlockDeclaration(const Location& loc, BlockDeclaration* decl) {
-    ResolveTypeNames(*module_, decl);
+    ResolveTypeNames(*module_, decl->sig, errors_);


How is this different to the code in src/wast-parser.cc that does name resolving?

I am not sure I understand this question. ResolveTypeNames is reworked to support error throwing.

zherczeg · 2025-03-26T18:52:55Z

test/typecheck/missing-ref.txt

+out/test/typecheck/missing-ref.txt:5:24: error: undefined reference type name $first_type
+  (func $f (param (ref $first_type))
+                       ^^^^^^^^^^^
+out/test/typecheck/missing-ref.txt:6:25: error: undefined reference type name $second_type


Just a note: the error is thrown twice. The reason is that the references are resolved as func type and as a function signature. I don't think it is a problem, but we could fix it (could be later) by removing the items of param_type_vars / result_type_vars, if somebody does not like it.

zherczeg · 2025-03-27T05:14:48Z

Did some code improvements. The unordered_map is replaced by a vector, because it is a simpler structure. Furthermore, when a reference is resolved, its index is set to kInvalidIndex to avoid duplicated resolves.

Parse (ref index) form, not just (ref $name) form Resolve references in types and locals Display location when a named reference is not found

zherczeg commented Mar 26, 2025

View reviewed changes

zherczeg force-pushed the ref_fix branch from cad84dc to 96f2659 Compare March 26, 2025 09:59

sbc100 reviewed Mar 26, 2025

View reviewed changes

zherczeg force-pushed the ref_fix branch 2 times, most recently from 1207345 to a4e230c Compare March 26, 2025 18:43

zherczeg commented Mar 26, 2025

View reviewed changes

zherczeg force-pushed the ref_fix branch 3 times, most recently from 946e527 to 74078be Compare March 27, 2025 05:12

zherczeg force-pushed the ref_fix branch 3 times, most recently from d403868 to 3fe420e Compare March 27, 2025 05:25

Fix reference type processing

8e0ae5a

Parse (ref index) form, not just (ref $name) form Resolve references in types and locals Display location when a named reference is not found

zherczeg force-pushed the ref_fix branch from 3fe420e to 8e0ae5a Compare March 27, 2025 05:47

zherczeg mentioned this pull request Mar 27, 2025

Remove implicit Enum conversion of types #2572

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reference type processing #2571

Fix reference type processing #2571

zherczeg commented Mar 26, 2025

zherczeg commented Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg Mar 26, 2025

sbc100 left a comment

sbc100 Mar 26, 2025

sbc100 Mar 26, 2025

zherczeg Mar 26, 2025

SoniEx2 Mar 26, 2025

sbc100 Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg Mar 26, 2025

sbc100 Mar 26, 2025

zherczeg Mar 27, 2025

sbc100 Mar 26, 2025

zherczeg Mar 26, 2025

sbc100 Mar 26, 2025

zherczeg Mar 26, 2025 •

edited

Loading

sbc100 Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg Mar 26, 2025

zherczeg commented Mar 27, 2025

Fix reference type processing #2571

Are you sure you want to change the base?

Fix reference type processing #2571

Conversation

zherczeg commented Mar 26, 2025

zherczeg commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zherczeg commented Mar 27, 2025

zherczeg Mar 26, 2025 •

edited

Loading