[turboshaft] Fix wrong constant folding of strings map loads

DadaIsCrazy · V8 LUCI CQ · commit afc18842029d · 2024-01-22T17:33:13.000Z
is_stable should only be used on non-primitive maps. Strings can change maps despite their maps being "stable". Bug: chromium:1518396 Change-Id: I3bcb33223f6d08df30d97da6eca2b06efb052960 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5217051 Auto-Submit: Darius Mercadier <dmercadier@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Commit-Queue: Darius Mercadier <dmercadier@chromium.org> Cr-Commit-Position: refs/heads/main@{#91942}
diff --git a/src/compiler/turboshaft/machine-optimization-reducer.h b/src/compiler/turboshaft/machine-optimization-reducer.h
@@ -1790,8 +1790,7 @@ class MachineOptimizationReducer : public Next {
             UnparkedScopeIfNeeded scope(broker);
             AllowHandleDereference allow_handle_dereference;
             OptionalMapRef map = TryMakeRef(broker, base.handle()->map());
-            if (map.has_value() && map->is_stable() && !map->is_deprecated()) {
-              broker->dependencies()->DependOnStableMap(*map);
+            if (MapLoadCanBeConstantFolded(map)) {
               return __ HeapConstant(map->object());
             }
           }
@@ -2399,6 +2398,27 @@ class MachineOptimizationReducer : public Next {
     return base::nullopt;
   }
 
+  // Returns true if loading the map of an object with map {map} can be constant
+  // folded and done at compile time or not. For instance, doing this for
+  // strings is not safe, since the map of a string could change during a GC,
+  // but doing this for a HeapNumber is always safe.
+  bool MapLoadCanBeConstantFolded(OptionalMapRef map) {
+    if (!map.has_value()) return false;
+
+    if (map->IsJSObjectMap() && map->is_stable()) {
+      broker->dependencies()->DependOnStableMap(*map);
+      // For JS objects, this is only safe is the map is stable.
+      return true;
+    }
+
+    if (map->instance_type() ==
+        any_of(BIG_INT_BASE_TYPE, HEAP_NUMBER_TYPE, ODDBALL_TYPE)) {
+      return true;
+    }
+
+    return false;
+  }
+
   static constexpr bool IsNegativePowerOfTwo(int64_t x) {
     if (x >= 0) return false;
     if (x == std::numeric_limits<int64_t>::min()) return true;
diff --git a/test/mjsunit/compiler/regress-crbug-1518396.js b/test/mjsunit/compiler/regress-crbug-1518396.js
@@ -0,0 +1,23 @@
+// Copyright 2024 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: --allow-natives-syntax
+
+function get_thin_string(a, b) {
+  var str = a + b;
+  var o = {};
+  o[str];
+  return str;
+}
+
+var str = get_thin_string("bar");
+
+function CheckCS() {
+  return str.charCodeAt();
+}
+
+%PrepareFunctionForOptimization(CheckCS);
+assertEquals(CheckCS(), 98);
+%OptimizeFunctionOnNextCall(CheckCS);
+assertEquals(CheckCS(), 98);
