wrap _malloc_message() on FreeBSD

RESOLVED FIXED in mozilla17

Status

()

Core
Memory Allocator
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jan Beich, Unassigned)

Tracking

Trunk
mozilla17
All
FreeBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.08 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
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");
                                                              ^~~~~~~~~
...
Attachment #646483 - Flags: review?(justin.lebar+bug)
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?
I'd say we can safely remove this #define.
Comment on attachment 646483 [details] [diff] [review]
wrap

Let's remove the define, then.  (Or at least try and see what breaks.  :)
Attachment #646483 - Flags: review?(justin.lebar+bug) → review-
(Reporter)

Comment 5

5 years ago
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 *);
Attachment #646483 - Attachment is obsolete: true
Attachment #646839 - Flags: review?(justin.lebar+bug)
> 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 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...
Attachment #646839 - Flags: review?(justin.lebar+bug) → review+
(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

5 years ago
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
(Reporter)

Comment 10

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
(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
(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.
(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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/a231af50b731
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/a231af50b731
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.