-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Make dn_simdhash OOM safe #117059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Make dn_simdhash OOM safe #117059
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates dn_simdhash
to handle allocation failures by returning error codes rather than asserting or leaking memory. Key changes include:
- Introducing
dn_simdhash_add_result
to report add operation outcomes including OOM. - Changing
ensure_capacity_internal
andensure_capacity
to return a success flag. - Propagating and asserting on OOM in all call sites (specializations, ght-compatible, and the CoreCLR stackmap).
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
run-benchmark.ps1 | Added dn-simdhash-ght-compatible.c to the compile command |
src/native/containers/dn-simdhash.h | Added dn_simdhash_add_result enum and updated capacity APIs |
src/native/containers/dn-simdhash.c | Checked malloc return values and propagated OOM in constructors and capacity logic |
src/native/containers/dn-simdhash-specialization.h | Changed TRY_ADD functions to return dn_simdhash_add_result |
src/native/containers/dn-simdhash-specialization-declarations.h | Updated declarations to use dn_simdhash_add_result |
src/native/containers/dn-simdhash-ght-compatible.c | Added allocation checks and assertions for ght-compatible API |
src/coreclr/interpreter/stackmap.cpp | Assert on creation and add return codes in the shared stack map |
Comments suppressed due to low confidence (3)
src/native/containers/dn-simdhash.h:56
- Add unit tests that simulate allocation failures to verify that
DN_SIMDHASH_OUT_OF_MEMORY
is correctly returned from both add andensure_capacity
operations.
typedef enum dn_simdhash_add_result {
src/native/containers/dn-simdhash.h:56
- [nitpick] Include comments explaining each
dn_simdhash_add_result
enumerator value (e.g.,DN_SIMDHASH_ADD_FAILED
vsDN_SIMDHASH_OUT_OF_MEMORY
) to clarify their distinct semantics for API consumers.
typedef enum dn_simdhash_add_result {
src/native/containers/simdhash-benchmark/run-benchmark.ps1:1
- The added line has an unexpected leading '1 ' before the '+'. It should start with '+cl ...' without the extra '1 ' to correctly include the new source file in the compile command.
cl /GS- /O2 /std:c17 ./*.c ../dn-simdhash-u32-ptr.c ../dn-simdhash.c ../dn-vector.c ../dn-simdhash-string-ptr.c ../dn-simdhash-ght-compatible.c /DNO_CONFIG_H /DSIZEOF_VOID_P=8 /DNDEBUG /Fe:all-measurements.exe
Updated to add a (speculative) |
Tagging subscribers to this area: @dotnet/interop-contrib |
… they can signal OOM and whether an overwrite happened Make simdhash handle malloc failures during new and resize operations Fix some zeros where NULL would do Move side effect out of assert 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() Address PR feedback Cleanup simdhash failure handling Cleanup Fix CI warnings
Since we had a conflict in the compiler code I took the opportunity to refactor a bit, there's a failures.h where NOMEM lives now (so files other than compiler can use it) and a simdhash.h that contains the ptr_ptr_holder and result check helpers. |
Like its predecessor ghashtable, dn_simdhash was not written to handle malloc failures. Thanks to @davidwrighton for reminding me that this needed to be fixed.
Note that the 'ght compatible' version of simdhash asserts on malloc failure, since there's no way to retrofit it to allow the caller to do anything else - all the existing call sites in mono would become wrong if we did that.
The 'new' operation now returns NULL on malloc failure.
The add operations now return an enum where a positive value indicates success, a negative value indicates OOM, and a 0 indicates failure.
The ensure capacity operation now returns a success bool, where 0 means malloc failed and 1 means that the hash is big enough to hold the capacity you requested.
This PR also adds the new
dn_simdhash_ght_try_insert
API which allows you to sense OOM when using the ght compatible variant.