Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Assert failed in _mi_segments_collect #1031

Closed
MaxwellGengYF opened this issue Mar 9, 2025 · 13 comments
Closed

Assert failed in _mi_segments_collect #1031

MaxwellGengYF opened this issue Mar 9, 2025 · 13 comments

Comments

@MaxwellGengYF
Copy link

The assert

mi_assert_internal(tld->pages_purge.first == NULL);

failed during function "_mi_segments_collect" called by "mi_heap_collect_ex" in heap.c :161, this only happened in the latest version 1.9.3.

@felixsych
Copy link

Same here:

assertion failed: at "segment.c":527, _mi_segments_collect
assertion: "tld->pages_purge.first == NULL"

@danleh
Copy link

danleh commented Mar 18, 2025

Same here, see WebAssembly/binaryen#7378 (comment) and https://github.com/WebAssembly/binaryen/actions/runs/13925295048/job/38975354336?pr=7378#step:10:2629

mimalloc: assertion failed: at "/src/third_party/mimalloc/src/segment.c":527, _mi_segments_collect
assertion: "tld->pages_purge.first == NULL"

I tried v2.1.7 instead, which doesn't trip that assertion, but gives warnings such as

mimalloc: warning: thread 0x7F9E734006C0: unable to allocate aligned OS memory directly, fall back to over-allocation (size: 0x40000000 bytes, address: 0x7F9E24600000, alignment: 0x2000000, commit: 1)

Interestingly, when I don't link statically (which is what we try in WebAssembly/binaryen#7378) but instead replace the allocator via LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libmimalloc.so.2 from the Ubuntu repos, I don't get said warning.

Could it be that I am missing some option related to pthreads when building/linking mimalloc as a static library? Since this assertion is behind a if (!_mi_is_main_thread()):

