Make dn_simdhash OOM safe (#117059)

* Make simdhash add operations return a result enum instead of a 0/1 so they can signal OOM and whether an overwrite happened
* Make simdhash handle malloc failures during new and resize operations
* Explicitly NOMEM if the simdhash add fails due to an out of memory condition
* Add dn_simdhash_ght_try_insert
* Cleanup some erroneous uses of assert() instead of dn_simdhash_assert()
* Interpreter adjustments based on simdhash changes

Author: kg

src/coreclr/interpreter/compiler.h

@@ -9,52 +9,8 @@
 #include "datastructs.h"
 #include "enum_class_flags.h"
 #include <new>
-
-#include "../../native/containers/dn-simdhash.h"
-#include "../../native/containers/dn-simdhash-specializations.h"
-#include "../../native/containers/dn-simdhash-utils.h"
-
-class dn_simdhash_ptr_ptr_holder
-{
-    dn_simdhash_ptr_ptr_t *Value;
-public:
-    dn_simdhash_ptr_ptr_holder() :
-        Value(nullptr)
-    {
-    }
-
-    dn_simdhash_ptr_ptr_t* GetValue()
-    {
-        if (!Value)
-            Value = dn_simdhash_ptr_ptr_new(0, nullptr);
-        return Value;
-    }
-
-    dn_simdhash_ptr_ptr_holder(const dn_simdhash_ptr_ptr_holder&) = delete;
-    dn_simdhash_ptr_ptr_holder& operator=(const dn_simdhash_ptr_ptr_holder&) = delete;
-    dn_simdhash_ptr_ptr_holder(dn_simdhash_ptr_ptr_holder&& other)
-    {
-        Value = other.Value;
-        other.Value = nullptr;
-    }
-    dn_simdhash_ptr_ptr_holder& operator=(dn_simdhash_ptr_ptr_holder&& other)
-    {
-        if (this != &other)
-        {
-            if (Value != nullptr)
-                dn_simdhash_free(Value);
-            Value = other.Value;
-            other.Value = nullptr;
-        }
-        return *this;
-    }
-
-    ~dn_simdhash_ptr_ptr_holder()
-    {
-        if (Value != nullptr)
-            dn_simdhash_free(Value);
-    }
-};
+#include "failures.h"
+#include "simdhash.h"
 
 struct InterpException
 {
@@ -67,16 +23,6 @@ struct InterpException
     const CorJitResult m_result;
 };
 
-#if defined(__GNUC__) || defined(__clang__)
-#define INTERPRETER_NORETURN    __attribute__((noreturn))
-#else
-#define INTERPRETER_NORETURN    __declspec(noreturn)
-#endif
-
-INTERPRETER_NORETURN void NO_WAY(const char* message);
-INTERPRETER_NORETURN void BADCODE(const char* message);
-INTERPRETER_NORETURN void NOMEM();
-
 class InterpCompiler;
 
 class InterpDataItemIndexMap
@@ -540,15 +486,11 @@ class InterpCompiler
     dn_simdhash_ptr_ptr_holder m_pointerToNameMap;
     bool PointerInNameMap(void* ptr)
     {
-        if (dn_simdhash_ptr_ptr_try_get_value(m_pointerToNameMap.GetValue(), ptr, NULL))
-        {
-            return true;
-        }
-        return false;
+        return dn_simdhash_ptr_ptr_try_get_value(m_pointerToNameMap.GetValue(), ptr, NULL) != 0;
     }
     void AddPointerToNameMap(void* ptr, const char* name)
     {
-        dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name);
+        checkNoError(dn_simdhash_ptr_ptr_try_add(m_pointerToNameMap.GetValue(), ptr, (void*)name));
     }
     void PrintNameInPointerMap(void* ptr);
 #endif
@@ -865,7 +807,10 @@ int32_t InterpDataItemIndexMap::GetDataItemIndexForT(const T& lookup)
     VarSizedDataWithPayload<T>* pLookup = new(hashItemPayload) VarSizedDataWithPayload<T>();
     memcpy(&pLookup->payload, &lookup, sizeof(T));
 
