778058 - wrap _malloc_message() on FreeBSD

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Comment 1

[Jan Beich]

Attachment: wrap

--enable-jemalloc build fails on FreeBSD 10.0-CURRENT which has jemalloc-3.0.0 in libc (since 2012-04-17)

/a/mozilla-central/memory/mozjemalloc/jemalloc.c:1538:8: error: redefinition of 'malloc_message' with a
      different type
void    (*_malloc_message)(const char *p1, const char *p2, const char *p3,
          ^
/a/mozilla-central/memory/mozjemalloc/jemalloc.c:1536:25: note: expanded from macro '_malloc_message'
#define _malloc_message malloc_message
                        ^
/usr/include/stdlib.h:232:15: note: previous definition is here
extern void (*malloc_message)(void *, const char *);
              ^
/a/mozilla-central/memory/mozjemalloc/jemalloc.c:1949:27: error: too many arguments to function call,
      expected 2, have 4
        _malloc_message(buf, "", "", "");
        ~~~~~~~~~~~~~~~          ^~~~~~
/a/mozilla-central/memory/mozjemalloc/jemalloc.c:2419:42: error: too many arguments to function call,
      expected 2, have 4
                            ": (malloc) Error in munmap(): ", buf, "\n");
                                                              ^~~~~~~~~
...

Comment 2

[Justin Lebar (not reading bugmail)]

Why is stdlib.h exporting malloc_message?  That seems like a bug to me.  (Not to suggest we shouldn't work around it.)

I'm not sure why we have this #define _malloc_message malloc_message to begin with; it doesn't seem to be doing anything.  What happens if we nix it altogether?

Comment 3

[Mike Hommey [:glandium]]

I'd say we can safely remove this #define.

Comment 4

[Justin Lebar (not reading bugmail)]

Comment on attachment 646483 [details] [diff] [review]
wrap

Let's remove the define, then.  (Or at least try and see what breaks.  :)

Comment 5

[Jan Beich]

Attachment: do not rename _malloc_message

> Why is stdlib.h exporting malloc_message? That seems like a bug to me.

It's a jemalloc(3) diagnostic feature, under __BSD_VISIBLE on FreeBSD.

$ gcc -include stdlib.h -E -</dev/null -D_POSIX_C_SOURCE=200809 | fgrep malloc_message
$ gcc -include stdlib.h -E -</dev/null | fgrep malloc_message                    extern void (*malloc_message)(void *, const char *);

Comment 6

[Justin Lebar (not reading bugmail)]

> It's a jemalloc(3) diagnostic feature, under __BSD_VISIBLE on FreeBSD.

But why does code external to jemalloc need to call malloc_message(str)?  It's only doing write(STDERR_FILENO, str, strlen(str)), at least in the vanilla jemalloc3 source.

Anyway, I pushed the patch to try just to be safe.  https://tbpl.mozilla.org/?tree=Try&rev=1c44261b8a85

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 646839 [details] [diff] [review]
do not rename _malloc_message

r=me, but I'll wait for the try results before pushing, because who knows...

Comment 8

[Mike Hommey [:glandium]]

(In reply to Justin Lebar [:jlebar] from comment #6)
> > It's a jemalloc(3) diagnostic feature, under __BSD_VISIBLE on FreeBSD.
> 
> But why does code external to jemalloc need to call malloc_message(str)? 
> It's only doing write(STDERR_FILENO, str, strlen(str)), at least in the
> vanilla jemalloc3 source.

FreeBSD's system allocator *is* jemalloc.

Which bears the question: why --enable-jemalloc at all?

Comment 9

[Mozilla RelEng Bot]

Try run for 1c44261b8a85 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c44261b8a85
Results (out of 217 total builds):
    exception: 2
    success: 166
    warnings: 45
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c44261b8a85

Comment 10

[Jan Beich]

(In reply to Mike Hommey [:glandium] from comment #8)
> Which bears the question: why --enable-jemalloc at all?

mozjemalloc is a fork of old jemalloc. The most obvious difference is jemalloc_stats() which is needed to properly support heap statistics in about:memory. mozjemalloc_compat.c isn't quite there yet, only heap-allocated works.

I have a local hack to skip compiling memory/jemalloc on 10.0-CURRENT, i.e. when using --enable-jemalloc + export MOZ_JEMALLOC. Here's the result for clean profile:

WARNING: the following values are negative or unreasonably large.
 heap-committed
 heap-committed-unused
 heap-dirty
This indicates a defect in one or more memory reporters.  The invalid values are highlighted.

Comment 11

[Jan Beich]

(In reply to Jan Beich from comment #10)
> I have a local hack to skip compiling memory/jemalloc on 10.0-CURRENT

Actually, it's not so local

http://trillian.chruetertee.ch/freebsd-gecko/changeset/781

Comment 12

[Mike Hommey [:glandium]]

(In reply to Jan Beich from comment #10)
> WARNING: the following values are negative or unreasonably large.
>  heap-committed
>  heap-committed-unused
>  heap-dirty
> This indicates a defect in one or more memory reporters.  The invalid values
> are highlighted.

These are not supported (yet). See bug 762445.

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Justin Lebar [:jlebar] from comment #6)
> > > It's a jemalloc(3) diagnostic feature, under __BSD_VISIBLE on FreeBSD.
> > 
> > But why does code external to jemalloc need to call malloc_message(str)? 
> > It's only doing write(STDERR_FILENO, str, strlen(str)), at least in the
> > vanilla jemalloc3 source.
> 
> FreeBSD's system allocator *is* jemalloc.

Understood.  But why does a function entirely internal to the system allocator need to be exposed in stdlib.h?

Comment 14

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a231af50b731

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a231af50b731