Compatibility with AddressSanitizer (fast memory error detector for C/C++)

Compatibility with AddressSanitizer (fast memory error detector for C/C++)

[ivanpe.google]

Very straightforward change.  Only affects a single unit test when compiled with
ADDRESS_SANITIZER=1.

Thanks,
-Ivan

[Ted Mielczarek]

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:542: #ifdef ADDRESS_SANITIZER
Seems like it'd be simpler to just wrap the block in #ifndef ADDRESS_SANITIZER.

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:546: // code in
AddressSanitizer may change in the future.
Comments should be in third-person, no "We".

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:590:
ASSERT_GT(memory_list->region_count(), 0u);
It sounds like perhaps this whole test may be invalid with ASAN enabled, since
the whole point is to test the behavior of some Breakpad code when the
instruction pointer is NULL, but ASAN rewrites that behavior. Maybe you should
just ifdef out the entire test, since it's not testing anything useful in this
configuration?

[Mark Mentovai]

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:588: // If AddressSanitizer
is used, we end up with more than one memory regions.
More than one region, singular. And, as Ted pointed out, “we” again.

[ivanpe.google]

I wrapped the whole test in #ifndef ADDRESS_SANITIZER.

Thanks,
-Ivan

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
File client/linux/handler/exception_handler_unittest.cc (right):

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:542: #ifdef ADDRESS_SANITIZER
On 2012/08/06 12:06:29, Ted Mielczarek wrote:
> Seems like it'd be simpler to just wrap the block in #ifndef
ADDRESS_SANITIZER.
I wrapped the whole function in #ifndef ADDRESS_SANITIZER

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:546: // code in
AddressSanitizer may change in the future.
On 2012/08/06 12:06:29, Ted Mielczarek wrote:
> Comments should be in third-person, no "We".

Done.

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:588: // If AddressSanitizer
is used, we end up with more than one memory regions.
On 2012/08/06 13:11:32, Mark Mentovai wrote:
> More than one region, singular. And, as Ted pointed out, “we” again.

Done.

http://breakpad.appspot.com/428002/diff/2001/client/linux/handler/exception_h...
client/linux/handler/exception_handler_unittest.cc:590:
ASSERT_GT(memory_list->region_count(), 0u);
On 2012/08/06 12:06:29, Ted Mielczarek wrote:
> It sounds like perhaps this whole test may be invalid with ASAN enabled, since
> the whole point is to test the behavior of some Breakpad code when the
> instruction pointer is NULL, but ASAN rewrites that behavior. Maybe you should
> just ifdef out the entire test, since it's not testing anything useful in this
> configuration?

Done.

[Mark Mentovai]

LGTM