Get rid of LNK4217 warnings.

Make content_renderer.lib depend directly on allocator so that PERFTOOLS_DLL_DECL is defined when building this lib.  This gets rid of a number of LNK4217 warnings about locally defined symbols.

BUG=none
TEST=look for LNK4217 in build logs.


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

[grt (UTC plus 2), 2011-08-15 13:54:50]

Hi guys,

I've picked you two to review this change since you've been involved in content
and allocator, respectively.  The builders show a number of warnings like this:

content_renderer.lib(render_thread.obj) : warning LNK4217: locally defined
symbol ?instance@MallocExtension@@SAPAV1@XZ (public: static class
MallocExtension * __cdecl MallocExtension::instance(void)) imported in function
"public: void __thiscall RenderThread::IdleHandler(void)"
(?IdleHandler@RenderThread@@QAEXXZ)

These are caused by PERFTOOLS_DLL_DECL not being defined. 
allocator/allocator.gyp defines this for its direct dependents, so I've made
content_renderer depend on allocator.

Cheers,

Greg

[jam, 2011-08-15 16:12:45]

I'm curious, shouldn't base be the one exporting this dependency to targets that
depend on it, through export_dependent_settings?

[grt (UTC plus 2), 2011-08-15 20:36:09]

On 2011/08/15 16:12:45, John Abd-El-Malek wrote:
> I'm curious, shouldn't base be the one exporting this dependency to targets
that
> depend on it, through export_dependent_settings?

I see many .gyp{,i} files with similar logic.  Any component that statically
links the allocator lib should have the PERFTOOLS_DLL_DECL definition.  I'm not
sure stylistically what the best way to achieve this is.  allocator.gyp
explicitly puts this in a direct_dependent_settings block.  Perhaps that should
be relaxed so that it's defined for indirect dependents, too?

[jam, 2011-08-16 00:47:31]

Mark: wdyt?

[Mark Mentovai, 2011-08-16 02:56:16]

Are these warnings you speak of something that only shows up in the components
build, or something like that?

Can you share what the warnings are? Can you share what the headers we’re
currently picking up are that don‘t have PERFTOOLS_DLL_DECL defined but should?
That kind of thing?

[grt (UTC plus 2), 2011-08-16 09:20:13]

On 2011/08/16 02:56:16, Mark Mentovai wrote:
> Are these warnings you speak of something that only shows up in the components
> build, or something like that?

No.  They appear in all Chromium and Google Chrome Continuous win builds. 
Search for LNK4217 in any recent build and you'll see 22ish occurrences of:

content_renderer.lib(render_thread.obj) : warning LNK4217: locally defined
symbol ?instance@MallocExtension@@SAPAV1@XZ (public: static class
MallocExtension * __cdecl MallocExtension::instance(void)) imported in function
"public: void __thiscall RenderThread::IdleHandler(void)"
(?IdleHandler@RenderThread@@QAEXXZ)

(http://build.chromium.org/p/chromium/builders/Win/builds/6245/steps/compile/l...
is one recent example.)

It does not seem to appear in the win shared build logs.

> Can you share what the headers we’re
> currently picking up are that don‘t have PERFTOOLS_DLL_DECL defined but
should?

It appears that we're linking in tcmalloc statically (allocator is a
static_library target in
http://src.chromium.org/viewvc/chrome/trunk/src/base/allocator/allocator.gyp?...).
 As per the tcmalloc README
(http://code.google.com/p/google-perftools/source/browse/trunk/README_windows.txt),
allocator.gyp defines PERFTOOLS_DLL_DECL to the empty string when building the
tcmalloc sources so that public functions and types are not annotated with
__declspec(dllexport) (see
http://src.chromium.org/viewvc/chrome/trunk/src/third_party/tcmalloc/chromium...
for the preprocessor logic).  If PERFTOOLS_DLL_DECL is not also set to the empty
string when building all .cc files that include and use tcmalloc headers
directly, then we'll see these warnings.

I see now that
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/chrome_renderer.gypi?r...
both adds allocator as a direct requirement and has an export_dependent_settings
block.  Exporting the settings seems like dependency leakage; there's no need
for consumers of the renderer to have tcmallo-specific settings unless they're
reaching into tcmalloc.

All of this makes me wonder why bits of code reach directly into tcmalloc for
ReleaseFreeMemory.  If this is something that needs to be done when tcmalloc is
in use, shouldn't base/allocator expose something general that delegates to
tcmalloc when that's the allocator in use?  In other words, why should
render_thread.cc reach into tcmalloc when CHROME_ALLOCATOR is set to anything
other than "tcmalloc"?  Isn't this a layering violation?  Why not expose
base::allocator::ReleaseFreeMemory that delegates to tcmalloc iff tcmalloc is in
use?

Anyway, the simple thing to do to get rid of this warning right now is to make
sure that all targets with .cc files that include headers from
third_party/tcmalloc depend on allocator directly.

[Mark Mentovai, 2011-08-16 13:41:18]

Thanks for the explanation. LGTM. I agree with what you wrote with respect to
export_dependent_settings.

[commit-bot: I haz the power, 2011-08-16 15:48:35]

Change committed as 96949

[jam, 2011-08-16 18:34:53]

On 2011/08/16 13:41:18, Mark Mentovai wrote:
> Thanks for the explanation. LGTM. I agree with what you wrote with respect to
> export_dependent_settings.

+1. i wasn't aware that code in content_renderer reaches to tcmalloc directly
when I first saw this patch