-    dn_simdhash_ght_insert(hash, (void*)pLookup, (void*)(size_t)dataItemIndex);
+    checkAddedNew(dn_simdhash_ght_try_insert(
+        hash, (void*)pLookup, (void*)(size_t)dataItemIndex, DN_SIMDHASH_INSERT_MODE_ENSURE_UNIQUE
+    ));
+
     return dataItemIndex;
 }
 

src/coreclr/interpreter/failures.h

@@ -0,0 +1,17 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#ifndef _FAILURES_H_
+#define _FAILURES_H_
+
+#if defined(__GNUC__) || defined(__clang__)
+#define INTERPRETER_NORETURN    __attribute__((noreturn))
+#else
+#define INTERPRETER_NORETURN    __declspec(noreturn)
+#endif
+
+INTERPRETER_NORETURN void NO_WAY(const char* message);
+INTERPRETER_NORETURN void BADCODE(const char* message);
+INTERPRETER_NORETURN void NOMEM();
+
+#endif

src/coreclr/interpreter/interpreter.h

@@ -27,6 +27,7 @@
 
 #ifdef DEBUG
 extern "C" void assertAbort(const char* why, const char* file, unsigned line);
+
 #undef assert
 #define assert(p) (void)((p) || (assertAbort(#p, __FILE__, __LINE__), 0))
 #endif // DEBUG

src/coreclr/interpreter/simdhash.h

@@ -0,0 +1,72 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#ifndef _SIMDHASH_H_
+#define _SIMDHASH_H_
+
+#include "failures.h"
+#include "../../native/containers/dn-simdhash.h"
+#include "../../native/containers/dn-simdhash-specializations.h"
+#include "../../native/containers/dn-simdhash-utils.h"
+
+class dn_simdhash_ptr_ptr_holder
+{
+    dn_simdhash_ptr_ptr_t *Value;
+public:
+    dn_simdhash_ptr_ptr_holder() :
+        Value(nullptr)
+    {
+    }
+
+    dn_simdhash_ptr_ptr_t* GetValue()
+    {
+        if (!Value)
+            Value = dn_simdhash_ptr_ptr_new(0, nullptr);
+        return Value;
+    }
+
+    dn_simdhash_ptr_ptr_holder(const dn_simdhash_ptr_ptr_holder&) = delete;
+    dn_simdhash_ptr_ptr_holder& operator=(const dn_simdhash_ptr_ptr_holder&) = delete;
+    dn_simdhash_ptr_ptr_holder(dn_simdhash_ptr_ptr_holder&& other)
+    {
+        Value = other.Value;
+        other.Value = nullptr;
+    }
+    dn_simdhash_ptr_ptr_holder& operator=(dn_simdhash_ptr_ptr_holder&& other)
+    {
+        if (this != &other)
+        {
+            if (Value != nullptr)
+                dn_simdhash_free(Value);
+            Value = other.Value;
+            other.Value = nullptr;
+        }
+        return *this;
+    }
+
+    ~dn_simdhash_ptr_ptr_holder()
+    {
+        if (Value != nullptr)
+            dn_simdhash_free(Value);
+    }
+};
+
+// Asserts that no error occurred during a simdhash add, but does not mind if no new item was inserted
+inline void checkNoError(dn_simdhash_add_result result)
+{
+    if (result == DN_SIMDHASH_OUT_OF_MEMORY)
+        NOMEM();
+    else if (result < 0)
+        NO_WAY("Internal error in simdhash");
+}
+
+// Asserts that a new item was successfully inserted into the simdhash
+inline void checkAddedNew(dn_simdhash_add_result result)
+{
+    if (result == DN_SIMDHASH_OUT_OF_MEMORY)
+        NOMEM();
+    else if (result != DN_SIMDHASH_ADD_INSERTED)
+        NO_WAY("Failed to add new item into simdhash");
+}
+
+#endif // _SIMDHASH_H_

