Assertion if used with LLVM code base together with mimalloc-new-delete.h new/delete overwriting

[christoph-cullmann]

I compiled a program using the LLVM/clang libraries and overwrote the global new/delete/... operators via mimalloc-new-delete.h

This leads to an assert for debug builds for the aligned new variants (on both Linux & macOS)

mimalloc: assertion failed: at "/Volumes/makefactory/mimalloc-Add0B0VQ/mimalloc/src/src/alloc-aligned.c":20, mi_heap_malloc_zero_aligned_at
assertion: "alignment > 0 && alignment % sizeof(void*) == 0"

4 rules-cxx 0x0000000110480e6a _mi_assert_fail.cold.1 + 58
5 rules-cxx 0x00000001101245d2 _mi_assert_fail + 18
6 rules-cxx 0x000000011012168f mi_heap_malloc_zero_aligned_at + 79
7 rules-cxx 0x0000000110120bc2 mi_new_aligned + 34
8 rules-cxx 0x000000010ec5a269 _ZN4llvm8DenseMapIjN5clang17DiagnosticMappingENS_12DenseMapInfoIjEENS_6detail12DenseMapPairIjS2_EEE4growEj + 121
9 rules-cxx 0x000000010ec5a0ee ZN4llvm12DenseMapBaseINS_8DenseMapIjN5clang17DiagnosticMappingENS_12DenseMapInfoIjEENS_6detail12DenseMapPairIjS3_EEEEjS3_S5_S8_E20InsertIntoBucketImplIjEEPS8_RKjRKT_SC + 78
10 rules-cxx 0x000000010ec53e41 _ZN5clang17DiagnosticsEngine11setSeverityEjNS_4diag8SeverityENS_14SourceLocationE + 625
11 rules-cxx 0x000000010ec53fe2 _ZN5clang17DiagnosticsEngine19setSeverityForGroupENS_4diag6FlavorEN4llvm9StringRefENS1_8SeverityENS_14SourceLocationE + 130
12 rules-cxx 0x000000010ecc5f17 _ZN5clang21ProcessWarningOptionsERNS_17DiagnosticsEngineERKNS_17DiagnosticOptionsEb + 935
13 rules-cxx 0x000000010dbacf29 _ZN5clang16CompilerInstance17createDiagnosticsEPNS_17DiagnosticOptionsEPNS_18DiagnosticConsumerEbPKNS_14CodeGenOptionsE + 1785
14 rules-cxx 0x000000010dbac7e8 _ZN5clang16CompilerInstance17createDiagnosticsEPNS_18DiagnosticConsumerEb + 40

I assume the code that uses the aligned new has some data type that has lower alignment requirements than enforce by the assert.

e.g. the call here start with some

allocate_buffer(sizeof(BucketT) * NumBuckets, alignof(BucketT)));

call that passes the alignof result to the aligned new variants.

Is the assert overly strict?

[daanx]

Thanks for the report. The assert:

alignment > 0 && alignment % sizeof(void*) == 0

indicates that the alignment is either 0 or not a multiple of a machine word -- that is generally not good and I am surprised that happens since your alignof(BucketT) should not be like that? Can you figure out what alignof(BucketT) is? and if that is the value where the assertion fails?

• note: the standard for aligned_alloc says:

As an example of the "supported by the implementation" requirement, POSIX function
posix_memalign accepts any alignment that is a power of two and a multiple of sizeof(void*), and
POSIX-based implementations of aligned_alloc inherit this requirements.

so the assertion seems ok.

[christoph-cullmann]

I agree that you are right for the limitations that are allowed.

I investigated the crash on Linux:

