Skip to content

GUFA: Fix signed reads of packed GC data #6494

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
merged 5 commits into from
Apr 11, 2024
Merged

GUFA: Fix signed reads of packed GC data #6494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52adaae
fix
kripken Apr 11, 2024
20b02d0
fix
kripken Apr 11, 2024
7096840
test
kripken Apr 11, 2024
b25bca5
test
kripken Apr 11, 2024
18e4ffd
typo
kripken Apr 11, 2024
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
69 changes: 68 additions & 1 deletion src/ir/possible-contents.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1999,6 +1999,8 @@ struct Flower {
const GlobalLocation& globalLoc);
void filterDataContents(PossibleContents& contents,
const DataLocation& dataLoc);
void filterPackedDataReads(PossibleContents& contents,
const ExpressionLocation& exprLoc);

// Reads from GC data: a struct.get or array.get. This is given the type of
// the read operation, the field that is read on that type, the known contents
Expand Down Expand Up @@ -2301,10 +2303,24 @@ bool Flower::updateContents(LocationIndex locationIndex,
if (auto* dataLoc = std::get_if<DataLocation>(&location)) {
filterDataContents(newContents, *dataLoc);
#if defined(POSSIBLE_CONTENTS_DEBUG) && POSSIBLE_CONTENTS_DEBUG >= 2
std::cout << " pre-filtered contents:\n";
std::cout << " pre-filtered data contents:\n";
newContents.dump(std::cout, &wasm);
std::cout << '\n';
#endif
} else if (auto* exprLoc = std::get_if<ExpressionLocation>(&location)) {
if (exprLoc->expr->is<StructGet>() || exprLoc->expr->is<ArrayGet>()) {
// Packed data reads must be filtered before the combine() operation, as
// we must only combine the filtered contents (e.g. if 0xff arrives which
// as a signed read is truly 0xffffffff then we cannot first combine the
// existing 0xffffffff with the new 0xff, as they are different, and the
// result will no longer be a constant).
filterPackedDataReads(newContents, *exprLoc);
#if defined(POSSIBLE_CONTENTS_DEBUG) && POSSIBLE_CONTENTS_DEBUG >= 2
std::cout << " pre-filtered packed read contents:\n";
newContents.dump(std::cout, &wasm);
std::cout << '\n';
#endif
}
}

contents.combine(newContents);
Expand Down Expand Up @@ -2633,6 +2649,57 @@ void Flower::filterDataContents(PossibleContents& contents,
}
}

void Flower::filterPackedDataReads(PossibleContents& contents,
const ExpressionLocation& exprLoc) {
auto* expr = exprLoc.expr;

// Packed fields are stored as the truncated bits (see comment on
// DataLocation; the actual truncation is done in filterDataContents), which
// means that unsigned gets just work but signed ones need fixing (and we only
// know how to do that here, when we reach the get and see if it is signed).
auto signed_ = false;
Expression* ref;
Index index;
if (auto* get = expr->dynCast<StructGet>()) {
signed_ = get->signed_;
ref = get->ref;
index = get->index;
} else if (auto* get = expr->dynCast<ArrayGet>()) {
signed_ = get->signed_;
ref = get->ref;
// Arrays are treated as having a single field.
index = 0;
} else {
WASM_UNREACHABLE("bad packed read");
}
if (!signed_) {
return;
}

// We are reading data here, so the reference must be a valid struct or
// array, otherwise we would never have gotten here.
assert(ref->type.isRef());
auto field = GCTypeUtils::getField(ref->type.getHeapType(), index);
assert(field);
if (!field->isPacked()) {
return;
}

if (contents.isLiteral()) {
// This is a constant. We can sign-extend it and use that value.
auto shifts = Literal(int32_t(32 - field->getByteSize() * 8));
auto lit = contents.getLiteral();
lit = lit.shl(shifts);
lit = lit.shrS(shifts);
contents = PossibleContents::literal(lit);
} else {
// This is not a constant. As in filterDataContents, give up and leave
// only the type, since we have no way to track the sign-extension on
// top of whatever this is.
contents = PossibleContents::fromType(contents.getType());
}
}

