Skip to content

Commit

Permalink
GUFA: Fix signed reads of packed GC data (#6494)
Browse files Browse the repository at this point in the history 
GUFA already truncated packed fields on write, which is enough for unsigned gets,
but for signed gets we also need to sign them on reads.

Similar to #6493 but for GUFA. Also found by #6486
  • Loading branch information
@kripken
kripken authored Apr 11, 2024
1 parent 0b0c338 commit 729f64c
Show file tree
Hide file tree
Showing 3 changed files with 334 additions and 1 deletion.
69 changes: 68 additions & 1 deletion src/ir/possible-contents.cpp
View file
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
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
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

0 comments on commit 729f64c

Please sign in to comment.