Add new define MEMORY_TOOL_REPLACES_ALLOCATOR for when building for ASAN, valgrind, etc.

Add new define MEMORY_TOOL_REPLACES_ALLOCATOR for when building for ASAN, valgrind, etc. This will be used to make simple decisions on when to disable custom allocators, so that the memory tools can do their job.

BUG=246860
R=thakis@chromium.rg

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206331

[Chris Evans, 2013-06-12 21:12:10]

Nico, may we do this? It will help simplify things -- both the new
PartitionAlloc and also existing things like RenderArena. (FWIW, I believe
RenderArena might be lacking in valgrind coverage due to the existing setup).

[Nico, 2013-06-12 21:15:01]

Do we really need this? We should strive to have fewer distinct build
configurations, not more.

[Chris Evans, 2013-06-12 21:20:15]

On 2013/06/12 21:15:01, Nico wrote:
> Do we really need this? We should strive to have fewer distinct build
> configurations, not more.

It's not a new distinct build configuration, but more of an umbrella flag for
any build which is related to a memory tool. It could be used to replace

#if defined(ADDRESS_SANITIZER) || defined(USE_HEAPCHECK) ||
defined(RUNNING_ON_VALGRIND)
with
#if defined(MEMORY_TOOL_BUILD)


Another motivation for this is that:
defined(RUNNING_ON_VALGRIND)
is not reliable; certainly doesn't work in Blink files.

We could use
#if !defined(NVALGRIND)
but checking for the absence of a negative seems fragile, and a
Blink-outside-of-Chromium build (if such a thing exists?) would get it wrong.


An alternate CL would be to define a reliable USE_VALGRIND or VALGRIND_BUILD
define in the valgrind case. Would that be preferable?

[Nico, 2013-06-13 00:05:41]

(sorry, accidentally hit <return>)

glider, what do you think about this?

[Alexander Potapenko, 2013-06-13 08:21:49]

+eugenis
Evgeniy suggests the define's name to reflect the fact the tool uses its own
malloc (or intercepts malloc).
We also need this define to be set for ThreadSanitizer and MemorySanitizer (the
tsan=1 and msan=1 GYP defines in the same file)

[Alexander Potapenko, 2013-06-13 08:23:19]

+earthdok
Sergey, please take this into account and update your LSan CL.

[Chris Evans, 2013-06-13 08:30:51]

On 2013/06/13 08:21:49, Alexander Potapenko wrote:
> +eugenis
> Evgeniy suggests the define's name to reflect the fact the tool uses its own
> malloc (or intercepts malloc).

Is there a specific suggestion on the table? Happy to defer to what you prefer.
More generally, are you ok with the umbrella define idea? I think that's Nico's
main question.

> We also need this define to be set for ThreadSanitizer and MemorySanitizer
(the
> tsan=1 and msan=1 GYP defines in the same file)

Can do.

[eugenis, 2013-06-13 10:09:33]

On 2013/06/13 08:30:51, Chris Evans wrote:
> On 2013/06/13 08:21:49, Alexander Potapenko wrote:
> > +eugenis
> > Evgeniy suggests the define's name to reflect the fact the tool uses its own
> > malloc (or intercepts malloc).
> 
> Is there a specific suggestion on the table? Happy to defer to what you
prefer.
> More generally, are you ok with the umbrella define idea? I think that's
Nico's
> main question.
> 
> > We also need this define to be set for ThreadSanitizer and MemorySanitizer
> (the
> > tsan=1 and msan=1 GYP defines in the same file)
> 
> Can do.

Yes, please.

I think such define is a good idea, and it could be used to replace long
#ifdef's that enumerate all known sanitizers.

I'd rather we had a set of defines for different parts of tool functionality,
like MEMORY_TOOL_REPLACES_ALLOCATOR, then something about addressability checks,
initializedness checks, leak detection, etc. Sorry, I'm not good at naming
things.

[Chris Evans, 2013-06-13 10:19:34]

