1203272 - Fix build of nsMemoryReporterManager on linux with musl libc

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Felix Janda]

Attachment: Proposed patch

Bug 828844 has introduced a build failure of xpcom/base/nsMemoryReporterManager.cpp on linux systems using the musl libc library (http://musl-libc.org) because musl does not have mallinfo().

According to bug 828844 glibc and most android systems have mallinfo(). At least linux systems using the uclibc or musl c library and possibly some android systems do not have it. To detect its presence it seems to be simplest to use a configure test.

In the case of musl, no interface for querying heap usage is provided, and therefore firefox also cannot expose this information via about:memory.

Comment 1

[Felix Janda]

Attachment: Proposed patch

Added missing changes to moz.build.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8658887 [details] [diff] [review]
Proposed patch

Review of attachment 8658887 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the fix. It looks ok to me but I'd like a build system peer to check it as well.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "xpcom-private.h"

Nit: I would put this after "#include "mozilla/ipc/FileDescriptorUtils.h" to preserve alphabetical order.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8658887 [details] [diff] [review]
Proposed patch

Review of attachment 8658887 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "xpcom-private.h"

That shouldn't be needed. AC_CHECK_FUNCS results (HAVE_* defines) are defined in mozilla-config.h, which is included everywhere.

::: xpcom/xpcom-private.h.in
@@ +34,5 @@
>  /* Define if statfs() is available */
>  #undef HAVE_STATFS
>  
> +/* Define if mallinfo() is available */
> +#undef HAVE_MALLINFO

That shouldn't be needed either.

::: xpcom/base/moz.build
@@ +145,4 @@
>  
>  FINAL_LIBRARY = 'xul'
>  
> +GENERATED_INCLUDES += ['..']

Likewise.

Comment 4

[Felix Janda]

(In reply to Mike Hommey [:glandium] from comment #3)

Thanks for the review!

> Comment on attachment 8658887 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 8658887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +4,4 @@
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +#include "xpcom-private.h"
> 
> That shouldn't be needed. AC_CHECK_FUNCS results (HAVE_* defines) are
> defined in mozilla-config.h, which is included everywhere.

I've tested that the HAVE_* macros don't get exposed in this particular file. So mozilla-config.h does not seem to be included for some reason.

> ::: xpcom/xpcom-private.h.in
> @@ +34,5 @@
> >  /* Define if statfs() is available */
> >  #undef HAVE_STATFS
> >  
> > +/* Define if mallinfo() is available */
> > +#undef HAVE_MALLINFO
> 
> That shouldn't be needed either.

I did not test removing this, but am wondering about the other uses of this file.

> ::: xpcom/base/moz.build
> @@ +145,4 @@
> >  
> >  FINAL_LIBRARY = 'xul'
> >  
> > +GENERATED_INCLUDES += ['..']
> 
> Likewise.

This is needed to expose "xpcom-private.h" and is therefore unnecessary exactly when your first remark applies.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Felix Janda from comment #4)
> > That shouldn't be needed. AC_CHECK_FUNCS results (HAVE_* defines) are
> > defined in mozilla-config.h, which is included everywhere.
> 
> I've tested that the HAVE_* macros don't get exposed in this particular
> file. So mozilla-config.h does not seem to be included for some reason.

All files in the xpcom/base directory are built with -include ../../mozilla-config.h, that seems hard to believe that it wouldn't be included.

Comment 6

[Felix Janda]

(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Felix Janda from comment #4)
> > > That shouldn't be needed. AC_CHECK_FUNCS results (HAVE_* defines) are
> > > defined in mozilla-config.h, which is included everywhere.
> > 
> > I've tested that the HAVE_* macros don't get exposed in this particular
> > file. So mozilla-config.h does not seem to be included for some reason.
> 
> All files in the xpcom/base directory are built with -include
> ../../mozilla-config.h, that seems hard to believe that it wouldn't be
> included.

You're right, mozilla-config.h gets included everywhere. My tests did not succeed because I had used HAVE_STATFS to test, but that is part of the _NON_GLOBAL_ACDEFINES.

Comment 7

[Felix Janda]

Attachment: Patch v2

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8659989 [details] [diff] [review]
Patch v2

Review of attachment 8659989 [details] [diff] [review]:
-----------------------------------------------------------------

This is the same patch as before, as far as I can tell.

Comment 9

[Felix Janda]

Attachment: Patch v3

Sorry for the previous bad patch and this delay.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8703264 [details] [diff] [review]
Patch v3

Review of attachment 8703264 [details] [diff] [review]:
-----------------------------------------------------------------

This version removes all the parts that Mike said were unnecessary, so it looks ok to me. Thank you.

Comment 11

[Nicholas Nethercote [inactive]]

Felix, do you have commit access, or do you need someone to land it for you? I can do so, if necessary.

Comment 12

[Felix Janda]

I don't have commit access. Thanks for the review and offering to land the patch! I would appreciate it.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f927d0ccfbd5971edf0e34808e1c78af7f9721b
Bug 1203272 - Fix build of nsMemoryReporterManager on linux systems without mallinfo(). r=glandium,njn.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f927d0ccfbd