1420355 - Allow to statically link replace-malloc libraries

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Don't link logalloc against the static runtime on Windows.

This was never strictly required (for instance, DMD doesn't do that),
and would make things harder with the subsequent changes.

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Don't declare replace_* functions in replace_malloc.h.

The original purpose of those declarations was to avoid the function
definitions being wrong, as well as forcing them being exported
properly (as extern "C", as weak symbols when necessary, etc.), but:

- The implementations being C++, function overloads simply allowed
  functions with the same name to have a different signature.

- As of bug 1420353, the functions don't need to be exported anymore,
  nor do we care whether their symbols are mangled. Furthermore, they're
  now being assigned to function table fields, meaning there is type
  checking in place, now.

So all in all, these declarations can be removed.

Also, as further down the line we're going to statically link the
replace-malloc libraries, avoid symbol conflicts by making those
functions static.

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Remove fallbacks when DMD is initializing.

As of bug 1420353, DMD's replace_* functions can't be called before
replace_init places them in the malloc function table, which only
happens after DMD::Init has run, meaning DMD is always initialized
by the time any of its replace_* function can be called.

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Don't initialize logalloc if MALLOC_LOG is not given.

Now that replace_init can opt-out of registering the replace-malloc
functions, don't do so when MALLOC_LOG was not set in the environment.

While one would normally set MALLOC_LOG alongside one of the environment
variable necessary to load the replace-malloc library, we're also going,
in a subsequent change, to allow statically linking replace-malloc
libraries, taking full advantage of this change.

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Don't initialize DMD if the DMD environment variable is not given.

This makes things slightly more inconvenient (having to set two
environmnt variables instead of one for the simplest case) until a few
patches down the line, when DMD is statically linked, at which point it
will get down to one environment variable every time.

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Allow to statically link replace-malloc libraries.

And statically link logalloc.

Statically linking is the default, except when building with
--enable-project=memory, allowing to use the generated libraries from
such builds with Firefox.

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

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1420355 - Statically link DMD.

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

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8932388 [details]
Bug 1420355 - Don't link logalloc against the static runtime on Windows.

https://reviewboard.mozilla.org/r/203416/#review209168

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8932389 [details]
Bug 1420355 - Don't declare replace_* functions in replace_malloc.h.

https://reviewboard.mozilla.org/r/203418/#review209170

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8932390 [details]
Bug 1420355 - Remove fallbacks when DMD is initializing.

https://reviewboard.mozilla.org/r/203420/#review209172

Nice.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8932391 [details]
Bug 1420355 - Don't initialize logalloc if MALLOC_LOG is not given.

https://reviewboard.mozilla.org/r/203422/#review209174

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8932392 [details]
Bug 1420355 - Don't initialize DMD if the DMD environment variable is not given.

https://reviewboard.mozilla.org/r/203424/#review209176

::: commit-message-c6b2a:4
(Diff revision 1)
> +Bug 1420355 - Don't initialize DMD if the DMD environment variable is not given. r?njn
> +
> +This makes things slightly more inconvenient (having to set two
> +environmnt variables instead of one for the simplest case) until a few

typo: environmnt

::: memory/replace/dmd/DMD.cpp:1565
(Diff revision 1)
> +  }
> +  // Initialize the function table first, because StatusMsg uses
> +  // InfallibleAllocPolicy::malloc_, which uses it.
>    gMallocTable = *aMallocTable;
> +
> +  StatusMsg("$DMD = '%s'\n", e);

It would be nice to have a StatusMsg here, something like "$DMD is undefined; DMD will not run".

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8932393 [details]
Bug 1420355 - Allow to statically link replace-malloc libraries.

https://reviewboard.mozilla.org/r/203426/#review209192

::: build/moz.configure/memory.configure:70
(Diff revision 1)
> +
> +
> +@depends(replace_malloc, build_project)
> +def replace_malloc_static(replace_malloc, build_project):
> +    # Default to statically linking replace-malloc libraries that can be
> +    # statically linked, except when building with --enable-project=memory

Nit: '.' at end of sentence.

