Fix IsAllocatorInitialized() for memory tools on Linux.

Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.

BUG=481611
Committed: https://crrev.com/82c77818ce6beb4054c6cda916a4be7ffe8c279e
Cr-Commit-Position: refs/heads/master@{#384084}

[Lei Zhang, 2016-03-30 00:58:16]

thestig@chromium.org changed reviewers:
+ primiano@chromium.org

[Will Harris, 2016-03-30 01:19:19]

ah I was also midway through looking at that... I hadn't even got as far as
reproducing the error yet.

thanks. lgtm

[Will Harris, 2016-03-30 01:34:09]

Description was changed from

==========
Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.
==========

to

==========
Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.

BUG=481611
==========

[Will Harris, 2016-03-30 01:40:24]

well I was finally able to repro, and patching your CL fixes it, so I'm going to
CQ.

[Will Harris, 2016-03-30 01:40:27]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2016-03-30 01:40:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846483002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846483002/1

[commit-bot: I haz the power, 2016-03-30 07:48:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-30 07:48:30]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Lei Zhang, 2016-03-30 19:11:46]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-03-30 19:12:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1846483002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1846483002/1

[commit-bot: I haz the power, 2016-03-30 20:45:19]

Description was changed from

==========
Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.

BUG=481611
==========

to

==========
Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.

BUG=481611
==========

[commit-bot: I haz the power, 2016-03-30 20:45:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-30 20:46:23]

Description was changed from

==========
Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.

BUG=481611
==========

to

==========
Fix IsAllocatorInitialized() for memory tools on Linux.

When MEMORY_TOOL_REPLACES_ALLOCATOR is defined, tcmalloc is not in use.

BUG=481611

