Last Comment Bug 778058 - wrap _malloc_message() on FreeBSD
: wrap _malloc_message() on FreeBSD
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Memory Allocator (show other bugs)
: Trunk
: All FreeBSD
: -- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-27 00:06 PDT by Jan Beich
Modified: 2012-07-29 15:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wrap (874 bytes, patch)
2012-07-27 00:48 PDT, Jan Beich
justin.lebar+bug: review-
Details | Diff | Splinter Review
do not rename _malloc_message (1.08 KB, patch)
2012-07-28 05:33 PDT, Jan Beich
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Jan Beich 2012-07-27 00:06:12 PDT

    
Comment 1 Jan Beich 2012-07-27 00:48:07 PDT
Created attachment 646483 [details] [diff] [review]
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) 2012-07-27 11:03:02 PDT
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] 2012-07-27 11:17:20 PDT
I'd say we can safely remove this #define.
Comment 4 Justin Lebar (not reading bugmail) 2012-07-27 11:19:57 PDT
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 2012-07-28 05:33:41 PDT
Created attachment 646839 [details] [diff] [review]
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) 2012-07-28 21:10:39 PDT
> 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) 2012-07-28 21:11:12 PDT
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] 2012-07-29 01:00:29 PDT
(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 2012-07-29 01:00:30 PDT
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/jlebar@mozilla.com-1c44261b8a85
Comment 10 Jan Beich 2012-07-29 04:12:07 PDT
(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 2012-07-29 04:39:11 PDT
(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] 2012-07-29 06:11:08 PDT
(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) 2012-07-29 07:49:37 PDT
(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) 2012-07-29 07:56:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a231af50b731
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-07-29 15:27:39 PDT
https://hg.mozilla.org/mozilla-central/rev/a231af50b731

Note You need to log in before you can comment on or make changes to this bug.