[gdbjit] Fix overlapping AddressRegion check

Whenever we are adding a new AddressRegion to the CodeMap, we first
remove all overlapping regions. The logic to check for overlapping
region is incomplete. For example, if all existing regions are less than
the region to be added, we incorrectly remove all regions, effectively
deleting all JITCodeEntry we have constructed.

We extract this overlapping check into a helper function, so that we can
unittest this without worrying about JITCodeEvent functionality, and also
without dealing with V8 internals (like Isolate and SFI).

The overlapping logic is rather hard to understand, has many special
cases, it will probably be much easier to just loop through all the
entries, rather than using lower_bound. Ideally, we can refactor this to
use some sort of sweep-line algorithm. Hopefully the unittests catch the
most obvious cases.

Bug: v8:11908
Change-Id: Id96975599ac59974185c3dbf64cdfceb17e98d18
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3105381
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76397}

src/diagnostics/gdb-jit.cc

@@ -4,6 +4,7 @@
 
 #include "src/diagnostics/gdb-jit.h"
 
+#include <iterator>
 #include <map>
 #include <memory>
 #include <vector>
@@ -1914,13 +1915,11 @@ static void AddUnwindInfo(CodeDescription* desc) {
 
 static base::LazyMutex mutex = LAZY_MUTEX_INITIALIZER;
 
-// Remove entries from the map that intersect the given address region,
-// and deregister them from GDB.
-static void RemoveJITCodeEntries(CodeMap* map,
-                                 const base::AddressRegion region) {
+static base::Optional<std::pair<CodeMap::iterator, CodeMap::iterator>>
+GetOverlappingRegions(CodeMap* map, const base::AddressRegion region) {
   DCHECK_LT(region.begin(), region.end());
 
-  if (map->empty()) return;
+  if (map->empty()) return {};
 
   // Find the first overlapping entry.
 
@@ -1931,30 +1930,53 @@ static void RemoveJITCodeEntries(CodeMap* map,
 
   if (it == map->end()) {
     start_it = map->begin();
+    // Find the first overlapping entry.
+    for (; start_it != map->end(); ++start_it) {
+      if (start_it->first.end() > region.begin()) {
+        break;
+      }
+    }
   } else if (it != map->begin()) {
     for (--it; it != map->begin(); --it) {
       if ((*it).first.end() <= region.begin()) break;
       start_it = it;
     }
+    if (it == map->begin() && it->first.end() > region.begin()) {
+      start_it = it;
+    }
   }
 
-  DCHECK_NE(start_it, map->end());
+  if (start_it == map->end()) {
+    return {};
+  }
 
   // Find the first non-overlapping entry after `region`.
 
   const auto end_it = map->lower_bound({region.end(), 0});
 
-  // Evict intersecting regions.
+  // Return a range containing intersecting regions.
 
-  if (std::distance(start_it, end_it) < 1) return;  // No overlapping entries.
+  if (std::distance(start_it, end_it) < 1)
+    return {};  // No overlapping entries.
 
-  for (auto it = start_it; it != end_it; it++) {
-    JITCodeEntry* old_entry = (*it).second;
-    UnregisterCodeEntry(old_entry);
-    DestroyCodeEntry(old_entry);
-  }
+  return {{start_it, end_it}};
+}
 
-  map->erase(start_it, end_it);
+// Remove entries from the map that intersect the given address region,
+// and deregister them from GDB.
+static void RemoveJITCodeEntries(CodeMap* map,
+                                 const base::AddressRegion region) {
+  if (auto overlap = GetOverlappingRegions(map, region)) {
+    auto start_it = overlap->first;
+    auto end_it = overlap->second;
+    for (auto it = start_it; it != end_it; it++) {
+      JITCodeEntry* old_entry = (*it).second;
+      UnregisterCodeEntry(old_entry);
+      DestroyCodeEntry(old_entry);
+    }
+
+    map->erase(start_it, end_it);
+  }
 }
 
 // Insert the entry into the map and register it with GDB.
@@ -2075,6 +2097,23 @@ void EventHandler(const v8::JitCodeEvent* event) {
     }
   }
 }
+
+void AddRegionForTesting(const base::AddressRegion region) {
+  // For testing purposes we don't care about JITCodeEntry, pass nullptr.
+  auto result = GetCodeMap()->emplace(region, nullptr);
+  DCHECK(result.second);  // Insertion happened.
+  USE(result);
+}
+
+void ClearCodeMapForTesting() { GetCodeMap()->clear(); }
+
+size_t NumOverlapEntriesForTesting(const base::AddressRegion region) {
+  if (auto overlaps = GetOverlappingRegions(GetCodeMap(), region)) {
+    return std::distance(overlaps->first, overlaps->second);
+  }
+  return 0;
+}
+
 #endif
 }  // namespace GDBJITInterface
 }  // namespace internal

