From 46192495c66b6a918c4fafbdd680f9577e84c173 Mon Sep 17 00:00:00 2001 From: Scott Roy Date: Tue, 23 Jun 2026 17:05:51 -0700 Subject: [PATCH] Remove low-signal FileDataLoader allocation-failure tests Summary: D109072812 added two allocation-failure unit tests to `file_data_loader_test.cpp` -- `AllocationFailureDuringLoadReturnsError` and `AllocationFailureDuringFromReturnsError` -- driven by a strong global replacement of `operator new(size_t, std::align_val_t, const std::nothrow_t&)` that returns nullptr on demand. That override broke the fbcode build (see T276843331): fbcode test binaries link jemalloc, which provides its own strong definition of the same operator-new symbol, so the second strong definition produces an `ld.lld: error: duplicate symbol` link failure (it likewise clashes with the ASAN allocator runtime). The original diff only validated the xplat/Apple target, where libc++ provides the operator-new family as weak symbols, so the breakage was not caught. These tests are also low signal: they only exercise the pre-existing `nullptr -> Error::MemoryAllocationFailed` guard, and because they replace the allocator wholesale they never actually exercise the production fix from D109072812 (switching `et_aligned_alloc` to the nothrow `operator new`). Covering that trivial guard is not worth a permanent global-operator-new override and its cross-platform build fragility. This change removes both tests and the operator-new override, reverting only the test-file additions from D109072812. The production fix from that diff -- using the nothrow `operator new` in `et_aligned_alloc` -- is retained. Applied to both the fbcode and xplat mirror copies of `file_data_loader_test.cpp`. Differential Revision: D109506462 --- .../test/file_data_loader_test.cpp | 95 ------------------- 1 file changed, 95 deletions(-) diff --git a/extension/data_loader/test/file_data_loader_test.cpp b/extension/data_loader/test/file_data_loader_test.cpp index bcf17e4afee..7dc872995a5 100644 --- a/extension/data_loader/test/file_data_loader_test.cpp +++ b/extension/data_loader/test/file_data_loader_test.cpp @@ -8,9 +8,7 @@ #include -#include #include -#include #include @@ -27,59 +25,6 @@ using executorch::runtime::Error; using executorch::runtime::FreeableBuffer; using executorch::runtime::Result; -namespace { -// When set, the replacement nothrow aligned operator new below returns nullptr, -// simulating an allocation failure without needing a real OOM. -std::atomic g_fail_aligned_nothrow_alloc{false}; - -// RAII guard to ensure flag is reset even if test asserts early. -struct FailAllocGuard { - FailAllocGuard() { - g_fail_aligned_nothrow_alloc.store(true, std::memory_order_relaxed); - } - ~FailAllocGuard() { - g_fail_aligned_nothrow_alloc.store(false, std::memory_order_relaxed); - } -}; -} // namespace - -// Detect ASAN to avoid multiple definition link error and to skip test when -// ASAN runtime provides its own strong operator new. -#if defined(__SANITIZE_ADDRESS__) || \ - (defined(__has_feature) && __has_feature(address_sanitizer)) -#define ET_TEST_ASAN_ENABLED 1 -#else -#define ET_TEST_ASAN_ENABLED 0 -#endif - -#if !ET_TEST_ASAN_ENABLED -// Replaces the global nothrow aligned allocation function for this test binary -// so FileDataLoader's segment allocation can be made to fail on demand. When -// the toggle is off it forwards to the real aligned allocator. We call the -// throwing aligned new inside a try/catch and convert exceptions to nullptr -// to emulate nothrow semantics without recursing into this same nothrow -// overload (calling ::operator new(size, alignment, std::nothrow) here would -// infinite-loop). Memory allocated here is released through the default -// operator delete, which is not replaced. -// This is a strong (non-weak) replacement so it reliably overrides libc++'s -// default on all platforms (a weak definition loses to libc++'s own weak -// definition on Apple's linker, leaving the override silently unused). Under -// ASAN this whole block is excluded so it can't clash with ASAN's allocator. -void* operator new( - std::size_t size, - std::align_val_t alignment, - const std::nothrow_t& /* tag */) noexcept { - if (g_fail_aligned_nothrow_alloc.load(std::memory_order_relaxed)) { - return nullptr; - } - try { - return ::operator new(size, alignment); - } catch (...) { - return nullptr; - } -} -#endif // !ET_TEST_ASAN_ENABLED - class FileDataLoaderTest : public ::testing::TestWithParam { protected: void SetUp() override { @@ -202,46 +147,6 @@ TEST_P(FileDataLoaderTest, OutOfBoundsLoadFails) { } } -#if !ET_TEST_ASAN_ENABLED -TEST_P(FileDataLoaderTest, AllocationFailureDuringLoadReturnsError) { - // Create a temp file; contents don't matter. - uint8_t data[256] = {}; - TempFile tf(data, sizeof(data)); - - Result fdl = - FileDataLoader::from(tf.path().c_str(), alignment()); - ASSERT_EQ(fdl.error(), Error::Ok); - - // Force the segment allocation inside load() to fail. The loader must surface - // Error::MemoryAllocationFailed rather than letting std::bad_alloc escape, - // which would abort the process in the exception-free runtime. - FailAllocGuard fail_guard; - Result fb = fdl->load( - /*offset=*/0, - /*size=*/sizeof(data), - DataLoader::SegmentInfo(DataLoader::SegmentInfo::Type::Program)); - - EXPECT_EQ(fb.error(), Error::MemoryAllocationFailed); -} -#endif // !ET_TEST_ASAN_ENABLED - -#if !ET_TEST_ASAN_ENABLED -TEST_P(FileDataLoaderTest, AllocationFailureDuringFromReturnsError) { - // Create a temp file; contents don't matter. - uint8_t data[256] = {}; - TempFile tf(data, sizeof(data)); - - // Force the filename allocation inside from() to fail. FileDataLoader::from - // copies the filename using et_aligned_alloc and must return - // Error::MemoryAllocationFailed on nullptr rather than throwing. - FailAllocGuard fail_guard; - Result fdl = - FileDataLoader::from(tf.path().c_str(), alignment()); - - EXPECT_EQ(fdl.error(), Error::MemoryAllocationFailed); -} -#endif // !ET_TEST_ASAN_ENABLED - TEST_P(FileDataLoaderTest, FromMissingFileFails) { // Wrapping a file that doesn't exist should fail. Result fdl = FileDataLoader::from(