tcmalloc: move includes for mallinfo from tcmalloc.cc to tcmalloc.h

It is important that 'struct mallinfo' is defined in the header file.
Otherwise, Clang will warn about the declaration of 'tc_mallinfo' because it has C-linkage, but has an incomplete return type.

This was sent upstream in http://code.google.com/p/gperftools/issues/detail?id=450

BUG=138571
TEST=it builds without warning


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148386

[Nico, 2012-07-24 13:38:16]

https://chromiumcodereview.appspot.com/10818025/diff/1/base/allocator/allocat...
File base/allocator/allocator.gyp (right):

https://chromiumcodereview.appspot.com/10818025/diff/1/base/allocator/allocat...
base/allocator/allocator.gyp:400: # with c-style linkage.
Do you know why the warning fires on this?

[hans, 2012-07-24 13:49:13]

https://chromiumcodereview.appspot.com/10818025/diff/1/base/allocator/allocat...
File base/allocator/allocator.gyp (right):

https://chromiumcodereview.appspot.com/10818025/diff/1/base/allocator/allocat...
base/allocator/allocator.gyp:400: # with c-style linkage.
On 2012/07/24 13:38:16, Nico wrote:
> Do you know why the warning fires on this?

Yes, tc_mallinfo is declared inside an 'extern "C"' block, which means that the
function name will not be mangled. However, 'struct mallinfo' is a C++ type, so
calling it from C code wouldn't work. This is what the warning is about.

However, as long as the function is defined in C++, and only called from c++
code, everything is fine (see e.g.
http://msdn.microsoft.com/en-us/library/1e02627y.aspx).

The reason for not wanting to mangle the name could be that the library wants to
have a nice name for the function to pass to libtool, or something similar.

This was also discussed when the warning was added to Clang, see e.g.
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120206/052972.html

[hans, 2012-07-24 13:53:32]

On 2012/07/24 13:49:13, hans wrote:
> However, 'struct mallinfo' is a C++ type,

Wait a minute.. it's usually just a vanilla struct with some ints in it. Hmm.

[Nico, 2012-07-24 14:15:23]

(Fill-in from chat: hans discovered that the warning goes away if he adds
`#include <malloc.h>`, which is where struct mallinfo is defined. He'll ask on
the clang list if people feel clang should really emit this warning for unknown
types. If so, we might either disable the warning or add the include (and add a
"note:" to the warning to make it less mysterious). If not, the warning will go
away once upstream clang is changed and that change makes it into chrome.)

[hans, 2012-07-24 16:13:21]

On 2012/07/24 14:15:23, Nico wrote:
> (Fill-in from chat: hans discovered that the warning goes away if he adds
> `#include <malloc.h>`, which is where struct mallinfo is defined. He'll ask on
> the clang list if people feel clang should really emit this warning for
unknown
> types. If so, we might either disable the warning or add the include (and add
a
> "note:" to the warning to make it less mysterious). If not, the warning will
go
> away once upstream clang is changed and that change makes it into chrome.)

Right, I looked at the thread on cfe-commits, and this was already discussed
there; I got the impression the warning isn't going away for this case.

The fix is to make sure struct mallinfo is defined by the time that function is
declared. There is actually already a line in tcmalloc.h that tries to do this:
"#include <stdlib.h> // for struct mallinfo", except that doesn't work on
systems where it's not defined in stdlib.h. tcmalloc.cc gets the includes right
though, so let's move them to the .h file.

I'll also get a patch out clang to change the wording of the warning for this
case.

[hans, 2012-07-24 16:20:34]

New patch. This patch was also sent upstream:
http://code.google.com/p/gperftools/issues/detail?id=450

[Nico, 2012-07-24 16:37:10]

https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
File third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h (right):

https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h:57: #ifdef
HAVE_STRUCT_MALLINFO
HAVE_STRUCT_MALLINFO etc are defined in
third_party/tcmalloc/chromium/src/config.h , which isn't included yet at this
point as far as I can tell.

[hans, 2012-07-24 16:52:08]

On 2012/07/24 16:37:10, Nico wrote:
>
https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
> File third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h (right):
> 
>
https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
> third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h:57: #ifdef
> HAVE_STRUCT_MALLINFO
> HAVE_STRUCT_MALLINFO etc are defined in
> third_party/tcmalloc/chromium/src/config.h , which isn't included yet at this
> point as far as I can tell.

Hmm, weird.

HAVE_STRUCT_MALLINFO is used further down in the file to guard the declaration
of tc_mallinfo, which is what triggers the warning in the first place :/

I guess this is kind of broken in a sense, because it means that no-one can use
that tc_mallinfo function unless they're including tcmalloc's config.h (or
defining HAVE_STRUCT_MALLINFO themselves).

On the up-side, my patch doesn't make it more broken, and it does fix the
warning :)