Committed: https://crrev.com/82c77818ce6beb4054c6cda916a4be7ffe8c279e
Cr-Commit-Position: refs/heads/master@{#384084}
==========

[commit-bot: I haz the power, 2016-03-30 20:46:25]

Patchset 1 (id:??) landed as
https://crrev.com/82c77818ce6beb4054c6cda916a4be7ffe8c279e
Cr-Commit-Position: refs/heads/master@{#384084}

[Primiano Tucci (use gerrit), 2016-03-31 11:32:47]

I AM BACK!

How can I reproduce this? GYP or GN? Which OS? Which flags?

We shouldn't be falsely setting USE_TCMALLOC when using memory tools.
To me this smells like there is a problem in the build files, and we are just
covering it up here.
use_allocator should be == 'none' on asan/tsan/*san & co, and that should be
enough.

IIRC the intended semantic of MEMORY_TOOL_REPLACES_ALLOCATOR is to prevent
symbol interposition (re-defining malloc & friends) + falling back on PartAlloc
to system alloc.
The choice of the allocator should come from the build files instead.

[Lei Zhang, 2016-03-31 17:45:13]

On 2016/03/31 11:32:47, Primiano Tucci wrote:
> I AM BACK!

Me too. Otherwise I wouldn't be here causing trouble.

> How can I reproduce this? GYP or GN? Which OS? Which flags?

Linux, GYP, GYP_DEFINES='build_for_tool=memcheck' -> content_shell won't launch.

> We shouldn't be falsely setting USE_TCMALLOC when using memory tools.
> To me this smells like there is a problem in the build files, and we are just
> covering it up here.
> use_allocator should be == 'none' on asan/tsan/*san & co, and that should be
> enough.

Maybe. Care to take a look? FWIW, this mirrors the ifdefs in UncheckedMalloc()
in base/process/memory_linux.cc.

[Will Harris, 2016-04-03 01:30:12]

On 2016/03/31 17:45:13, Lei Zhang wrote:
> On 2016/03/31 11:32:47, Primiano Tucci wrote:
> > I AM BACK!
> 
> Me too. Otherwise I wouldn't be here causing trouble.
> 
> > How can I reproduce this? GYP or GN? Which OS? Which flags?
> 
> Linux, GYP, GYP_DEFINES='build_for_tool=memcheck' -> content_shell won't
launch.
> 
> > We shouldn't be falsely setting USE_TCMALLOC when using memory tools.
> > To me this smells like there is a problem in the build files, and we are
just
> > covering it up here.
> > use_allocator should be == 'none' on asan/tsan/*san & co, and that should be
> > enough.
> 
> Maybe. Care to take a look? FWIW, this mirrors the ifdefs in UncheckedMalloc()
> in base/process/memory_linux.cc.

I think this CL is correct, as lei says the defines now equal each other between
base/process/memory_linux.cc and allocator_check.cc

[Primiano Tucci (use gerrit), 2016-04-04 13:06:57]

On 2016/04/03 01:30:12, Will Harris wrote:
> On 2016/03/31 17:45:13, Lei Zhang wrote:
> > On 2016/03/31 11:32:47, Primiano Tucci wrote:
> > > I AM BACK!
> > 
> > Me too. Otherwise I wouldn't be here causing trouble.
> > 
> > > How can I reproduce this? GYP or GN? Which OS? Which flags?
> > 
> > Linux, GYP, GYP_DEFINES='build_for_tool=memcheck' -> content_shell won't
> launch.
> > 
> > > We shouldn't be falsely setting USE_TCMALLOC when using memory tools.
> > > To me this smells like there is a problem in the build files, and we are
> just
> > > covering it up here.
> > > use_allocator should be == 'none' on asan/tsan/*san & co, and that should
be
> > > enough.
> > 
> > Maybe. Care to take a look? FWIW, this mirrors the ifdefs in
UncheckedMalloc()
> > in base/process/memory_linux.cc.
> 
> I think this CL is correct, as lei says the defines now equal each other
between
> base/process/memory_linux.cc and allocator_check.cc

I took a look to this, build_for_tool=memcheck does not reproduce this issue
(yes I reverted this cl locally).
I this this is expected, in fact from common.gypi build_for_tool==memcheck does
NOT set MEMORY_TOOL_REPLACES_ALLOCATOR.

The only conditions that set MEMORY_TOOL_REPLACES_ALLOCATOR in common-gypi I see
are:
asan=1
syzyasan=1
release_valgrind_build=1 or tsan=1
asan==1 or lsan==1 or tsan==1 or msan==1

But, furthermore, I see that use_allocator==None (which does NOT set
USE_TCMALLOC) if
asan==1 or msan==1 or lsan==1 or tsan==1


Given the current state of common.gypi, I can't understand which build
configuration is this CL fixing. Very likely I am missing something, the
question is: what?



>I think this CL is correct, as lei says the defines now equal each other
between
>base/process/memory_linux.cc and allocator_check.cc
Hmm not entirely sure I follow.

This is the way I read  UncheckedMalloc in memory_linux.cc:
If we are using a sanitizer (MEMORY_TOOL_REPLACES_ALLOCATOR) or don't have
neither glibc nor tcmalloc, let's just call malloc to implement UncheckedMalloc.

And this is the way I read allocator_check.cc after this CL:
Do the tcmalloc check if we are using tcmalloc ***but skip it if we are using a
sanitizer***

Here's my objection (but this might be a big conceptual misunderstanding from my
side): we should never end up in a case where, on Linux, we both want tcmalloc
AND a sanitizer.

In other words, I'd be tempted to land a CL that adds a compile time check to
allocator_check.cc as follows:
#if defined(USE_TCMALLOC) && defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
#error USE_TCMALLOC and MEMORY_TOOL_REPLACES_ALLOCATOR are mutually exclusive,
your current build config is inconsistent. Please file a crbug.com
#endif


Summarizing my questions are:
1) What is the build config fixed by this CL?
2) Is my latter understanding correct? (If so I'll land such #ifdef check)

[Lei Zhang, 2016-04-04 22:47:44]

On 2016/04/04 13:06:57, Primiano Tucci wrote:
> I took a look to this, build_for_tool=memcheck does not reproduce this issue
> (yes I reverted this cl locally).
> I this this is expected, in fact from common.gypi build_for_tool==memcheck
does
> NOT set MEMORY_TOOL_REPLACES_ALLOCATOR.
> 
> The only conditions that set MEMORY_TOOL_REPLACES_ALLOCATOR in common-gypi I
see
> are:
> asan=1
> syzyasan=1
> release_valgrind_build=1 or tsan=1
> asan==1 or lsan==1 or tsan==1 or msan==1
> 
> But, furthermore, I see that use_allocator==None (which does NOT set
> USE_TCMALLOC) if
> asan==1 or msan==1 or lsan==1 or tsan==1

build_for_tool==memcheck sets release_valgrind_build=1 -> sets
MEMORY_TOOL_REPLACES_ALLOCATOR.

> Given the current state of common.gypi, I can't understand which build
> configuration is this CL fixing. Very likely I am missing something, the
> question is: what?

Oh, the missing bit was to run: ./tools/valgrind/valgrind.sh
out/Release/content_shell. Running content_shell by itself w/o Valgrind does not
trigger the CHECK().

> Here's my objection (but this might be a big conceptual misunderstanding from
my
> side): we should never end up in a case where, on Linux, we both want tcmalloc
> AND a sanitizer.
> 
> In other words, I'd be tempted to land a CL that adds a compile time check to
> allocator_check.cc as follows:
> #if defined(USE_TCMALLOC) && defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
> #error USE_TCMALLOC and MEMORY_TOOL_REPLACES_ALLOCATOR are mutually exclusive,
> your current build config is inconsistent. Please file a http://crbug.com
> #endif

You better file a bug then. :) As is, we have this build config with
build_for_tool=memcheck.

[Primiano Tucci (use gerrit), 2016-04-13 17:31:53]

Closing the loop on this.

> You better file a bug then. :) As is, we have this build config with
> build_for_tool=memcheck.

Ok, as I was suspecting the problem here is that my speculation on how we use
valgrind was wrong and there is indeed one case in which we actually want to
have both tcmalloc and MEMORY_TOOL_REPLACES_ALLOCATOR at the same time. (which
is what you mentioned above)

I am still not convinced this is reasonable (the variable is saying clearly "the
memory tool replaces the allocator"), but this is undeniably the way valgrind
worked for years.
As a side note, we did shutdown the valgrind bot in chromium in
https://codereview.chromium.org/1727013002 explicitly declaring that it's too
painful to maintain, too slow. The new *san bot should give us that coverage in
a much faster way.

I think we should consistently shut down all the shadow instances, as this is
less maintained today. Also there is going to be no support for build_for_tool
in gn.
But, this is a separate discussion, beyond the scope of this CL.
At this point this CL LGTM, I was clearly just wrong. Sorry for the back and
forth.