search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

DynamicLibraryTests fails on Windows when building with rpmalloc #46850

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR47881
Status NEW
Importance P enhancement
Reported by Hans Wennborg (hans@chromium.org)
Reported on 2020-10-16 07:41:24 -0700
Last modified on 2020-10-16 10:36:22 -0700
Version trunk
Hardware PC Linux
CC alex_toresh@yahoo.fr, htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also 
cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=llvm -
DLLVM_USE_CRT_RELEASE=MT -DLLVM_INTEGRATED_CRT_ALLOC=\src\rpmalloc ..\llvm &&
ninja DynamicLibraryTests.exe &&
unittests\Support\DynamicLibrary\DynamicLibraryTests.exe && echo OKAY || echo
FAIL

Fails

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS=llvm -
DLLVM_USE_CRT_RELEASE=MT ..\llvm && ninja DynamicLibraryTests.exe &&
unittests\Support\DynamicLibrary\DynamicLibraryTests.exe && echo OKAY || echo
FAIL

Succeeds.

(Note that the tests themselves appear to succeed, but the binary doesn't exit
cleanly.)

Looking at DynamicLibraryTest and PipSqueek, it kind of makes sense. Objects
are getting passed across the main exe and the PipSqueek dll, and when building
with rpmalloc, those will have different heaps.

(Interestingly, lit didn't notice until https://github.com/llvm/llvm-
project/commit/338dd138ea4a70b52ab48e0c8aa38ec152b3569a and I'm still not
entirely sure why.)
Quuxplusone commented 3 years ago

It would probably be possible to rewrite the test so it doesn't pass heap-allocated objects across, but for now I'll look into disabling it when using rpmalloc.

Quuxplusone commented 3 years ago

Disable the test in 95fb3542e8f195454ad4aa2290ca02906f5dfb78

Quuxplusone commented 3 years ago

Thanks for fixing it! This evidently used to fail with any other allocator (mimalloc, snmalloc, SCUDO). If we want this to work in the future, we'll need to pass around the rpmalloc/allocator context to the DLL init/shutdown.

Quuxplusone commented 3 years ago 
(In reply to Alexandre Ganea from comment #3)
> Thanks for fixing it! This evidently used to fail with any other allocator
> (mimalloc, snmalloc, SCUDO). If we want this to work in the future, we'll
> need to pass around the rpmalloc/allocator context to the DLL init/shutdown.

I didn't fix it, just disabled it :)

I think maybe the proper fix is to make the test not pass heap-allocated
objects across. I don't know if we care about making that work.
Quuxplusone commented 3 years ago 
Yeah sorry, I meant "fixing" as in disabling it :)

I never used Clang plugins, would that be a good reason to fix the test? Also
that could matter for ORC? One way to fix it would be do patch the calls to all
the Win32 Heap functions, like Hoard does, and replace with direct calls to the
allocator we're overwriting with:

https://github.com/emeryberger/Hoard/blob/master/src/source/wintls.cpp#L134
https://github.com/emeryberger/Malloc-Implementations/blob/17e52035d91f9f2e52e3303c8872ee29f5b5a7c5/Heap-Layers/wrappers/winwrapper.cpp#L283
https://github.com/emeryberger/Malloc-Implementations/blob/17e52035d91f9f2e52e3303c8872ee29f5b5a7c5/Heap-Layers/wrappers/winwrapper.cpp#L257
Quuxplusone commented 3 years ago 
(In reply to Alexandre Ganea from comment #5)
> I never used Clang plugins, would that be a good reason to fix the test?
> Also that could matter for ORC?

In Chromium we always link our plugins into clang.exe rather than load them
dynamically. I don't have any experience with ORC.

I'm not sure if supporting custom allocators and passing objects across dlls is
an important use case.