[Nico, 2012-07-24 16:58:14]

On 2012/07/24 16:52:08, hans wrote:
> On 2012/07/24 16:37:10, Nico wrote:
> >
>
https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
> > File third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
> > third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h:57: #ifdef
> > HAVE_STRUCT_MALLINFO
> > HAVE_STRUCT_MALLINFO etc are defined in
> > third_party/tcmalloc/chromium/src/config.h , which isn't included yet at
this
> > point as far as I can tell.
> 
> Hmm, weird.
> 
> HAVE_STRUCT_MALLINFO is used further down in the file to guard the declaration
> of tc_mallinfo, which is what triggers the warning in the first place :/
> 
> I guess this is kind of broken in a sense, because it means that no-one can
use
> that tc_mallinfo function unless they're including tcmalloc's config.h (or
> defining HAVE_STRUCT_MALLINFO themselves).
> 
> On the up-side, my patch doesn't make it more broken, and it does fix the
> warning :)

Hm, ok :-P

So Tony said the right way to get changes into tcmalloc is by posting a bug on
the open bug tracker?

Please include the link to that patch in a) the CL description b) the
third_party/tcmalloc README.chromium.

[hans, 2012-07-24 17:03:12]

On 2012/07/24 16:58:14, Nico wrote:
>
https://chromiumcodereview.appspot.com/10818025/diff/9001/third_party/tcmallo...
> > > third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h:57: #ifdef
> > > HAVE_STRUCT_MALLINFO
> > > HAVE_STRUCT_MALLINFO etc are defined in
> > > third_party/tcmalloc/chromium/src/config.h , which isn't included yet at
> this
> > > point as far as I can tell.
> > 
> > Hmm, weird.
> > 
> > HAVE_STRUCT_MALLINFO is used further down in the file to guard the
declaration
> > of tc_mallinfo, which is what triggers the warning in the first place :/
> > 
> > I guess this is kind of broken in a sense, because it means that no-one can
> use
> > that tc_mallinfo function unless they're including tcmalloc's config.h (or
> > defining HAVE_STRUCT_MALLINFO themselves).
> > 
> > On the up-side, my patch doesn't make it more broken, and it does fix the
> > warning :)
> 
> Hm, ok :-P
> 
> So Tony said the right way to get changes into tcmalloc is by posting a bug on
> the open bug tracker?
> 
> Please include the link to that patch in a) the CL description b) the
> third_party/tcmalloc README.chromium.

+willchan, because Tony said you're the right person to look at this.

No, I sent an email with the patch to the upstream mailing list, and the
maintainer replied saying he wanted it on the bug tracker.

I've updated the CL description.

The README.chromium file doesn't contain any other info about local changes. It
just says that the vendor/ subdirectory contains the vanilla upstream source,
and chromium/ contains the source plus our modifications. I'm not sure I should
update it.

[Nico, 2012-07-24 17:08:24]

lgtm then

[hans, 2012-07-25 07:53:30]

willchan: ping?

[willchan no longer on Chromium, 2012-07-25 17:42:54]

My preferred method would be for you to make the change upstream and then merge
it back here. If this is blocking something you want to enable when building
Chromium in clang, then it's fine to commit directly to Chromium and merge the
upstream change later.

[hans, 2012-07-25 17:58:50]

On 2012/07/25 17:42:54, willchan wrote:
> My preferred method would be for you to make the change upstream and then
merge
> it back here. If this is blocking something you want to enable when building
> Chromium in clang, then it's fine to commit directly to Chromium and merge the
> upstream change later.

The change was sent upstream, but I don't know how long it will take before the
maintainer lands it.

It's blocking us building warning-free with Clang on Linux. Does that mean it's
ok to land?

[willchan no longer on Chromium, 2012-07-25 18:01:21]

Yes, LGTM then.

[commit-bot: I haz the power, 2012-07-25 18:02:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hans@chromium.org/10818025/9001

[commit-bot: I haz the power, 2012-07-25 19:58:17]

Change committed as 148386