[stringrefs] Fix inlining, and some corner case bugs

1) Inlining functions that contain stringref operations require builtin
   calls to be marked as kNoThrow appropriately (or have exception
   handling support in the graph).
2) Some overly-large inputs for string creation hit DCHECKs before
   getting to the places where they would have thrown an orderly
   exception.
3) We still had a known issue that some exceptions thrown by JS-focused
   code were erroneously catchable by Wasm.
4) When string.concat attempted to create a too-long string, it ran into
   a DCHECK because we didn't clear the "thread in wasm" flag.
5) The builtin call for string.concat was erroneously marked as
   kEliminatable, which could cause the trap get eliminated.

Bug: v8:12868
Change-Id: Iad3ada0e2465bfd8f3d00bb064c32049d6b19d87
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3902522
Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Andy Wingo <wingo@igalia.com>
Reviewed-by: Andy Wingo <wingo@igalia.com>
Cr-Commit-Position: refs/heads/main@{#83292}

src/heap/factory.cc

@@ -775,6 +775,11 @@ MaybeHandle<String> NewStringFromUtf8Variant(Isolate* isolate,
 MaybeHandle<String> Factory::NewStringFromUtf8(
     const base::Vector<const uint8_t>& string,
     unibrow::Utf8Variant utf8_variant, AllocationType allocation) {
+  if (string.size() > kMaxInt) {
+    // The Utf8Decode can't handle longer inputs, and we couldn't create
+    // strings from them anyway.
+    THROW_NEW_ERROR(isolate(), NewInvalidStringLengthError(), String);
+  }
   auto peek_bytes = [&]() -> base::Vector<const uint8_t> { return string; };
   return NewStringFromUtf8Variant(isolate(), peek_bytes, utf8_variant,
                                   allocation);
@@ -793,6 +798,8 @@ MaybeHandle<String> Factory::NewStringFromUtf8(
   DCHECK_EQ(sizeof(uint8_t), array->type()->element_type().value_kind_size());
   DCHECK_LE(start, end);
   DCHECK_LE(end, array->length());
+  // {end - start} can never be more than what the Utf8Decoder can handle.
+  static_assert(WasmArray::MaxLength(sizeof(uint8_t)) <= kMaxInt);
   auto peek_bytes = [&]() -> base::Vector<const uint8_t> {
     const uint8_t* contents =
         reinterpret_cast<const uint8_t*>(array->ElementAddress(0));
@@ -807,6 +814,8 @@ MaybeHandle<String> Factory::NewStringFromUtf8(
     unibrow::Utf8Variant utf8_variant, AllocationType allocation) {
   DCHECK_LE(start, end);
   DCHECK_LE(end, array->length());
+  // {end - start} can never be more than what the Utf8Decoder can handle.
+  static_assert(ByteArray::kMaxLength <= kMaxInt);
   auto peek_bytes = [&]() -> base::Vector<const uint8_t> {
     const uint8_t* contents =
         reinterpret_cast<const uint8_t*>(array->GetDataStartAddress());
@@ -839,6 +848,8 @@ MaybeHandle<String> Factory::NewStringFromUtf16(Handle<WasmArray> array,
   DCHECK_EQ(sizeof(uint16_t), array->type()->element_type().value_kind_size());
   DCHECK_LE(start, end);
   DCHECK_LE(end, array->length());
+  // {end - start} can never be more than what the Utf8Decoder can handle.
+  static_assert(WasmArray::MaxLength(sizeof(uint16_t)) <= kMaxInt);
   auto peek_bytes = [&]() -> base::Vector<const uint16_t> {
     const uint16_t* contents =
         reinterpret_cast<const uint16_t*>(array->ElementAddress(0));

src/runtime/runtime-internal.cc

@@ -19,8 +19,8 @@
 #include "src/utils/ostreams.h"
 
 #if V8_ENABLE_WEBASSEMBLY
-// TODO(jkummerow): Drop this when the "SaveAndClearThreadInWasmFlag"
-// short-term mitigation is no longer needed.
+// TODO(chromium:1236668): Drop this when the "SaveAndClearThreadInWasmFlag"
+// approach is no longer needed.
 #include "src/trap-handler/trap-handler.h"
 #endif  // V8_ENABLE_WEBASSEMBLY
 
@@ -418,7 +418,7 @@ RUNTIME_FUNCTION(Runtime_BytecodeBudgetInterruptWithStackCheck_Maglev) {
 namespace {
 
 #if V8_ENABLE_WEBASSEMBLY
-class SaveAndClearThreadInWasmFlag {
+class V8_NODISCARD SaveAndClearThreadInWasmFlag {
  public:
   SaveAndClearThreadInWasmFlag() {
     if (trap_handler::IsTrapHandlerEnabled()) {
@@ -460,10 +460,10 @@ RUNTIME_FUNCTION(Runtime_AllocateInYoungGeneration) {
   }
 
 #if V8_ENABLE_WEBASSEMBLY
-  // Short-term mitigation for crbug.com/1236668. When this is called from
-  // WasmGC code, clear the "thread in wasm" flag, which is important in case
-  // any GC needs to happen.
-  // TODO(jkummerow): Find a better fix, likely by replacing the global flag.
+  // When this is called from WasmGC code, clear the "thread in wasm" flag,
+  // which is important in case any GC needs to happen.
+  // TODO(chromium:1236668): Find a better fix, likely by replacing the global
+  // flag.
   SaveAndClearThreadInWasmFlag clear_wasm_flag;
 #endif  // V8_ENABLE_WEBASSEMBLY
 

src/runtime/runtime-strings.cc

@@ -11,9 +11,46 @@
 #include "src/objects/smi.h"
 #include "src/strings/string-builder-inl.h"
 
+#if V8_ENABLE_WEBASSEMBLY
+// TODO(chromium:1236668): Drop this when the "SaveAndClearThreadInWasmFlag"
+// approach is no longer needed.
+#include "src/trap-handler/trap-handler.h"
+#endif  // V8_ENABLE_WEBASSEMBLY
+
 namespace v8 {
 namespace internal {
 
+namespace {
+
+#if V8_ENABLE_WEBASSEMBLY
+class V8_NODISCARD SaveAndClearThreadInWasmFlag {
+ public:
+  explicit SaveAndClearThreadInWasmFlag(Isolate* isolate) : isolate_(isolate) {
+    if (trap_handler::IsTrapHandlerEnabled()) {
+      if (trap_handler::IsThreadInWasm()) {
+        thread_was_in_wasm_ = true;
+        trap_handler::ClearThreadInWasm();
+      }
+    }
+  }
+  ~SaveAndClearThreadInWasmFlag() {
+    if (thread_was_in_wasm_ && !isolate_->has_pending_exception()) {
+      trap_handler::SetThreadInWasm();
+    }
+  }
+
+ private:
+  bool thread_was_in_wasm_{false};
+  Isolate* isolate_;
+};
+#define CLEAR_THREAD_IN_WASM_SCOPE \
+  SaveAndClearThreadInWasmFlag non_wasm_scope(isolate)
+#else
+#define CLEAR_THREAD_IN_WASM_SCOPE (void)0
+#endif  // V8_ENABLE_WEBASSEMBLY
+
+}  // namespace
+
 RUNTIME_FUNCTION(Runtime_GetSubstitution) {
   HandleScope scope(isolate);
   DCHECK_EQ(5, args.length());
@@ -154,6 +191,8 @@ RUNTIME_FUNCTION(Runtime_StringSubstring) {
 }
 
 RUNTIME_FUNCTION(Runtime_StringAdd) {
+  // This is used by Wasm stringrefs.
+  CLEAR_THREAD_IN_WASM_SCOPE;
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
   Handle<String> str1 = args.at<String>(0);

src/runtime/runtime-wasm.cc

@@ -870,6 +870,27 @@ RUNTIME_FUNCTION(Runtime_WasmCreateResumePromise) {
   return *result;
 }
 
+#define RETURN_RESULT_OR_TRAP(call)                                            \
+  do {                                                                         \
+    Handle<Object> result;                                                     \
+    if (!(call).ToHandle(&result)) {                                           \
+      DCHECK(isolate->has_pending_exception());                                \
+      /* Mark any exception as uncatchable by Wasm. */                         \
+      Handle<JSObject> exception(JSObject::cast(isolate->pending_exception()), \
+                                 isolate);                                     \
+      Handle<Name> uncatchable =                                               \
+          isolate->factory()->wasm_uncatchable_symbol();                       \
+      LookupIterator it(isolate, exception, uncatchable, LookupIterator::OWN); \
+      if (!JSReceiver::HasProperty(&it).FromJust()) {                          \
+        JSObject::AddProperty(isolate, exception, uncatchable,                 \
+                              isolate->factory()->true_value(), NONE);         \
+      }                                                                        \
+      return ReadOnlyRoots(isolate).exception();                               \
+    }                                                                          \
+    DCHECK(!isolate->has_pending_exception());                                 \
+    return *result;                                                            \
+  } while (false)
+
 // Returns the new string if the operation succeeds.  Otherwise throws an
 // exception and returns an empty result.
 RUNTIME_FUNCTION(Runtime_WasmStringNewWtf8) {
@@ -896,8 +917,8 @@ RUNTIME_FUNCTION(Runtime_WasmStringNewWtf8) {
 
   const base::Vector<const uint8_t> bytes{instance.memory_start() + offset,
                                           size};
-  RETURN_RESULT_OR_FAILURE(
-      isolate, isolate->factory()->NewStringFromUtf8(bytes, utf8_variant));
+  RETURN_RESULT_OR_TRAP(
+      isolate->factory()->NewStringFromUtf8(bytes, utf8_variant));
 }
 
 RUNTIME_FUNCTION(Runtime_WasmStringNewWtf8Array) {
@@ -913,8 +934,8 @@ RUNTIME_FUNCTION(Runtime_WasmStringNewWtf8Array) {
          static_cast<uint32_t>(unibrow::Utf8Variant::kLastUtf8Variant));
   auto utf8_variant = static_cast<unibrow::Utf8Variant>(utf8_variant_value);
 
-  RETURN_RESULT_OR_FAILURE(isolate, isolate->factory()->NewStringFromUtf8(
-                                        array, start, end, utf8_variant));
+  RETURN_RESULT_OR_TRAP(
+      isolate->factory()->NewStringFromUtf8(array, start, end, utf8_variant));
 }
 
 RUNTIME_FUNCTION(Runtime_WasmStringNewWtf16) {
@@ -940,10 +961,8 @@ RUNTIME_FUNCTION(Runtime_WasmStringNewWtf16) {
 
   const byte* bytes = instance.memory_start() + offset;
   const base::uc16* codeunits = reinterpret_cast<const base::uc16*>(bytes);
-  // TODO(12868): Override any exception with an uncatchable-by-wasm trap.
-  RETURN_RESULT_OR_FAILURE(isolate,
-                           isolate->factory()->NewStringFromTwoByteLittleEndian(
-                               {codeunits, size_in_codeunits}));
+  RETURN_RESULT_OR_TRAP(isolate->factory()->NewStringFromTwoByteLittleEndian(
+      {codeunits, size_in_codeunits}));
 }
 
 RUNTIME_FUNCTION(Runtime_WasmStringNewWtf16Array) {
@@ -954,9 +973,8 @@ RUNTIME_FUNCTION(Runtime_WasmStringNewWtf16Array) {
   uint32_t start = NumberToUint32(args[1]);
   uint32_t end = NumberToUint32(args[2]);
 
-  // TODO(12868): Override any exception with an uncatchable-by-wasm trap.
-  RETURN_RESULT_OR_FAILURE(
-      isolate, isolate->factory()->NewStringFromUtf16(array, start, end));
+  RETURN_RESULT_OR_TRAP(
+      isolate->factory()->NewStringFromUtf16(array, start, end));
 }
 
 // Returns the new string if the operation succeeds.  Otherwise traps.
@@ -1291,9 +1309,12 @@ RUNTIME_FUNCTION(Runtime_WasmStringViewWtf8Slice) {
   DCHECK_LT(start, end);
   DCHECK(base::IsInBounds<size_t>(start, end - start, array->length()));
 
-  RETURN_RESULT_OR_FAILURE(isolate,
-                           isolate->factory()->NewStringFromUtf8(
-                               array, start, end, unibrow::Utf8Variant::kWtf8));
+  // This can't throw because the result can't be too long if the input wasn't,
+  // and encoding failures are ruled out too because {start}/{end} are aligned.
+  return *isolate->factory()
+              ->NewStringFromUtf8(array, start, end,
+                                  unibrow::Utf8Variant::kWtf8)
+              .ToHandleChecked();
 }
 
 }  // namespace internal

src/wasm/wasm-objects.h

@@ -1000,7 +1000,7 @@ class WasmArray : public TorqueGeneratedWasmArray<WasmArray, WasmObject> {
   inline uint32_t element_offset(uint32_t index);
   inline Address ElementAddress(uint32_t index);
 
-  static int MaxLength(uint32_t element_size_bytes) {
+  static constexpr int MaxLength(uint32_t element_size_bytes) {
     // The total object size must fit into a Smi, for filler objects. To make
     // the behavior of Wasm programs independent from the Smi configuration,
     // we hard-code the smaller of the two supported ranges.

test/mjsunit/wasm/stringrefs-regressions.js

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --experimental-wasm-stringref --allow-natives-syntax
+// We just want speculative inlining, but the "stress" variant doesn't like
+// that flag for some reason, so use the GC flag which implies it.
+// Flags: --experimental-wasm-gc
+
+d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js");
+
+let kSig_w_v = makeSig([], [kWasmStringRef]);
+let kSig_w_i = makeSig([kWasmI32], [kWasmStringRef]);
+let kSig_v_w = makeSig([kWasmStringRef], []);
+
+(function () {
+  let huge_builder = new WasmModuleBuilder();
+  huge_builder.addMemory(65001, undefined, false, false);
+
+  huge_builder.addFunction("huge", kSig_w_v).exportFunc().addBody([
+    kExprI32Const, 0,                  // address
+    ...wasmI32Const(65000 * 65536),    // bytes
+    ...GCInstr(kExprStringNewUtf8), 0  // memory index
+  ]);
+
+  let callee = huge_builder.addFunction("callee", kSig_w_i).addBody([
+    kExprI32Const, 0,                  // address
+    kExprLocalGet, 0,                  // bytes
+    ...GCInstr(kExprStringNewUtf8), 0  // memory index
+  ]);
+
+  let caller = huge_builder.addFunction("caller", kSig_i_i).exportFunc()
+    .addBody([
+      kExprTry, kWasmI32,
+      kExprLocalGet, 0,
+      kExprCallFunction, callee.index,
+      kExprDrop,
+      kExprI32Const, 1,
+      kExprCatchAll,
+      kExprI32Const, 0,
+      kExprEnd
+    ]);
+
+  let instance;
+  try {
+    instance = huge_builder.instantiate();
+    // On 64-bit platforms, expect to see this message.
+    console.log("Instantiation successful, proceeding.");
+  } catch (e) {
+    // 32-bit builds don't have enough virtual memory, that's OK.
+    assertInstanceof(e, RangeError);
+    assertMatches(/Cannot allocate Wasm memory for new instance/, e.message,
+      'Error message');
+    return;
+  }
+
+  // Bug 1: The Utf8Decoder can't handle more than kMaxInt bytes as input.
+  assertThrows(() => instance.exports.huge(), RangeError);
+
+  // Bug 2: Exceptions created by the JS-focused strings infrastructure must
+  // be marked as uncatchable by Wasm.
+  let f1 = instance.exports.caller;
+  assertThrows(() => f1(2147483647), RangeError);
+
+  // Bug 3: Builtin calls that have neither a kNoThrow annotation nor exception-
+  // handling support make the Wasm inliner sad.
+  for (let i = 0; i < 20; i++) f1(10);
+  %WasmTierUpFunction(instance, caller.index);
+  f1(10);
+})();
+
+let builder = new WasmModuleBuilder();
+
+let concat_body = [];
+// This doubles the string 26 times, i.e. multiplies its length with a factor
+// of ~65 million.
+for (let i = 0; i < 26; i++) {
+  concat_body.push(...[
+    kExprLocalGet, 0, kExprLocalGet, 0,
+    ...GCInstr(kExprStringConcat),
+    kExprLocalSet, 0
+  ]);
+}
+
+let concat =
+    builder.addFunction('concat', kSig_v_w).exportFunc().addBody(concat_body);
+
+let instance = builder.instantiate();
+
+// Bug 4: Throwing in StringAdd must clear the "thread in wasm" bit.
+let f2 = instance.exports.concat;
+assertThrows(() => f2("1234567890"));  // 650M characters is too much.
+
+// Bug 5: Operations that can trap must not be marked as kEliminatable,
+// otherwise the trap may be eliminated.
+for (let i = 0; i < 3; i++) f2("a");   // 65M characters is okay.
+%WasmTierUpFunction(instance, concat.index);
+assertThrows(() => f2("1234567890"));  // Optimized code still traps.