Skip to content

GUFA: Fix string.encode #8812

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.

Sign up for GitHub

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

Merged
kripken merged 3 commits into from
Jun 8, 2026
+92 −6
Merged

GUFA: Fix string.encode #8812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c086cb8
fix
kripken Jun 8, 2026
336b698
fix
kripken Jun 8, 2026
3392f55
unatomic
kripken Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/ir/possible-contents.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,11 +1161,12 @@ struct InfoCollector
curr->ref, curr->index, curr->value, MemoryOrder::Unordered);
visitArraySet(set);
}
template<typename ArrayInit> void visitArrayInit(ArrayInit* curr) {

void handleArrayWrite(Expression* ref) {
// Check for both unreachability and a bottom type. In either case we have
// no work to do, and would error on an assertion below in finding the array
// type.
auto field = GCTypeUtils::getField(curr->ref->type);
auto field = GCTypeUtils::getField(ref->type);
if (!field) {
return;
}
Expand All @@ -1179,12 +1180,14 @@ struct InfoCollector
Builder builder(*getModule());
auto* get = builder.makeLocalGet(-1, valueType);
addRoot(get);
auto* set =
builder.makeArraySet(curr->ref, curr->index, get, MemoryOrder::Unordered);
// The index does not matter, as we do not track array indexes yet TODO
Expression* index = builder.makeNop();

@tlively tlively Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it would be easy to change some other code that would then trip over a none-typed index expression. Should we make this an i32.const 0 instead?

@kripken kripken Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 would be incorrect, though, which could lead to a subtle bug. a Nop doesn't even have the right type, so if this is actually used, it should error loudly.

@tlively tlively Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm sympathetic to failing quickly and loudly on errors, but I don't think a type error will be super obvious. Maybe some other sentinel would be better, like -2? (IIRC, -1 is used for descriptors.) I don't feel too strongly about this, though. I guess this ArraySet will only be visible in call stacks that include this function?

@kripken kripken Jun 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 or -2 are valid values, though - if we improve the analysis, it would be like we are actually writing there, which seems risky for correctness.

If we really want to make valid IR here, we could generate imported functions and put calls to them here. Then it would even be valid to print the module in the middle of the pass here. I think if we ever run into annoying issues here, we can consider that.

auto* set = builder.makeArraySet(ref, index, get, MemoryOrder::Unordered);
visitArraySet(set);
}
void visitArrayInitData(ArrayInitData* curr) { visitArrayInit(curr); }
void visitArrayInitElem(ArrayInitElem* curr) { visitArrayInit(curr); }

void visitArrayInitData(ArrayInitData* curr) { handleArrayWrite(curr->ref); }
void visitArrayInitElem(ArrayInitElem* curr) { handleArrayWrite(curr->ref); }
void visitArrayRMW(ArrayRMW* curr) {
if (curr->ref->type == Type::unreachable) {
return;
Expand Down Expand Up @@ -1219,6 +1222,7 @@ struct InfoCollector
}
void visitStringEncode(StringEncode* curr) {
// TODO: optimize when possible
handleArrayWrite(curr->array);
addRoot(curr);
}
void visitStringConcat(StringConcat* curr) {
Expand Down
82 changes: 82 additions & 0 deletions test/lit/passes/gufa-closed-open.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,85 @@
)
)

;; Write to an array using string.encode.
(module
;; OPEND: (type $array (array (mut i16)))
;; CLOSE: (type $array (array (mut i16)))
(type $array (array (mut i16)))

;; OPEND: (type $1 (func))

;; OPEND: (type $2 (func (result i32)))

;; OPEND: (global $global (ref $array) (array.new_default $array
;; OPEND-NEXT: (i32.const 42)
;; OPEND-NEXT: ))
;; CLOSE: (type $1 (func))

;; CLOSE: (type $2 (func (result i32)))

;; CLOSE: (global $global (ref $array) (array.new_default $array
;; CLOSE-NEXT: (i32.const 42)
;; CLOSE-NEXT: ))
(global $global (ref $array) (array.new_default $array
(i32.const 42)
))

;; OPEND: (export "encode" (func $encode))

;; OPEND: (export "read" (func $read))

;; OPEND: (func $encode (type $1)
;; OPEND-NEXT: (drop
;; OPEND-NEXT: (string.encode_wtf16_array
;; OPEND-NEXT: (string.const "hello")
;; OPEND-NEXT: (global.get $global)
;; OPEND-NEXT: (i32.const 0)
;; OPEND-NEXT: )
;; OPEND-NEXT: )
;; OPEND-NEXT: )
;; CLOSE: (export "encode" (func $encode))

;; CLOSE: (export "read" (func $read))

;; CLOSE: (func $encode (type $1)
;; CLOSE-NEXT: (drop
;; CLOSE-NEXT: (string.encode_wtf16_array
;; CLOSE-NEXT: (string.const "hello")
;; CLOSE-NEXT: (global.get $global)
;; CLOSE-NEXT: (i32.const 0)
;; CLOSE-NEXT: )
;; CLOSE-NEXT: )
;; CLOSE-NEXT: )
(func $encode (export "encode")
(drop
(string.encode_wtf16_array
(string.const "hello")
(global.get $global)
(i32.const 0)
)
)
)

;; OPEND: (func $read (type $2) (result i32)
;; OPEND-NEXT: (array.get_s $array
;; OPEND-NEXT: (global.get $global)
;; OPEND-NEXT: (i32.const 0)
;; OPEND-NEXT: )
;; OPEND-NEXT: )
;; CLOSE: (func $read (type $2) (result i32)
;; CLOSE-NEXT: (array.get_s $array
;; CLOSE-NEXT: (global.get $global)
;; CLOSE-NEXT: (i32.const 0)
;; CLOSE-NEXT: )
;; CLOSE-NEXT: )
(func $read (export "read") (result i32)
;; We could infer what the value is here, since there is only one write. TODO
;; Meanwhile, we should not infer a wrong value, even in closed world.
(array.get_s $array
(global.get $global)
(i32.const 0)
)
)
)

Loading