src/diagnostics/gdb-jit.h

@@ -5,6 +5,8 @@
 #ifndef V8_DIAGNOSTICS_GDB_JIT_H_
 #define V8_DIAGNOSTICS_GDB_JIT_H_
 
+#include "src/base/address-region.h"
+
 //
 // GDB has two ways of interacting with JIT code.  With the "JIT compilation
 // interface", V8 can tell GDB when it emits JIT code.  Unfortunately to do so,
@@ -29,9 +31,19 @@ struct JitCodeEvent;
 namespace internal {
 namespace GDBJITInterface {
 #ifdef ENABLE_GDB_JIT_INTERFACE
+
 // JitCodeEventHandler that creates ELF/Mach-O objects and registers them with
 // GDB.
 void EventHandler(const v8::JitCodeEvent* event);
+
+// Expose some functions for unittests. These only exercise the logic to add
+// AddressRegion to CodeMap, and checking for overlap. It does not touch the
+// actual JITCodeEntry at all.
+V8_EXPORT_PRIVATE void AddRegionForTesting(const base::AddressRegion region);
+V8_EXPORT_PRIVATE void ClearCodeMapForTesting();
+V8_EXPORT_PRIVATE size_t
+NumOverlapEntriesForTesting(const base::AddressRegion region);
+
 #endif
 }  // namespace GDBJITInterface
 }  // namespace internal

test/unittests/BUILD.gn

@@ -305,6 +305,7 @@ v8_source_set("unittests_sources") {
     "debug/debug-property-iterator-unittest.cc",
     "diagnostics/eh-frame-iterator-unittest.cc",
     "diagnostics/eh-frame-writer-unittest.cc",
+    "diagnostics/gdb-jit-unittest.cc",
     "execution/microtask-queue-unittest.cc",
     "heap/allocation-observer-unittest.cc",
     "heap/barrier-unittest.cc",

test/unittests/diagnostics/gdb-jit-unittest.cc

+// Copyright 2021 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.
+
+#include "src/diagnostics/gdb-jit.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace v8 {
+namespace internal {
+namespace GDBJITInterface {
+
+#ifdef ENABLE_GDB_JIT_INTERFACE
+TEST(GDBJITTest, OverlapEntries) {
+  ClearCodeMapForTesting();
+
+  base::AddressRegion ar{10, 10};
+  AddRegionForTesting(ar);
+
+  // Full containment.
+  ASSERT_EQ(1u, NumOverlapEntriesForTesting({11, 2}));
+  // Overlap start.
+  ASSERT_EQ(1u, NumOverlapEntriesForTesting({5, 10}));
+  // Overlap end.
+  ASSERT_EQ(1u, NumOverlapEntriesForTesting({15, 10}));
+
+  // No overlap.
+  // Completely smaller.
+  ASSERT_EQ(0u, NumOverlapEntriesForTesting({5, 5}));
+  // Completely bigger.
+  ASSERT_EQ(0u, NumOverlapEntriesForTesting({20, 10}));
+
+  AddRegionForTesting({20, 10});
+  // Now we have 2 code entries that don't overlap:
+  // [ entry 1 ][entry 2]
+  // ^ 10       ^ 20
+
+  // Overlap none.
+  ASSERT_EQ(0u, NumOverlapEntriesForTesting({0, 5}));
+  ASSERT_EQ(0u, NumOverlapEntriesForTesting({30, 5}));
+
+  // Overlap one.
+  ASSERT_EQ(1u, NumOverlapEntriesForTesting({15, 5}));
+  ASSERT_EQ(1u, NumOverlapEntriesForTesting({20, 5}));
+
+  // Overlap both.
+  ASSERT_EQ(2u, NumOverlapEntriesForTesting({15, 10}));
+  ASSERT_EQ(2u, NumOverlapEntriesForTesting({5, 20}));
+  ASSERT_EQ(2u, NumOverlapEntriesForTesting({15, 20}));
+  ASSERT_EQ(2u, NumOverlapEntriesForTesting({0, 40}));
+
+  ClearCodeMapForTesting();
+}
+#endif
+
+}  // namespace GDBJITInterface
+}  // namespace internal
+}  // namespace v8