Skip to content

GUFA: Don't misoptimize struct/array atomics #8484

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 7 commits into from
Mar 18, 2026
+315 −33
Merged

GUFA: Don't misoptimize struct/array atomics #8484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0481105
go
kripken Mar 18, 2026
318fb9a
array
kripken Mar 18, 2026
3163e52
fix
kripken Mar 18, 2026
98defbf
fix
kripken Mar 18, 2026
d987b1a
fix
kripken Mar 18, 2026
68184fa
split.test
kripken Mar 18, 2026
38cfcc2
Merge remote-tracking branch 'origin/main' into gufa.struct.rmw
kripken Mar 18, 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
1 change: 1 addition & 0 deletions scripts/test/fuzzing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
'cfp-rmw.wast',
'unsubtyping-cmpxchg.wast',
'struct-atomic-threads.wast',
'type-refining-gufa-rmw.wast',
# contains too many segments to run in a wasm VM
'limit-segments_disable-bulk-memory.wast',
# https://github.com/WebAssembly/binaryen/issues/7176
Expand Down
97 changes: 64 additions & 33 deletions src/ir/possible-contents.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,16 +1073,19 @@ struct InfoCollector
if (curr->ref->type == Type::unreachable) {
return;
}
// TODO: Model the modification part of the RMW in addition to the read and
// the write.
addChildParentLink(curr->ref, curr);
addChildParentLink(curr->value, curr);
// TODO: Model the output
addRoot(curr);
}
void visitStructCmpxchg(StructCmpxchg* curr) {
if (curr->ref->type == Type::unreachable) {
return;
}
// TODO: Model the modification part of the RMW in addition to the read and
// the write.
addChildParentLink(curr->ref, curr);
addChildParentLink(curr->expected, curr);
addChildParentLink(curr->replacement, curr);
// TODO: Model the output
addRoot(curr);
}
void visitStructWait(StructWait* curr) { addRoot(curr); }
Expand Down Expand Up @@ -1171,16 +1174,19 @@ struct InfoCollector
if (curr->ref->type == Type::unreachable) {
return;
}
// TODO: Model the modification part of the RMW in addition to the read and
// the write.
addChildParentLink(curr->ref, curr);
addChildParentLink(curr->value, curr);
// TODO: Model the output
addRoot(curr);
}
void visitArrayCmpxchg(ArrayCmpxchg* curr) {
if (curr->ref->type == Type::unreachable) {
return;
}
// TODO: Model the modification part of the RMW in addition to the read and
// the write.
addChildParentLink(curr->ref, curr);
addChildParentLink(curr->expected, curr);
addChildParentLink(curr->replacement, curr);
// TODO: Model the output
addRoot(curr);
}
void visitStringNew(StringNew* curr) {
Expand Down Expand Up @@ -2247,10 +2253,12 @@ struct Flower {
Expression* read);

// Similar to readFromData, but does a write for a struct.set or array.set.
void writeToData(Expression* ref, Expression* value, Index fieldIndex);

// A write with given contents.
void writeToData(Expression* ref,
Expression* value,
Index fieldIndex,
bool multibyte = false);
const PossibleContents& valueContents,
Index fieldIndex);