::: memory/build/mozjemalloc.cpp:4913
(Diff revision 1)
>    gReplaceMallocInitialized = true;
>    if (replace_init) {
>      replace_init(&gReplaceMallocTable, &gReplaceMallocBridge);
>    }
> +#ifdef MOZ_REPLACE_MALLOC_STATIC
> +  if (initialTable == gReplaceMallocTable) {

I wonder if using a method name like Equals() would be better. It's easy to misread this as a pointer equality.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8932394 [details]
Bug 1420355 - Statically link DMD.

https://reviewboard.mozilla.org/r/203428/#review209194

This certainly simplifies lots of stuff.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #12)
> > +  StatusMsg("$DMD = '%s'\n", e);
> 
> It would be nice to have a StatusMsg here, something like "$DMD is
> undefined; DMD will not run".

That would pollute the output of Firefox, since DMD is always there (in debug builds for now, but in opt builds too after bug  1409739).

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8932388 [details]
Bug 1420355 - Don't link logalloc against the static runtime on Windows.

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

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8932389 [details]
Bug 1420355 - Don't declare replace_* functions in replace_malloc.h.

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

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8932390 [details]
Bug 1420355 - Remove fallbacks when DMD is initializing.

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

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8932391 [details]
Bug 1420355 - Don't initialize logalloc if MALLOC_LOG is not given.

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

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 8932392 [details]
Bug 1420355 - Don't initialize DMD if the DMD environment variable is not given.

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

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8932393 [details]
Bug 1420355 - Allow to statically link replace-malloc libraries.

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

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8932394 [details]
Bug 1420355 - Statically link DMD.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d80b5c4e1dd0
Don't link logalloc against the static runtime on Windows. r=njn
https://hg.mozilla.org/integration/autoland/rev/451cd087922f
Don't declare replace_* functions in replace_malloc.h. r=njn
https://hg.mozilla.org/integration/autoland/rev/3742a4b69ba2
Remove fallbacks when DMD is initializing. r=njn
https://hg.mozilla.org/integration/autoland/rev/e69357ccca9e
Don't initialize logalloc if MALLOC_LOG is not given. r=njn
https://hg.mozilla.org/integration/autoland/rev/0479dda078a2
Don't initialize DMD if the DMD environment variable is not given. r=njn
https://hg.mozilla.org/integration/autoland/rev/fd6702e6e0a0
Allow to statically link replace-malloc libraries. r=njn
https://hg.mozilla.org/integration/autoland/rev/a7ed89e13a4c
Statically link DMD. r=njn

Comment 24

[Natalia Csoregi [:nataliaCs]]

Backed out for mass failures on OS X and Android

Backout : https://hg.mozilla.org/integration/autoland/rev/8cbc145b9f915eb5911141be9266c909f8ac26af

Push with failures : https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a7ed89e13a4cc4ce7b8ebe4dd99bce5160852003

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=148332263&repo=autoland&lineNumber=1340 
              https://treeherder.mozilla.org/logviewer.html#?job_id=148333454&repo=autoland&lineNumber=1649

Comment 25

[Mike Hommey [:glandium]]

Turns out the last commit was removing things that were used for other things than DMD.

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8932394 [details]
Bug 1420355 - Statically link DMD.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30920e0f5df4
Don't link logalloc against the static runtime on Windows. r=njn
https://hg.mozilla.org/integration/autoland/rev/921a1441fd22
Don't declare replace_* functions in replace_malloc.h. r=njn
https://hg.mozilla.org/integration/autoland/rev/a19bce0cd06c
Remove fallbacks when DMD is initializing. r=njn
https://hg.mozilla.org/integration/autoland/rev/b19b3307075a
Don't initialize logalloc if MALLOC_LOG is not given. r=njn
https://hg.mozilla.org/integration/autoland/rev/f1d96b6ee076
Don't initialize DMD if the DMD environment variable is not given. r=njn
https://hg.mozilla.org/integration/autoland/rev/076e21f3b25e
Allow to statically link replace-malloc libraries. r=njn
https://hg.mozilla.org/integration/autoland/rev/eff8b66ce5c7
Statically link DMD. r=njn

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/30920e0f5df4
https://hg.mozilla.org/mozilla-central/rev/921a1441fd22
https://hg.mozilla.org/mozilla-central/rev/a19bce0cd06c
https://hg.mozilla.org/mozilla-central/rev/b19b3307075a
https://hg.mozilla.org/mozilla-central/rev/f1d96b6ee076
https://hg.mozilla.org/mozilla-central/rev/076e21f3b25e
https://hg.mozilla.org/mozilla-central/rev/eff8b66ce5c7

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c387f1f2e93e
Port bug 1420355 to TB/IB/SM: Statically link DMD. rs=bustage-fix