src/coreclr/interpreter/stackmap.cpp

@@ -6,15 +6,16 @@
 #include "interpreter.h"
 #include "stackmap.h"
 
-#include "../../native/containers/dn-simdhash.h"
-#include "../../native/containers/dn-simdhash-specializations.h"
-#include "../../native/containers/dn-simdhash-utils.h"
-
-extern "C" void assertAbort(const char* why, const char* file, unsigned line);
+#include "failures.h"
+#include "simdhash.h"
 
 void
 dn_simdhash_assert_fail (const char* file, int line, const char* condition) {
+#if DEBUG
     assertAbort(condition, file, line);
+#else
+    NO_WAY(condition);
+#endif
 }
 
 thread_local dn_simdhash_ptr_ptr_t *t_sharedStackMapLookup = nullptr;
@@ -24,10 +25,13 @@ InterpreterStackMap* GetInterpreterStackMap(ICorJitInfo* jitInfo, CORINFO_CLASS_
     InterpreterStackMap* result = nullptr;
     if (!t_sharedStackMapLookup)
         t_sharedStackMapLookup = dn_simdhash_ptr_ptr_new(0, nullptr);
+    if (!t_sharedStackMapLookup)
+        NOMEM();
+
     if (!dn_simdhash_ptr_ptr_try_get_value(t_sharedStackMapLookup, classHandle, (void **)&result))
     {
         result = new InterpreterStackMap(jitInfo, classHandle);
-        dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result);
+        checkAddedNew(dn_simdhash_ptr_ptr_try_add(t_sharedStackMapLookup, classHandle, result));
     }
     return result;
 }

src/native/containers/dn-simdhash-ght-compatible.c

