1460720 - MinGW Build fails with --enable-jemalloc

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

This fixes the build and results in a browser that runs.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4772264790ada28b9f3e4859a9f5def2d0363300&selectedJob=177951191

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1460720 Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc

Review commit: https://reviewboard.mozilla.org/r/243276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243276/

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 8974886 [details]
Bug 1460720 Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243276/diff/1-2/

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(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 6

[Mike Hommey [:glandium]]

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

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/144d50bc6891
Do not define _aligned_malloc - instead define _aligned_malloc_impl and export _aligned_malloc r=glandium

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/144d50bc6891

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/562dc783be1a