Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mingw support (2022-1) #451

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Mingw support (2022-1) #451

wants to merge 3 commits into from

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Jan 2, 2022 •
edited

The old problem was addressed by introducing new way to register clean-up callbacks. However, there are still some works to do.
Signed-off-by: Schrodinger ZHU Yifan i@zhuyi.fan

@SchrodingerZhu
init mingw support
3b5acd2 
Signed-off-by: Schrodinger ZHU Yifan <i@zhuyi.fan>
@SchrodingerZhu SchrodingerZhu changed the title init mingw support Mingw support (2022-1) Jan 2, 2022
@SchrodingerZhu
format
5830dbd 
Signed-off-by: Schrodinger ZHU Yifan <i@zhuyi.fan>
@SchrodingerZhu
Copy link
Contributor Author

SchrodingerZhu commented Jan 2, 2022 •
edited

With MinGW UCRT64 runtime, perf-contention fails with the following stacktrace:

Thread 1 received signal ?, Unknown signal.
0x00007fff84f4e88e in ucrtbase!abort () from C:\WINDOWS\System32\ucrtbase.dll
(gdb) bt
#0  0x00007fff84f4e88e in ucrtbase!abort ()
   from C:\WINDOWS\System32\ucrtbase.dll
#1  0x00007ff69fb65930 in snmalloc::PALWindows::error (
    str=0x7ff69fb76fe0 <snmalloc::NUM_EPOCHS+1496> "assert fail: initialised in D:/snmalloc/src/mem/remotecache.h on 84")
    at D:/snmalloc/src/pal/pal_windows.h:125
#2  0x00007ff69fb6ccf5 in snmalloc::error (
    str=0x7ff69fb76fe0 <snmalloc::NUM_EPOCHS+1496> "assert fail: initialised in D:/snmalloc/src/mem/remotecache.h on 84") at D:/snmalloc/src/pal/pal.h:66
#3  0x00007ff69fb6a881 in snmalloc::RemoteDeallocCache::post<10368ull, snmalloc::Globals> (this=0x23e7ab9da40, local_state=0x23e0001a600,
    id=140697218265472, key=...) at D:/snmalloc/src/mem/remotecache.h:84
#4  0x00007ff69fb656bb in snmalloc::LocalCache::flush<10368ull, snmalloc::Globals, snmalloc::CoreAllocator<snmalloc::Globals>::flush(bool)::{lambda(snmalloc::CapPtr<void, snmalloc::capptr::bound<(snmalloc::capptr::dimension::Spatial)0, (snmalloc::capptr::dimension::AddressSpaceControl)0, (snmalloc::capptr::dimension::Wildness)1> >)#3}>(snmalloc::Globals::LocalState*, snmalloc::CoreAllocator<snmalloc::Globals>::flush(bool)::{lambda(snmalloc::CapPtr<void, snmalloc::capptr::bound<(snmalloc::capptr::dimension::Spatial)0, (snmalloc::capptr::dimension::AddressSpaceControl)0, (snmalloc::capptr::dimension::Wildness)1> >)#3}) (
    this=0x23e7ab9d898, local_state=0x23e0001a600, dealloc=...)
    at D:/snmalloc/src/mem/localcache.h:100
#5  0x00007ff69fb683f4 in snmalloc::CoreAllocator<snmalloc::Globals>::flush (
    this=0x23e00018000, destroy_queue=true)
    at D:/snmalloc/src/mem/corealloc.h:864
#6  0x00007ff69fb67586 in snmalloc::CoreAllocator<snmalloc::Globals>::debug_is_empty_impl (this=0x23e00018000, result=0xe0c4dff316)
    at D:/snmalloc/src/mem/corealloc.h:916
#7  0x00007ff69fb66983 in snmalloc::CoreAllocator<snmalloc::Globals>::debug_is_empty (this=0x23e00018000, result=0xe0c4dff316)
    at D:/snmalloc/src/mem/corealloc.h:964
#8  0x00007ff69fb632b6 in snmalloc::debug_check_empty<snmalloc::Globals> (
    result=0x0) at D:/snmalloc/src/mem/globalalloc.h:118
#9  0x00007ff69fb630e8 in test_tasks (num_tasks=8, count=1048576, size=262144)
    at D:/snmalloc/src/test/perf/contention/contention.cc:145
#10 0x00007ff69fb6320a in main (argc=1, argv=0x23e7ab6cf40)
    at D:/snmalloc/src/test/perf/contention/contention.cc:164

This is only reproducible under debug mode.

@davidchisnall
Copy link
Collaborator

Hi, most of these changes seem to be replacing stdio with iostreams. Can you pull those out into a separate PR? Are those things necessary for MinGW or just cleanups?

@mjp41
Copy link
Member

mjp41 commented Jan 4, 2022

Is it definitely using the PTHREAD_DESTRUCTORS for thread cleanup? It looks like it has not properly release the allocators back on thread teardown.

@SchrodingerZhu
more fixes
906d2cc 
Signed-off-by: Schrodinger ZHU Yifan <i@zhuyi.fan>
@SchrodingerZhu
Copy link
Contributor Author

Hi, most of these changes seem to be replacing stdio with iostreams. Can you pull those out into a separate PR? Are those things necessary for MinGW or just cleanups?

It is needed because mingw's gcc disagrees with %zu being a proper format flag.

@SchrodingerZhu
Copy link
Contributor Author

Is it definitely using the PTHREAD_DESTRUCTORS for thread cleanup? It looks like it has not properly release the allocators back on thread teardown.
image

unfortunately, the problem persists

@SchrodingerZhu
Copy link
Contributor Author

SchrodingerZhu commented Jan 5, 2022 •
edited

I added an output on each pthread-related function entrance:
image

davidchisnall
davidchisnall requested changes Mar 3, 2022
View reviewed changes
Copy link
Collaborator

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 90% of this PR is unnecessary after the latest refactorings, which should make it more maintainable going forward.

If it actually works, please can you add MinGW to CI as well?

CMakeLists.txt
@@ -186,6 +188,10 @@ if (WIN32)
message(STATUS "snmalloc: Avoiding Windows 10 APIs is ${WIN8COMPAT}")
endif()

if (MINGW)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you add a comment explaining what this is needed for (getrandom?)

src/override/memcpy.cc
@@ -15,7 +15,7 @@ using namespace snmalloc;
# define snprintf_l(buf, size, loc, msg, ...) \
snprintf(buf, size, msg, __VA_ARGS__)
// Windows has it with an underscore prefix
#elif defined(_MSC_VER)
#elif defined(_MSC_VER) || defined(__MINGW32__)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is gone now, please can you rebase the diff?

src/pal/pal_windows.h
@@ -13,7 +13,9 @@
# define NOMINMAX
# endif
# include <windows.h>
# pragma comment(lib, "bcrypt.lib")
# ifndef __MINGW32__
# pragma comment(lib, "bcrypt.lib")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're embedding something in the .lib to tell the linker to link bcrypt, then can we make bcrypt a PRIVATE library rather than an INTERFACE one in the cmake?

src/test/func/malloc/malloc.cc
@@ -15,23 +17,23 @@ void check_result(size_t size, size_t align, void* p, int err, bool null)
bool failed = false;
if (errno != err && err != SUCCESS)
{
printf("Expected error: %d but got %d\n", err, errno);
std::cout << "Expected error: " << err << " but got " << errno << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have all been replaced with the INFO and EXPECT macros. If there are any other places where they're necessary then please can you use the new formatting infrastructure rather than iostream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidchisnall davidchisnall davidchisnall requested changes

@mjp41 mjp41 Awaiting requested review from mjp41

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SchrodingerZhu @davidchisnall @mjp41