@@ -69,9 +69,9 @@ dn_simdhash_ght_replaced (dn_simdhash_ght_data data, void * old_key, void * new_
 static unsigned int
 dn_simdhash_ght_default_hash (const void * key)
 {
-    // You might think we should avalanche the key bits but in my testing, it doesn't help.
-    // Right now the default hash function is rarely used anyway
-    return (unsigned int)(size_t)key;
+	// You might think we should avalanche the key bits but in my testing, it doesn't help.
+	// Right now the default hash function is rarely used anyway
+	return (unsigned int)(size_t)key;
 }
 
 static int32_t
@@ -87,6 +87,8 @@ dn_simdhash_ght_new (
 )
 {
 	dn_simdhash_ght_t *hash = dn_simdhash_new_internal(&DN_SIMDHASH_T_META, DN_SIMDHASH_T_VTABLE, capacity, allocator);
+	if (!hash)
+		return NULL;
 	// Most users of dn_simdhash_ght are passing a custom comparer, and always doing an indirect call ends up being faster
 	//  than conditionally doing a fast inline check when there's no comparer set. Somewhat counter-intuitive, but true
 	//  on both x64 and arm64. Probably due to the smaller code size and reduced branch predictor pressure.
@@ -107,6 +109,8 @@ dn_simdhash_ght_new_full (
 )
 {
 	dn_simdhash_ght_t *hash = dn_simdhash_new_internal(&DN_SIMDHASH_T_META, DN_SIMDHASH_T_VTABLE, capacity, allocator);
+	if (!hash)
+		return NULL;
 	if (!hash_func)
 		hash_func = dn_simdhash_ght_default_hash;
 	if (!key_equal_func)
@@ -133,7 +137,14 @@ dn_simdhash_ght_insert_replace (
 
 	dn_simdhash_insert_result ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, imode);
 	if (ok == DN_SIMDHASH_INSERT_NEED_TO_GROW) {
-		dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1);
+		uint8_t grow_ok;
+		dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1, &grow_ok);
+		// The ghashtable API doesn't have a way to signal failure, so just assert that we didn't fail to grow
+		dn_simdhash_assert(grow_ok);
+		// Don't attempt to insert after a failed grow (it's possible to get here because dn_simdhash_assert is configurable)
+		if (!grow_ok)
+			return;
+
 		if (old_buffers.buckets) {
 			DN_SIMDHASH_REHASH_INTERNAL(hash, old_buffers);
 			dn_simdhash_free_buffers(old_buffers);
@@ -151,11 +162,51 @@ dn_simdhash_ght_insert_replace (
 		case DN_SIMDHASH_INSERT_KEY_ALREADY_PRESENT:
 		case DN_SIMDHASH_INSERT_NEED_TO_GROW:
 		default:
-			assert(0);
+			dn_simdhash_assert(!"Internal error in dn_simdhash_ght_insert_replace");
 			return;
 	}
 }
 
+dn_simdhash_add_result
+dn_simdhash_ght_try_insert (
+	dn_simdhash_ght_t *hash,
+	void *key, void *value,
+	dn_simdhash_insert_mode mode
+)
+{
+	check_self(hash);
+	uint32_t key_hash = DN_SIMDHASH_KEY_HASHER(DN_SIMDHASH_GET_DATA(hash), key);
+
+	dn_simdhash_insert_result ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, mode);
+	if (ok == DN_SIMDHASH_INSERT_NEED_TO_GROW) {
+		uint8_t grow_ok;
+		dn_simdhash_buffers_t old_buffers = dn_simdhash_ensure_capacity_internal(hash, dn_simdhash_capacity(hash) + 1, &grow_ok);
+		if (!grow_ok)
+			return DN_SIMDHASH_OUT_OF_MEMORY;
+
+		if (old_buffers.buckets) {
+			DN_SIMDHASH_REHASH_INTERNAL(hash, old_buffers);
+			dn_simdhash_free_buffers(old_buffers);
+		}
+		ok = DN_SIMDHASH_TRY_INSERT_INTERNAL(hash, key, key_hash, value, mode);
+	}
+
+	switch (ok) {
+		case DN_SIMDHASH_INSERT_OK_ADDED_NEW:
+			hash->count++;
+			return DN_SIMDHASH_ADD_INSERTED;
+		case DN_SIMDHASH_INSERT_OK_OVERWROTE_EXISTING:
+			return DN_SIMDHASH_ADD_OVERWROTE;
+		case DN_SIMDHASH_INSERT_KEY_ALREADY_PRESENT:
+			return DN_SIMDHASH_ADD_FAILED;
+
+		case DN_SIMDHASH_INSERT_NEED_TO_GROW:
+		default:
+			dn_simdhash_assert(!"Internal error in dn_simdhash_ght_insert_replace");
+			return DN_SIMDHASH_INTERNAL_ERROR;
+	}
+}
+
 void *
 dn_simdhash_ght_get_value_or_default (
 	dn_simdhash_ght_t *hash, void * key

src/native/containers/dn-simdhash-ght-compatible.h

@@ -38,6 +38,13 @@ dn_simdhash_ght_get_value_or_default (
 	dn_simdhash_ght_t *hash, void * key
 );
 
+dn_simdhash_add_result
+dn_simdhash_ght_try_insert (
+	dn_simdhash_ght_t *hash,
+	void *key, void *value,
+	dn_simdhash_insert_mode mode
+);
+
 #ifdef __cplusplus
 }
 #endif // __cplusplus

src/native/containers/dn-simdhash-specialization-declarations.h

@@ -52,10 +52,10 @@ DN_SIMDHASH_T_PTR
 DN_SIMDHASH_NEW (uint32_t capacity, dn_allocator_t *allocator);
 #endif
 
-uint8_t
+dn_simdhash_add_result
 DN_SIMDHASH_TRY_ADD (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, DN_SIMDHASH_VALUE_T value);
 
-uint8_t
+dn_simdhash_add_result
 DN_SIMDHASH_TRY_ADD_WITH_HASH (DN_SIMDHASH_T_PTR hash, DN_SIMDHASH_KEY_T key, uint32_t key_hash, DN_SIMDHASH_VALUE_T value);
 
 // result is an optional parameter that will be set to the value of the item if it was found.