#0 0x00007ffff7c5bce5 in raise () from /usr/lib/libc.so.6 #1 0x00007ffff7c45857 in abort () from /usr/lib/libc.so.6 #2 0x0000000005e87916 in _mi_assert_fail (assertion=0xb1d599 "alignment > 0 && alignment % sizeof(void*) == 0", fname=0xb1d5c9 "/local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/alloc-aligned.c", line=20, func=0xb9b056 "mi_heap_malloc_zero_aligned_at") at /local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/options.c:333 #3 0x0000000005e835f6 in mi_heap_malloc_zero_aligned_at (heap=0x667dd20 <_mi_heap_main>, size=512, alignment=4, offset=0, zero=false) at /local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/alloc-aligned.c:20 #4 0x0000000005e83580 in mi_heap_malloc_aligned_at (heap=0x667dd20 <_mi_heap_main>, size=512, alignment=4, offset=0) at /local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/alloc-aligned.c:67 #5 0x0000000005e83a49 in mi_heap_malloc_aligned (heap=0x667dd20 <_mi_heap_main>, size=512, alignment=4) at /local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/alloc-aligned.c:71 #6 0x0000000005e83c95 in mi_malloc_aligned (size=512, alignment=4) at /local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/alloc-aligned.c:97 #7 0x0000000005e712dd in mi_new_aligned (size=512, alignment=4) at /local/ssd/cullmann/build/astree.default.O0/mimalloc/src/src/alloc.c:799 #8 0x0000000005d7f83d in operator new (n=512, al=(unknown: 4)) at /local/ssd/cullmann/build/astree.default.O0/usr/include/mimalloc-new-delete.h:45 #9 0x0000000004986445 in llvm::DenseMap<unsigned int, clang::DiagnosticMapping, llvm::DenseMapInfo<unsigned int>, llvm::detail::DenseMapPair<unsigned int, clang::DiagnosticMapping> >::grow(unsigned int) () #10 0x00000000049862e0 in llvm::detail::DenseMapPair<unsigned int, clang::DiagnosticMapping>* llvm::DenseMapBase<llvm::DenseMap<unsigned int, clang::DiagnosticMapping, llvm::DenseMapInfo<unsigned int>, llvm::detail::DenseMapPair<unsigned int, clang::DiagnosticMapping> >, unsigned int, clang::DiagnosticMapping, llvm::DenseMapInfo<unsigned int>, llvm::detail::DenseMapPair<unsigned int, clang::DiagnosticMapping> >::InsertIntoBucketImpl<unsigned int>(unsigned int const&, unsigned int const&, llvm::detail::DenseMapPair<unsigned int, clang::DiagnosticMapping>*) () #11 0x00000000049827b7 in clang::DiagnosticsEngine::setSeverity(unsigned int, clang::diag::Severity, clang::SourceLocation) () #12 0x0000000004982872 in clang::DiagnosticsEngine::setSeverityForGroup(clang::diag::Flavor, llvm::StringRef, clang::diag::Severity, clang::SourceLocation) () #13 0x00000000049edd9f in clang::ProcessWarningOptions(clang::DiagnosticsEngine&, clang::DiagnosticOptions const&, bool) () #14 0x00000000039757c2 in clang::CompilerInstance::createDiagnostics(clang::DiagnosticOptions*, clang::DiagnosticConsumer*, bool, clang::CodeGenOptions const*) () #15 0x0000000003974f38 in clang::CompilerInstance::createDiagnostics(clang::DiagnosticConsumer*, bool) () #16 0x000000000324fc71 in CompilerDriver::runForUnit (this=0x7fffffffd508, compilationUnitGroup=..., compilationUnit=..., action=..., outputFile=...) at generic/compilerdriver.cpp:1172 #17 0x000000000324edf2 in CompilerDriver::run (this=0x7fffffffd508) at generic/compilerdriver.cpp:1014 #18 0x000000000321b3d5 in main (argc=2, argv=0x400000c0560) at generic/main.cpp:219

The alignment is 4. The type is some wrapped integer.

Thought, e.g. on neither Linux, macOS nor Windows the default allocators bail out with such an issue (the aligned variants are used there, too).

