[wasm] Lower kMaxCanonicalTypes again

jakobkummerow · V8 LUCI CQ · commit 79f3f1276efa · 2024-08-20T10:33:14.000Z
This reverts part of 2b43121, because there are still some ValueType uses of canonicalized type indices left. So for now we allow a maximum of 1'000'000 canonicalized types. Fixed: 360533914 Change-Id: I5041dc2190165781948b186f31cf02bdf894c1bb Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5797071 Reviewed-by: Matthias Liedtke <mliedtke@chromium.org> Auto-Submit: Jakob Kummerow <jkummerow@chromium.org> Commit-Queue: Matthias Liedtke <mliedtke@chromium.org> Commit-Queue: Jakob Kummerow <jkummerow@chromium.org> Cr-Commit-Position: refs/heads/main@{#95710}
diff --git a/src/wasm/canonical-types.cc b/src/wasm/canonical-types.cc
@@ -17,14 +17,25 @@ TypeCanonicalizer* GetTypeCanonicalizer() {
 
 TypeCanonicalizer::TypeCanonicalizer() { AddPredefinedArrayTypes(); }
 
-// For convenience, limit canonicalized type indices to Smi range.
-// We could squeeze out a few more bits if necessary by passing them
-// from compiled wrappers to runtime functions as Smi-tagged unsigned ints.
-// That would give us "uint31" range on 32-bit platforms, and allow
-// uint32_t (or even more) on 64-bit platforms. But we probably don't want
-// to store that many types in the TypeCanonicalizer anyway.
-static constexpr size_t kMaxCanonicalTypes = kSmiMaxValue;
+// Inside the TypeCanonicalizer, we use ValueType instances constructed
+// from canonical type indices, so we can't let them get bigger than what
+// we have storage space for. Code outside the TypeCanonicalizer already
+// supports up to Smi range for canonical type indices.
+// TODO(jkummerow): Raise this limit. Possible options:
+// - increase the size of ValueType::HeapTypeField, using currently-unused bits.
+// - change the encoding of ValueType: one bit says whether it's a ref type,
+//   the other bits then encode the index or the kind of non-ref type.
+// - refactor the TypeCanonicalizer's internals to no longer use ValueTypes
+//   and related infrastructure, and use a wider encoding of canonicalized
+//   type indices only here.
+// - wait for 32-bit platforms to no longer be relevant, and increase the
+//   size of ValueType to 64 bits.
+// None of this seems urgent, as we have no evidence of the current limit
+// being an actual limitation in practice.
+static constexpr size_t kMaxCanonicalTypes = kV8MaxWasmTypes;
+// We don't want any valid modules to fail canonicalization.
 static_assert(kMaxCanonicalTypes >= kV8MaxWasmTypes);
+// We want the invalid index to fail any range checks.
 static_assert(kInvalidCanonicalIndex > kMaxCanonicalTypes);
 
 void TypeCanonicalizer::CheckMaxCanonicalIndex() const {
@@ -215,6 +226,7 @@ ValueType TypeCanonicalizer::CanonicalizeValueType(
     const WasmModule* module, ValueType type,
     uint32_t recursive_group_start) const {
   if (!type.has_index()) return type;
+  static_assert(kMaxCanonicalTypes <= (1u << ValueType::kHeapTypeBits));
   return type.ref_index() >= recursive_group_start
              ? ValueType::CanonicalWithRelativeIndex(
                    type.kind(), type.ref_index() - recursive_group_start)
diff --git a/src/wasm/value-type.h b/src/wasm/value-type.h
@@ -594,6 +594,7 @@ class ValueType {
 
   static constexpr ValueType FromIndex(ValueKind kind, uint32_t index) {
     DCHECK(kind == kRefNull || kind == kRef || kind == kRtt);
+    CHECK_LT(index, kV8MaxWasmTypes);
     return ValueType(KindField::encode(kind) | HeapTypeField::encode(index));
   }
 

Original file line number	Diff line number	Diff line change
`@@ -594,6 +594,7 @@ class ValueType {`
`594`	`594`
`595`	`595`	`static constexpr ValueType FromIndex(ValueKind kind, uint32_t index) {`
`596`	`596`	`DCHECK(kind == kRefNull \|\| kind == kRef \|\| kind == kRtt);`
	`597`	`+ CHECK_LT(index, kV8MaxWasmTypes);`
`597`	`598`	`return ValueType(KindField::encode(kind) \| HeapTypeField::encode(index));`
`598`	`599`	`}`
`599`	`600`