// We will need subtypes during the flow, so compute them once ahead of time.
std::unique_ptr<SubTypes> subTypes;
Expand Down Expand Up @@ -2839,6 +2847,18 @@ void Flower::flowAfterUpdate(LocationIndex locationIndex) {
// |child| is either the reference or the value child of a struct.set.
assert(set->ref == child || set->value == child);
writeToData(set->ref, set->value, set->index);
} else if (auto* set = parent->dynCast<StructRMW>()) {
assert(set->ref == child || set->value == child);
// TODO: model the stored value, depending on the actual operation.
writeToData(
set->ref, PossibleContents::fromType(set->value->type), set->index);
} else if (auto* set = parent->dynCast<StructCmpxchg>()) {
assert(set->ref == child || set->expected == child ||
set->replacement == child);
// TODO: model the stored value, depending on the actual operation.
writeToData(set->ref,
PossibleContents::fromType(set->replacement->type),
set->index);
} else if (auto* get = parent->dynCast<ArrayGet>()) {
assert(get->ref == child);
readFromData(get->ref->type, 0, contents, get);
Expand All @@ -2847,9 +2867,21 @@ void Flower::flowAfterUpdate(LocationIndex locationIndex) {
writeToData(set->ref, set->value, 0);
} else if (auto* store = parent->dynCast<ArrayStore>()) {
assert(store->ref == child || store->value == child);
writeToData(store->ref, store->value, 0, /*multibyte*/ true);
// TODO: model the stored value, and handle different but equal values in
// type, e.g. writing i16 0x1212 is the same as i8 0x12.
writeToData(
store->ref, PossibleContents::fromType(store->value->type), 0);
} else if (auto* set = parent->dynCast<ArrayRMW>()) {
assert(set->ref == child || set->value == child);
// TODO: model the stored value, depending on the actual operation.
writeToData(set->ref, PossibleContents::fromType(set->value->type), 0);
} else if (auto* set = parent->dynCast<ArrayCmpxchg>()) {
assert(set->ref == child || set->expected == child ||
set->replacement == child);
// TODO: model the stored value, depending on the actual operation.
writeToData(
set->ref, PossibleContents::fromType(set->replacement->type), 0);
} else if (auto* get = parent->dynCast<RefGetDesc>()) {
// Similar to struct.get.
assert(get->ref == child);
readFromData(
get->ref->type, DataLocation::DescriptorIndex, contents, get);
Expand Down Expand Up @@ -3214,23 +3246,7 @@ void Flower::readFromData(Type declaredType,
connectDuringFlow(coneReadLocation, ExpressionLocation{read, 0});
}

void Flower::writeToData(Expression* ref,
Expression* value,
Index fieldIndex,
bool multibyte) {
#if defined(POSSIBLE_CONTENTS_DEBUG) && POSSIBLE_CONTENTS_DEBUG >= 2
std::cout << " add special writes\n";
#endif

auto refContents = getContents(getIndex(ExpressionLocation{ref, 0}));

#ifndef NDEBUG
// We must not have anything in the reference that is invalid for the wasm
// type there.
auto maximalContents = PossibleContents::coneType(ref->type);
assert(PossibleContents::isSubContents(refContents, maximalContents));
#endif

void Flower::writeToData(Expression* ref, Expression* value, Index fieldIndex) {
// We could set up links here as we do for reads, but as we get to this code
// in any case, we can just flow the values forward directly. This avoids
// adding any links (edges) to the graph (and edges are what we want to avoid
Expand All @@ -3253,6 +3269,24 @@ void Flower::writeToData(Expression* ref,
// reference and value.)

auto valueContents = getContents(getIndex(ExpressionLocation{value, 0}));
writeToData(ref, valueContents, fieldIndex);
}

void Flower::writeToData(Expression* ref,
const PossibleContents& valueContents,
Index fieldIndex) {
#if defined(POSSIBLE_CONTENTS_DEBUG) && POSSIBLE_CONTENTS_DEBUG >= 2
std::cout << " add special writes\n";
#endif

auto refContents = getContents(getIndex(ExpressionLocation{ref, 0}));

#ifndef NDEBUG
// We must not have anything in the reference that is invalid for the wasm
// type there.
auto maximalContents = PossibleContents::coneType(ref->type);
assert(PossibleContents::isSubContents(refContents, maximalContents));
#endif

// See the related comment in readFromData() as to why these are the only
// things we need to check, and why the assertion afterwards contains the only
Expand All @@ -3265,9 +3299,6 @@ void Flower::writeToData(Expression* ref,
// As in readFromData, normalize to the proper cone.
auto cone = refContents.getCone();
auto normalizedDepth = getNormalizedConeDepth(cone.type, cone.depth);
if (multibyte) {
valueContents = PossibleContents::fromType(value->type);
}
Comment on lines -3268 to -3270
Copy link
Member

@tlively tlively Mar 18, 2026

Choose a reason for hiding this comment

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

Are these multibyte changes a drive-by fix? Are they NFC? This isn't clear to me.

Copy link
Member Author

@kripken kripken Mar 18, 2026

Choose a reason for hiding this comment

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

This uses the new code added for the rmw instrs, as mentioned in the description,

Simplify ArrayStore similarly: remove special multibyte handling and
just pass in an unknown value of the type.

The old code manually saw "multibyte" and applied an unknown value. This sends
it in without a special multibyte param.


subTypes->iterSubTypes(
cone.type.getHeapType(), normalizedDepth, [&](HeapType type, Index depth) {
Expand Down
Loading
Loading