On 2013/06/13 10:09:33, eugenis wrote:
> On 2013/06/13 08:30:51, Chris Evans wrote:
> > On 2013/06/13 08:21:49, Alexander Potapenko wrote:
> > > +eugenis
> > > Evgeniy suggests the define's name to reflect the fact the tool uses its
own
> > > malloc (or intercepts malloc).
> > 
> > Is there a specific suggestion on the table? Happy to defer to what you
> prefer.
> > More generally, are you ok with the umbrella define idea? I think that's
> Nico's
> > main question.
> > 
> > > We also need this define to be set for ThreadSanitizer and MemorySanitizer
> > (the
> > > tsan=1 and msan=1 GYP defines in the same file)
> > 
> > Can do.
> 
> Yes, please.
> 
> I think such define is a good idea, and it could be used to replace long
> #ifdef's that enumerate all known sanitizers.
> 
> I'd rather we had a set of defines for different parts of tool functionality,
> like MEMORY_TOOL_REPLACES_ALLOCATOR, then something about addressability
checks,
> initializedness checks, leak detection, etc. Sorry, I'm not good at naming
> things.

Ok. MEMORY_TOOL_REPLACES_ALLOCATOR. Which tools does this apply to? Everything
but tsan? Give me the list and I can get it done tomorrow :)

[Alexander Potapenko, 2013-06-13 11:08:50]

Um, this is about the tools that need to be aware of all memory allocations that
happen in the program. These include:
 - Valgrind Memcheck (replaces the allocator)
 - Valgrind-based TSan (wraps malloc())
 - PIN-based TSan (wraps? malloc())
 - compiler-based TSan, ASan and MSan (replace the allocator)
 - Heapchecker (is a part of tcmalloc)
 - LSan (upcoming, nothing to be changed in the existing code, will replace the
allocator)
 - Dr. Memory (don't know anything on this, CCing Derek)

Most of these replace the allocator (except for the old versions of TSan, but
let's not bother changing the wording just because of it,
MEMORY_TOOL_REPLACES_ALLOCATOR SGTM).

For the compiler-based tools there are "asan=1", "tsan=1", "msan=1" GYP
switches. For LSan there'll be "lsan=1". For Heapchecker that's
"linux_use_heapcheck=1", for Valgrind-based tools and Dr. Memory that's
"build_for_tool={memcheck,tsan,drmemory}".

[Chris Evans, 2013-06-13 20:24:50]

On 2013/06/13 11:08:50, Alexander Potapenko wrote:
> Um, this is about the tools that need to be aware of all memory allocations
that
> happen in the program. These include:
>  - Valgrind Memcheck (replaces the allocator)
>  - Valgrind-based TSan (wraps malloc())
>  - PIN-based TSan (wraps? malloc())
>  - compiler-based TSan, ASan and MSan (replace the allocator)
>  - Heapchecker (is a part of tcmalloc)
>  - LSan (upcoming, nothing to be changed in the existing code, will replace
the
> allocator)
>  - Dr. Memory (don't know anything on this, CCing Derek)
> 
> Most of these replace the allocator (except for the old versions of TSan, but
> let's not bother changing the wording just because of it,
> MEMORY_TOOL_REPLACES_ALLOCATOR SGTM).

Done.

> 
> For the compiler-based tools there are "asan=1", "tsan=1", "msan=1" GYP
> switches. For LSan there'll be "lsan=1". For Heapchecker that's
> "linux_use_heapcheck=1", for Valgrind-based tools and Dr. Memory that's
> "build_for_tool={memcheck,tsan,drmemory}".

Thanks for the list; it made it easy to get them all. I had missed msan (how
could I forget msan! :P )

@thakis: PTAL

[Nico, 2013-06-13 20:25:46]

Please update the CL description too. Also, CL descriptions should always answer
"why?"

[Derek Bruening, 2013-06-13 20:40:59]

On 2013/06/13 11:08:50, Alexander Potapenko wrote:
>  - Dr. Memory (don't know anything on this, CCing Derek)

Supports both wrapping or replacing all layers (operators, dbg crt, malloc,
Heap*, Rtl*Heap).  Current version in chromium wraps but that will soon swap to
replacing by default (as wrapping on win8 is even more painful than win7).

[Chris Evans, 2013-06-13 20:55:19]

On 2013/06/13 20:25:46, Nico wrote:
> Please update the CL description too. Also, CL descriptions should always
answer
> "why?"

Done, thanks.

[Nico, 2013-06-13 21:07:12]

lgtm

[commit-bot: I haz the power, 2013-06-13 21:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cevans@chromium.org/16844006/17001

[commit-bot: I haz the power, 2013-06-14 06:36:42]

Change committed as 206331