Added an utility function to ensure that we don't "asanize" the same image twice.

Added an utility function to ensure that we don't "asanize" the same image twice.


BUG=

Committed: https://code.google.com/p/sawbuck/source/detail?r=1220

[Sébastien Marchand, 2012-11-02 19:52:05]

PTAL.

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
File syzygy/instrument/transforms/asan_transform.cc (right):

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:249: 
Most of this code come from
pe\transforms\add_imports_transform.cc::FindOrAddImageImportDescriptor , maybe
we should move this code somewhere else to avoid duplicated code ?

[Roger McFarlane, 2012-11-02 20:01:01]

Nice.

This can be a generalized PE utility function to check if the header block
indicates a dependence on a given dll.

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
File syzygy/instrument/transforms/asan_transform.cc (right):

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:254: // @returns true if the
inage has already been instrumented, false otherwise.
s/inage/image/

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:256: BlockGraph::Block*
header_block,
block_graph isn't used (except to DCHECK)

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:257: const char* dll_name) {
return true/false to indicate success of the operation and take an extra boolean
param to indicate whetehr or not the image was already instrumented.

[Sébastien Marchand, 2012-11-02 21:09:55]

Thanks, PTAnL.

I've moved the function to pe_utils. However, I can't simply call it in
add_imports_transform because it'll require more arguments specific to this case
and is not used in the same context.

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
File syzygy/instrument/transforms/asan_transform.cc (right):

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:254: // @returns true if the
inage has already been instrumented, false otherwise.
On 2012/11/02 20:01:01, Roger McFarlane wrote:
> s/inage/image/

Done.

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:256: BlockGraph::Block*
header_block,
On 2012/11/02 20:01:01, Roger McFarlane wrote:
> block_graph isn't used (except to DCHECK)

Done.

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:257: const char* dll_name) {
On 2012/11/02 20:01:01, Roger McFarlane wrote:
> return true/false to indicate success of the operation and take an extra
boolean
> param to indicate whetehr or not the image was already instrumented.

Done.

https://codereview.appspot.com/6817076/diff/1/syzygy/instrument/transforms/as...
syzygy/instrument/transforms/asan_transform.cc:257: const char* dll_name) {
On 2012/11/02 20:01:01, Roger McFarlane wrote:
> return true/false to indicate success of the operation and take an extra
boolean
> param to indicate whetehr or not the image was already instrumented.

Done.

[Siggi, 2012-11-05 13:58:30]

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.cc
File syzygy/pe/pe_utils.cc (right):

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.cc#newcode405
syzygy/pe/pe_utils.cc:405: size_t max_len = ref_dll_name.block()->size() -
ref_dll_name.offset();
this looks like ref_dll_name.ElementCount()?

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.h
File syzygy/pe/pe_utils.h (right):

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.h#newcode123
syzygy/pe/pe_utils.h:123: bool
ContainsDependenceToDLL(block_graph::BlockGraph::Block* header_block,
HasImportEntry, perhaps - better to be consistent with the underlying
terminology.
If you haven't already read this
[http://msdn.microsoft.com/en-us/magazine/cc301805.aspx] and the part II, maybe
you should do so.

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.h#newcode124
syzygy/pe/pe_utils.h:124: const char* dll_name,
does it make sense to use StringPiece here?

[Sébastien Marchand, 2012-11-05 14:51:23]

Thanks, PTAnL.

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.cc
File syzygy/pe/pe_utils.cc (right):

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.cc#newcode405
syzygy/pe/pe_utils.cc:405: size_t max_len = ref_dll_name.block()->size() -
ref_dll_name.offset();
On 2012/11/05 13:58:30, Siggi wrote:
> this looks like ref_dll_name.ElementCount()?

Done.

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.h
File syzygy/pe/pe_utils.h (right):

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.h#newcode123
syzygy/pe/pe_utils.h:123: bool
ContainsDependenceToDLL(block_graph::BlockGraph::Block* header_block,
On 2012/11/05 13:58:30, Siggi wrote:
> HasImportEntry, perhaps - better to be consistent with the underlying
> terminology.
> If you haven't already read this
> [http://msdn.microsoft.com/en-us/magazine/cc301805.aspx] and the part II,
maybe
> you should do so.

Done.

https://codereview.appspot.com/6817076/diff/3/syzygy/pe/pe_utils.h#newcode124
syzygy/pe/pe_utils.h:124: const char* dll_name,
On 2012/11/05 13:58:30, Siggi wrote:
> does it make sense to use StringPiece here?

Yep. Done. (I've to get used to what's in base !)

[Siggi, 2012-11-05 14:55:21]

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc
File syzygy/pe/pe_utils.cc (right):

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:353: base::StringPiece dll_name,
we use const base::StringPiece&

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.h
File syzygy/pe/pe_utils.h (right):

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.h#newcode117
syzygy/pe/pe_utils.h:117: // Check if an image contains a dependence to a given
DLL.
update comment?

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.h#newcode123
syzygy/pe/pe_utils.h:123: bool HasImportEntry(block_graph::BlockGraph::Block*
header_block,
this function deserves testing.

[Roger McFarlane, 2012-11-05 15:25:38]

A few more nits.

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc
File syzygy/pe/pe_utils.cc (right):

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:354: bool* contains_dependence) {
s/contains_dependence/has_import_entry/??
-- or --
s/contains_dependence/found/??

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:356: DCHECK(dll_name != NULL);
DCHECK(!dll_name.empty());

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:396: sizeof(IMAGE_IMPORT_DESCRIPTOR)) - 1;
descriptor_count is only valid if image_import_descriptor_block->size() > 0.
Validate it?

[Sébastien Marchand, 2012-11-05 15:58:00]

PTAnL.

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc
File syzygy/pe/pe_utils.cc (right):

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:353: base::StringPiece dll_name,
On 2012/11/05 14:55:21, Siggi wrote:
> we use const base::StringPiece&

Done. I've read in the StringPiece documentation that "you should pass
StringPiece by value just like you would an int or a double because  StringPiece
is a small value". Is it an exception for Chrome ?

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:354: bool* contains_dependence) {
On 2012/11/05 15:25:38, Roger McFarlane wrote:
> s/contains_dependence/has_import_entry/??
> -- or --
> s/contains_dependence/found/??

Done.

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:356: DCHECK(dll_name != NULL);
On 2012/11/05 15:25:38, Roger McFarlane wrote:
> DCHECK(!dll_name.empty());

Done.

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.cc#newcod...
syzygy/pe/pe_utils.cc:396: sizeof(IMAGE_IMPORT_DESCRIPTOR)) - 1;
On 2012/11/05 15:25:38, Roger McFarlane wrote:
> descriptor_count is only valid if image_import_descriptor_block->size() > 0.
> Validate it?

Done.

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.h
File syzygy/pe/pe_utils.h (right):

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.h#newcode117
syzygy/pe/pe_utils.h:117: // Check if an image contains a dependence to a given
DLL.
On 2012/11/05 14:55:21, Siggi wrote:
> update comment?

Done.

https://codereview.appspot.com/6817076/diff/5005/syzygy/pe/pe_utils.h#newcode123
syzygy/pe/pe_utils.h:123: bool HasImportEntry(block_graph::BlockGraph::Block*
header_block,
On 2012/11/05 14:55:21, Siggi wrote:
> this function deserves testing.

Done.

[Roger McFarlane, 2012-11-05 16:13:23]

lgtm

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest.cc
File syzygy/pe/pe_utils_unittest.cc (right):

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest...
syzygy/pe/pe_utils_unittest.cc:327: ASSERT_FALSE(has_import);
you can EXPECT_* on has_import as well.

[Roger McFarlane, 2012-11-05 16:14:44]

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest.cc
File syzygy/pe/pe_utils_unittest.cc (right):

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest...
syzygy/pe/pe_utils_unittest.cc:313: EXPECT_EQ("foo_func",
module.GetSymbolName(function_foo));
put foo_func into a string kFooFunc and ASSERT_EQ

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest...
syzygy/pe/pe_utils_unittest.cc:318:
EXPECT_TRUE(block_graph::ApplyBlockGraphTransform(
ASSERT_TRUE

[Siggi, 2012-11-05 16:17:27]

lgtm modulo Roger's remaining nits.

[Sébastien Marchand, 2012-11-05 16:27:46]

Thanks, committing.

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest.cc
File syzygy/pe/pe_utils_unittest.cc (right):

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest...
syzygy/pe/pe_utils_unittest.cc:313: EXPECT_EQ("foo_func",
module.GetSymbolName(function_foo));
On 2012/11/05 16:14:45, Roger McFarlane wrote:
> put foo_func into a string kFooFunc and ASSERT_EQ

Done.

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest...
syzygy/pe/pe_utils_unittest.cc:318:
EXPECT_TRUE(block_graph::ApplyBlockGraphTransform(
On 2012/11/05 16:14:45, Roger McFarlane wrote:
> ASSERT_TRUE

Done.

https://codereview.appspot.com/6817076/diff/12001/syzygy/pe/pe_utils_unittest...
syzygy/pe/pe_utils_unittest.cc:327: ASSERT_FALSE(has_import);
On 2012/11/05 16:13:24, Roger McFarlane wrote:
> you can EXPECT_* on has_import as well.

Done.