I would assume is is common to call the aligned variants in templates, like in this case and I think it would be a hassle to start to preach to the projects to guard them with type alignment conditions.

Other malloc implementations relaxed this restraint, too, it seems, to accommodate user space code used to the more relaxed behavior of the system libc allocators, see e.g.

https://bugzilla.mozilla.org/show_bug.cgi?id=526152
=> https://hg.mozilla.org/mozilla-central/rev/526ea23c8b21

Could such a relaxation be possible here, too?

[christoph-cullmann]

The trivial patch in the linked pull request will fix the issue like e.g. done for jemalloc.

[daanx]

I looked at the PR as well -- thanks so much. I think is is fixed in c9ffe30 on dev by just weakening the assertion. Let me know how it goes.

[christoph-cullmann]

Ok, I tried this again.

Many things work (like my initial unit test that failed before), but some other runs then hit the next assert later in the function.

I guess the unit test is lucky and uses the early out for 'small block found'.

The failing assert is the:

assertion: "adjust % sizeof(uintptr_t) == 0"

which makes sense, as this will not hold, if the alignment didn't have the sizeof(...) requirement.

[daanx]

Ah of course. I guess we should remove that assertion as well..

[daanx]

Just pushed a fix; hope it work now. :-)

[adamdmoss]

Just pushed a fix; hope it work now. :-)

I think that just went to dev-slice - dunno if that was intentional. :)

[christoph-cullmann]

I will redo my testing with the master version and adjusted asserts:

mi_assert(alignment > 0);
mi_assert_internal(adjust >= alignment);

[christoph-cullmann]

Hmm, actually, not even the unit tests of mimalloc pass with just these two changes:

Start 1: test_api,

1/2 Test #1: test_api, ........................Child aborted***Exception: 0.00 sec
test: malloc-zero... ok.
test: malloc-nomem1... mimalloc: error: cannot allocate memory (9223372036854775815 bytes requested)
ok.
test: malloc-null... ok.
test: calloc-overflow... mimalloc: error: allocation request too large (4392871920 * 18446744073709551 bytes)
ok.
test: calloc0... ok.
test: posix_memalign1... ok.
test: posix_memalign_no_align... ok.
test: posix_memalign_zero... ok.
test: posix_memalign_nopow2... ok.
test: posix_memalign_nomem... mimalloc: error: allocation request is too large (18446744073709551615 b requested)
mimalloc: error: allocation request is too large (18446744073709551615 b requested)
mimalloc: error: cannot allocate memory (7 bytes requested)
ok.
test: malloc-aligned1... ok.
test: malloc-aligned2... ok.
test: malloc-aligned3... ok.
test: malloc-aligned4... ok.
test: malloc-aligned5... ok.
test: malloc-aligned-at1... ok.
test: malloc-aligned-at2... mimalloc: assertion failed: at "/Volumes/makefactory/mimalloc-Y2u3FN6G/mimalloc/src/src/alloc-aligned.c":57, mi_heap_malloc_zero_aligned_at
assertion: "adjust >= alignment"

Start 2: test_stress,

2/2 Test #2: test_stress, ..................... Passed 2.24 sec

[christoph-cullmann]

Hmm, given the

uintptr_t adjust = alignment - (((uintptr_t)p + offset) & align_mask);

computation, when would adjust be ever > alignment?
I think without the alignment % sizeof(..) invariant one can just remove the assert for the adjust completely.

[daanx]

Ah, that was embarrassing -- fixed now I hope and pushed to dev with the correct mi_assert(adjust <= alignment), apologies!

(yes, it is a bit obvious this always holds but the philosophy of mimalloc is to always make invariants explicit and checked even if we can "obviously" derive them from the code. For an allocator i think this is the only way to get confidence in the code as unit test never cover enough cases)

[christoph-cullmann]

Ok, this should do the job for me, thanks for taking care!