if (!_mi_is_main_thread()) {

@daanx
Copy link
Collaborator

daanx commented Mar 19, 2025

Strange! I cannot repro and not sure what is the cause. Can you repro with v1.8.9 as well? If you can break on it, can you check if force was true or not? btw. this assert failing is not really an issue (although I would like to understand why it fails)

@danleh
Copy link

danleh commented Mar 19, 2025

Repro instructions of the assertion failure on x64 Linux with mimalloc dev:

# Clone branch that statically links mimalloc.
git clone https://github.com/danleh/binaryen.git
cd binaryen
git submodule init
git submodule update

# Switch to affected mimalloc version.
cd third_party/mimalloc/
git checkout dev
cd ../../

# Build with statically linked libc and mimalloc.
cmake . -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DMIMALLOC_STATIC=ON
make -j32 wasm-opt

# Check that it is indeed statically linked and contains mimalloc.
ldd bin/wasm-opt
>        not a dynamic executable
MIMALLOC_VERBOSE=1 bin/wasm-opt --version
> mimalloc: process init: 0x204F0C40
> [...]

# Optimize a medium size binary with wasm-opt (.wasm file attached as a zip), crashes due to the assertion failure:
bin/wasm-opt -all --closed-world --inline-functions-with-loops --traps-never-happen --fast-math --type-ssa -O3 -O3 --gufa -O3 --type-merging -O3 -Oz compose-benchmarks-wasm-js.wasm -o /dev/null 
> mimalloc: assertion failed: at "/usr/local/google/home/dlehmann/binaryen/third_party/mimalloc/src/segment.c":527, _mi_segments_collect
>   assertion: "tld->pages_purge.first == NULL"
> Aborted (core dumped)

Input .wasm file for the last step compose-benchmarks-benchmarks-wasm-js.zip

With v1.8.9 (i.e., rebuilding after cd third_party/mimalloc; git checkout v1.8.9), I cannot reproduce the assertion failure.

@danleh
Copy link

danleh commented Mar 19, 2025

When building mimalloc, are these lines problematic/surprising: Performing Test mi_has_libpthread - Failed, Performing Test mi_has_librt - Failed, and Performing Test mi_has_libatomic - Failed?

See output of the cmake step above (cmake . -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DMIMALLOC_STATIC=ON):

[...]
-- Override standard malloc (MI_OVERRIDE=ON)
-- Use the C++ compiler to compile (MI_USE_CXX=ON)
-- Performing Test mi_has_libpthread
-- Performing Test mi_has_libpthread - Failed
-- Performing Test mi_has_librt
-- Performing Test mi_has_librt - Failed
-- Performing Test mi_has_libatomic
-- Performing Test mi_has_libatomic - Failed
-- 
-- Library base name: mimalloc
-- Version          : 2.1
-- Build type       : release
-- C++ Compiler     : /usr/bin/c++
-- Compiler flags   : -Wall;-Wextra;-Wno-unknown-pragmas;-fvisibility=hidden;-ftls-model=initial-exec;-fno-builtin-malloc
-- Compiler defines : 
-- Link libraries   : 
-- Build targets    : static
[...]

Also, there are a bunch of compiler warnings related to atomic loads, what's up with those?

In file included from /usr/include/c++/14/bits/shared_ptr_atomic.h:33,
                 from /usr/include/c++/14/memory:81,
                 from /usr/local/google/home/dlehmann/experiments/2025-03-19-mimalloc-assertion/binaryen/third_party/mimalloc/include/mimalloc.h:493,
                 from /usr/local/google/home/dlehmann/experiments/2025-03-19-mimalloc-assertion/binaryen/third_party/mimalloc/src/heap.c:8:
In member function ‘std::__atomic_base<_IntTp>::__int_type std::__atomic_base<_IntTp>::load(std::memory_order) const [with _ITp = long unsigned int]’,
    inlined from ‘std::__atomic_base<_IntTp>::operator __int_type() const [with _ITp = long unsigned int]’ at /usr/include/c++/14/bits/atomic_base.h:361:20,
    inlined from ‘bool mi_heap_page_is_valid(mi_heap_t*, mi_page_queue_t*, mi_page_t*, void*, void*)’ at /usr/local/google/home/dlehmann/experiments/2025-03-19-mimalloc-assertion/binaryen/third_party/mimalloc/src/heap.c:62:3,
    inlined from ‘bool mi_heap_page_collect(mi_heap_t*, mi_page_queue_t*, mi_page_t*, void*, void*)’ at /usr/local/google/home/dlehmann/experiments/2025-03-19-mimalloc-assertion/binaryen/third_party/mimalloc/src/heap.c:95:3,
    inlined from ‘bool mi_heap_visit_pages(mi_heap_t*, bool (*)(mi_heap_t*, mi_page_queue_t*, mi_page_t*, void*, void*), void*, void*)’ at /usr/local/google/home/dlehmann/experiments/2025-03-19-mimalloc-assertion/binaryen/third_party/mimalloc/src/heap.c:46:14,
    inlined from ‘void mi_heap_collect_ex(mi_heap_t*, mi_collect_t)’ at /usr/local/google/home/dlehmann/experiments/2025-03-19-mimalloc-assertion/binaryen/third_party/mimalloc/src/heap.c:162:22:
/usr/include/c++/14/bits/atomic_base.h:501:31: warning: ‘long unsigned int __atomic_load_8(const volatile void*, int)’ writing 8 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
  501 |         return __atomic_load_n(&_M_i, int(__m));
      |                ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
In function ‘void mi_heap_collect_ex(mi_heap_t*, mi_collect_t)’:
cc1plus: note: destination object is likely at address zero

@danleh
Copy link

danleh commented Mar 19, 2025

Is GCC supported? By default I build with

$ gcc --version
gcc (Debian 14.2.0-3+build4) 14.2.0
Copyright (C) 2024 Free Software Foundation, Inc.

but if I switch to Clang, the failing library checks go away, as do the atomic load warnings:

$ CC=/usr/bin/clang CXX=/usr/bin/clang++ cmake . -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DMIMALLOC_STATIC=ON
[...]
-- Override standard malloc (MI_OVERRIDE=ON)
-- Use the C++ compiler to compile (MI_USE_CXX=ON)
-- Performing Test mi_has_libpthread
-- Performing Test mi_has_libpthread - Success
-- Performing Test mi_has_librt
-- Performing Test mi_has_librt - Success
-- Performing Test mi_has_libatomic
-- Performing Test mi_has_libatomic - Success
-- 
-- Library base name: mimalloc
-- Version          : 2.1
-- Build type       : release
-- C++ Compiler     : /usr/bin/clang++
-- Compiler flags   : -Wall;-Wextra;-Wpedantic;-Wno-deprecated;-Wno-unknown-pragmas;-fvisibility=hidden;-Wno-static-in-inline;-ftls-model=initial-exec;-fno-builtin-malloc
-- Compiler defines : 
-- Link libraries   : pthread;rt;atomic
-- Build targets    : static
[...]

@daanx
Copy link
Collaborator

daanx commented Mar 19, 2025

Hi @danleh -- thanks for all the feedback! Looking into it.

  • I replied in the other issue about -DNDEBUG -- strange that is not happening with your cmake?
  • the failing test for pthread library that works with clang but not gcc is very strange as it happens in the cmake file (see CMakeFiles.txt:find_link_library). What specific OS / gcc/ clang versions are you using? (I tested on arm64/macOS/gcc 14 and it works)
  • I would probably recommend using dev3 going forward (hoping to put out a fresh release 3.0.3 this week).
  • The atomic warning is strange -- maybe because of the cast from uintptr_t to mi_heap_t* -- it might be a gcc specific thing? I tried on arm64/macOS/gcc 14 and there is no warning (just the visibility one).

tbc.

@daanx
Copy link
Collaborator

daanx commented Mar 19, 2025

Hi @danleh -- I tried to repro but when I clone the binaryen repository as above, there is no thirdparty/mimalloc directory -- is it on another branch the main?

@kripken
Copy link
Contributor

kripken commented Mar 19, 2025

@daanx it landed just a few hours ago on main, but it is there now - you may need to pull main.

(can see it here: https://github.com/WebAssembly/binaryen/tree/main/third_party )

@danleh
Copy link

danleh commented Mar 20, 2025

My bad, the repro instructions in the comment above were missing a checkout of the mimalloc-static branch. Note that instructions clone from danleh/binaryen - a fork that contains said branch. It then should have the third_party/mimalloc directory.

daanx added a commit that referenced this issue Mar 20, 2025
daanx added a commit that referenced this issue Mar 20, 2025
@daanx
Copy link
Collaborator

daanx commented Mar 20, 2025

It is fixed now. The assertion was wrong -- it is only true if force is set. Normally it wouldn't trigger but in the latest dev (v1.9.2) there is more intermittent purging of memory with force being false. Hope to do a fresh release of mimalloc soon that will contain the fix.

@daanx daanx closed this as completed Mar 20, 2025
@danleh
Copy link

danleh commented Mar 20, 2025

Awesome, thanks a lot! (and for fixing the visibility warning as well :) )

@danleh
Copy link

danleh commented Mar 20, 2025

And following up on your earlier questions in #1031 (comment)

  • the failing test for pthread library that works with clang but not gcc is very strange as it happens in the cmake file (see CMakeFiles.txt:find_link_library). What specific OS / gcc/ clang versions are you using? (I tested on arm64/macOS/gcc 14 and it works)

I am compiling on a Linux Debian Testing derivate, x64, gcc 14. However, the issue seems to be the integration with binaryen's build again. The problem was that binaryen adds -fno-rtti to CMAKE_C_FLAGS (which is a C++-only flag), which produces a warning in the C compiler, which causes CMakes check_linker_flag to fail (used by mimalloc). Changing binaryen's build scripts to not add -fno-rtti to CMAKE_C_FLAGS fixes the issue.

  • I would probably recommend using dev3 going forward (hoping to put out a fresh release 3.0.3 this week).
  • The atomic warning is strange -- maybe because of the cast from uintptr_t to mi_heap_t* -- it might be a gcc specific thing? I tried on arm64/macOS/gcc 14 and there is no warning (just the visibility one).

The warnings are gone with any of the dev, dev2, dev3, or v3.0.2-beta branches; but are present with master, v1.9.2, and v2.2.2. So looking forward to a new release and will switch to that once available.

kripken pushed a commit to WebAssembly/binaryen that referenced this issue Mar 20, 2025
And some minor drive-by fixes: gitignore Ninja in-tree build file,
update mimalloc to latest stable release.

This gets rid of warnings in the mimalloc cmake/make step. See
microsoft/mimalloc#1031 (comment)
and microsoft/mimalloc#1038.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants