Closed Bug 1460720 Opened 3 years ago Closed 3 years ago

MinGW Build fails with --enable-jemalloc

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(1 file)

It looks like MinGW provides _aligned_malloc

> /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp: In function 'void* _recalloc(void*, size_t, size_t)':
> /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp:4885:1: warning: 'void* _recalloc(void*, size_t, size_t)' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
>  _recalloc(void* aPtr, size_t aCount, size_t aSize)
>  ^~~~~~~~~
> /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp: In function 'void* _expand(void*, size_t)':
> /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp:4912:1: warning: 'void* _expand(void*, size_t)' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
>  _expand(void* aPtr, size_t newsize)
>  ^~~~~~~
> /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp: In function 'size_t _msize(void*)':
> /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp:4922:1: warning: 'size_t _msize(void*)' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes]
>  _msize(void* aPtr)
>  ^~~~~~
> In file included from /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp:11:0:
> /builds/worker/workspace/build/src/memory/build/mozmemory_wrap.cpp: In function 'void* _aligned_malloc(size_t, size_t)':
> /builds/worker/workspace/build/src/memory/build/mozmemory_wrap.cpp:163:1: error: redefinition of 'void* _aligned_malloc(size_t, size_t)'
>  _aligned_malloc(size_t size, size_t alignment)
>  ^~~~~~~~~~~~~~~
> In file included from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/stdlib.h:741:0,
>                  from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/c++/6.4.0/cstdlib:75,
>                  from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/c++/6.4.0/stdlib.h:36,
>                  from /builds/worker/workspace/build/src/mingw32/lib/gcc/x86_64-w64-mingw32/6.4.0/include/mm_malloc.h:27,
>                  from /builds/worker/workspace/build/src/mingw32/lib/gcc/x86_64-w64-mingw32/6.4.0/include/xmmintrin.h:34,
>                  from /builds/worker/workspace/build/src/mingw32/lib/gcc/x86_64-w64-mingw32/6.4.0/include/x86intrin.h:33,
>                  from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/winnt.h:1554,
>                  from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/minwindef.h:163,
>                  from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/windef.h:8,
>                  from /builds/worker/workspace/build/src/mingw32/x86_64-w64-mingw32/include/windows.h:69,
>                  from /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp:115,
>                  from /builds/worker/workspace/build/src/obj-firefox/memory/build/Unified_cpp_memory_build0.cpp:2:
> /builds/worker/workspace/build/src/mingw32/lib/gcc/x86_64-w64-mingw32/6.4.0/include/mm_malloc.h:31:1: note: 'void* _aligned_malloc(size_t, size_t)' previously defined here
>  _mm_malloc (size_t __size, size_t __align)
>  ^
Comment on attachment 8974886 [details]
Bug 1460720 Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc

https://reviewboard.mozilla.org/r/243276/#review249110

That can't be the right fix, especially since we *do* have code that calls _aligned_malloc, which means they would be using the system allocator, but we'd be trying to free those buffers with jemalloc (through _aligned_free, which is redirected to jemalloc free through mozglue.def), and kaboom.
Attachment #8974886 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #3)
> Comment on attachment 8974886 [details]
> Bug 1460720 Do not declare _aligned_malloc for MinGW x64
> 
> https://reviewboard.mozilla.org/r/243276/#review249110
> 
> That can't be the right fix, especially since we *do* have code that calls
> _aligned_malloc, which means they would be using the system allocator, but
> we'd be trying to free those buffers with jemalloc (through _aligned_free,
> which is redirected to jemalloc free through mozglue.def), and kaboom.

So I confirmed that with that patch, when we call _aligned_malloc we wind up in msvcrt's implementation. I don't know why the browser doesn't kaboom though.

I figured out two ways to fix this:

1) Trick MinGW into not redefining _aligned_malloc and redireting it to gcc's mm_malloc implementation:
https://hg.mozilla.org/try/rev/e5a64b93c20f3202866bf3dcfd158df8bd54aec5

2) Same as wcsdup - don't define _aligned_malloc, but define a _aligned_malloc_impl that we alias (that's called aliasing right?) in mozglue.def

The second one is the one I submitted for review.
Comment on attachment 8974886 [details]
Bug 1460720 Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc

https://reviewboard.mozilla.org/r/243276/#review250204
Attachment #8974886 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8974886 [details]
Bug 1460720 Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc

[Approval Request Comment]

This is one of several MinGW Build patches I'd like to land in esr60 for Tor. It will prevent them from carrying their own patches for the lifetime of esr60 and will enable us to keep the MinGW build functioning and know if/when/how it was broken by new commits into esr60.

This commit does affect non-MinGW builds; I'm ensuring a try run is green. In general this change is pretty structural, so it's likely that if it broke anything we would know very quickly.
Attachment #8974886 - Flags: approval-mozilla-esr60?
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/144d50bc6891
Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/144d50bc6891
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment on attachment 8974886 [details]
Bug 1460720 Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc

mingw build fix, approved for 60.1esr
Attachment #8974886 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.