void Flower::readFromData(Type declaredType,
Index fieldIndex,
const PossibleContents& refContents,
Expand Down
5 changes: 5 additions & 0 deletions src/ir/possible-contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,11 @@ struct SignatureResultLocation {
// The location of contents in a struct or array (i.e., things that can fit in a
// dataref). Note that this is specific to this type - it does not include data
// about subtypes or supertypes.
//
// We store the truncated bits here when the field is packed. That is, if -1 is
// written to an i8 then the value here will be 0xff. StructGet/ArrayGet
// operations that read a signed value must then perform a sign-extend
// operation.
struct DataLocation {
HeapType type;
// The index of the field in a struct, or 0 for an array (where we do not
Expand Down
261 changes: 261 additions & 0 deletions test/lit/passes/gufa-refs.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5622,6 +5622,267 @@
)
)

;; Packed fields with signed gets.
(module
;; CHECK: (type $array (array (mut i8)))

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

;; CHECK: (type $struct (struct (field i16)))
(type $struct (struct (field i16)))

(type $array (array (mut i8)))

;; CHECK: (func $test-struct (type $1)
;; CHECK-NEXT: (local $x (ref $struct))
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 65535)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test-struct
(local $x (ref $struct))
(local.set $x
(struct.new $struct
(i32.const -1)
)
)
;; This reads -1.
(drop
(struct.get_s $struct 0
(local.get $x)
)
)
;; This reads 65535, as the other bits were truncated.
(drop
(struct.get_u $struct 0
(local.get $x)
)
)
)

;; CHECK: (func $test-array (type $1)
;; CHECK-NEXT: (local $x (ref $array))
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (array.new_fixed $array 1
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (array.get_s $array
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (array.get_u $array
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 255)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test-array
(local $x (ref $array))
(local.set $x
(array.new_fixed $array 1
(i32.const -1)
)
)
;; This reads -1.
(drop
(array.get_s $array
(local.get $x)
(i32.const 0)
)
)
;; This reads 255, as the other bits were truncated.
(drop
(array.get_u $array
(local.get $x)
(i32.const 0)
)
)
)
)

;; Packed fields with conflicting sets.
(module

;; CHECK: (type $struct (struct (field i16)))

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

;; CHECK: (import "a" "b" (global $import i32))
(import "a" "b" (global $import i32))

(type $struct (struct (field i16)))

;; CHECK: (func $test-struct (type $1)
;; CHECK-NEXT: (local $x (ref null $struct))
;; CHECK-NEXT: (if
;; CHECK-NEXT: (global.get $import)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (i32.const 42)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get_s $struct 0
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get_u $struct 0
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test-struct
(local $x (ref null $struct))
(if
(global.get $import)
(then
(local.set $x
(struct.new $struct
(i32.const -1)
)
)
)
(else
(local.set $x
(struct.new $struct
(i32.const 42)
)
)
)
)
;; We cannot infer anything for these reads.
(drop
(struct.get_s $struct 0
(local.get $x)
)
)
(drop
(struct.get_u $struct 0
(local.get $x)
)
)
)
)

;; Packed fields with different sets that actually do not conflict.
(module

;; CHECK: (type $struct (struct (field i16)))

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

;; CHECK: (import "a" "b" (global $import i32))
(import "a" "b" (global $import i32))

(type $struct (struct (field i16)))

;; CHECK: (func $test-struct (type $1)
;; CHECK-NEXT: (local $x (ref null $struct))
;; CHECK-NEXT: (if
;; CHECK-NEXT: (global.get $import)
;; CHECK-NEXT: (then
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (else
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (struct.new $struct
;; CHECK-NEXT: (i32.const 65535)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get_s $struct 0
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const -1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (block (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (struct.get_u $struct 0
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.const 65535)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $test-struct
(local $x (ref null $struct))
(if
(global.get $import)
(then
(local.set $x
(struct.new $struct
(i32.const -1)
)
)
)
(else
(local.set $x
(struct.new $struct
(i32.const 65535)
)
)
)
)
;; We can infer here because -1 and 65535 are actually the same, after
;; truncation.
(drop
(struct.get_s $struct 0
(local.get $x)
)
)
(drop
(struct.get_u $struct 0
(local.get $x)
)
)
)
)

;; Test that we do not error on array.init of a bottom type.
(module
(type $"[mut:i32]" (array (mut i32)))
Expand Down