Refactor code in preparation of merging with the fork in Chromium OS.

Refactor code in preparation of merging with the fork in Chromium OS.

This patch is part of a bigger patch that helps merging the breakpad code
with the modified version in Chromium OS.

Specifically, this patch makes the following changes:
1. Add a MemoryRange class for encapsulating and checking read access
   to a contiguous range of memory.
2. Add a MemoryMappedFile class for mapping a file into memory for
   read-only access.
3. Refactor other source code to use MemoryMappedFile.

BUG=455
TEST=Tested the following:
1. Build on 32-bit and 64-bit Linux with gcc 4.4.3 and gcc 4.6.
2. Build on Mac OS X 10.6.8 with gcc 4.2 and clang 3.0 (with latest gmock).
3. All unit tests pass.
4. Run minidump-2-core to covnert a minidump file to a core file.

[Ben Chan]

Hi Mark and Ted,

FYI, we're trying to upstream the Chromium OS version of breakpad. As part of
the merging, I'm refactoring the code and adding unit tests. To make the code
review easier, I try to break up the changes into smaller patches and this one
is part of them.

Thanks,
Ben

[kmixter]

http://breakpad.appspot.com/332001/diff/3001/4007
File src/common/linux/memory_mapped_file.cc (right):

http://breakpad.appspot.com/332001/diff/3001/4007#newcode62
Line 62: #if defined(__x86_64__)
Is there a way to cover other 64b targets?

http://breakpad.appspot.com/332001/diff/3001/4011
File src/common/memory_range_unittest.cc (right):

http://breakpad.appspot.com/332001/diff/3001/4011#newcode77
Line 77: { offset: 0, element_size: 1, index: 0,
will this compile on non-gcc ompilers?

[Ben Chan]

http://breakpad.appspot.com/332001/diff/3001/4007
File src/common/linux/memory_mapped_file.cc (right):

http://breakpad.appspot.com/332001/diff/3001/4007#newcode62
Line 62: #if defined(__x86_64__)
On 2011/12/15 02:39:23, kmixter wrote:
> Is there a way to cover other 64b targets?

I follow the same architecture macros (__i386__, __x86_64__) used by
third_party/lss/linux_syscall_support.h

The code should work for __ia64__, but no breakpad code (except gtest) currently
uses __ia64__.

http://breakpad.appspot.com/332001/diff/3001/4011
File src/common/memory_range_unittest.cc (right):

http://breakpad.appspot.com/332001/diff/3001/4011#newcode77
Line 77: { offset: 0, element_size: 1, index: 0,
On 2011/12/15 02:39:23, kmixter wrote:
> will this compile on non-gcc ompilers?

Done.

Thanks for pointing it out.  Should have used the C99 syntax instead of field
label syntax.  Anyway, I've changed the code to use the safest syntax.

[Ted Mielczarek]

http://breakpad.appspot.com/332001/diff/9001/10009
File src/common/linux/memory_mapped_file_unittest.cc (right):

http://breakpad.appspot.com/332001/diff/9001/10009#newcode144
Line 144: }
The only case that I don't see but I'd expect to see tested here is calling Map
on an already-mapped MemoryMappedFile. Otherwise looks pretty comprehensive.

http://breakpad.appspot.com/332001/diff/9001/10010
File src/common/memory_range.h (right):

http://breakpad.appspot.com/332001/diff/9001/10010#newcode45
Line 45: // access of a subrange of the memory.Its implemementation does not
nit: space after the period.

http://breakpad.appspot.com/332001/diff/9001/10010#newcode83
Line 83: bool Covers(size_t offset, size_t length) const {
This method reads a little confusingly with offset vs. offset_ and length vs.
length_. Maybe there are less-confusing alternate names you could use for the
parameters?

http://breakpad.appspot.com/332001/diff/9001/10010#newcode94
Line 94: const void* GetObject(size_t offset, size_t length) const {
GetObject is kind of a weird name here when you're just returning a void*.

http://breakpad.appspot.com/332001/diff/9001/10010#newcode103
Line 103: unsigned index) const {
I wonder if this method wouldn't make more sense as a templated member? Then you
could just pass offset+index, use sizeof(T), and return const T*.

http://breakpad.appspot.com/332001/diff/9001/10011
File src/common/memory_range_unittest.cc (right):

http://breakpad.appspot.com/332001/diff/9001/10011#newcode43
Line 43: const u_int8_t* const kNullPointer = NULL;
It seems a little unnecessary to define a constant for NULL. Is this just to
placate the Google Test macros?

[Ben Chan]

http://breakpad.appspot.com/332001/diff/9001/10009
File src/common/linux/memory_mapped_file_unittest.cc (right):

http://breakpad.appspot.com/332001/diff/9001/10009#newcode144
Line 144: }
On 2011/12/15 12:40:30, Ted Mielczarek wrote:
> The only case that I don't see but I'd expect to see tested here is calling
Map
> on an already-mapped MemoryMappedFile. Otherwise looks pretty comprehensive.

Done.

http://breakpad.appspot.com/332001/diff/9001/10010
File src/common/memory_range.h (right):

http://breakpad.appspot.com/332001/diff/9001/10010#newcode45
Line 45: // access of a subrange of the memory.Its implemementation does not
On 2011/12/15 12:40:30, Ted Mielczarek wrote:
> nit: space after the period.

Done.

http://breakpad.appspot.com/332001/diff/9001/10010#newcode83
Line 83: bool Covers(size_t offset, size_t length) const {
On 2011/12/15 12:40:30, Ted Mielczarek wrote:
> This method reads a little confusingly with offset vs. offset_ and length vs.
> length_. Maybe there are less-confusing alternate names you could use for the
> parameters?

Done.

Good point. I've changed the code to consistently use sub_offset and sub_length
when referring to a subrange.

http://breakpad.appspot.com/332001/diff/9001/10010#newcode94
Line 94: const void* GetObject(size_t offset, size_t length) const {
On 2011/12/15 12:40:30, Ted Mielczarek wrote:
> GetObject is kind of a weird name here when you're just returning a void*.

Done.

I adopted the same name used in md2core / core2md to minimize code change. But
now I think it isn't too bad to rename the method for better readability.

http://breakpad.appspot.com/332001/diff/9001/10010#newcode103
Line 103: unsigned index) const {
On 2011/12/15 12:40:30, Ted Mielczarek wrote:
> I wonder if this method wouldn't make more sense as a templated member? Then
you
> could just pass offset+index, use sizeof(T), and return const T*.

With an explicit element_size, GetArrayElement may take a forward declared
opaque type or an element size obtained at runtime (there is one case in
md2ocre). I've added a template helper for the common case.

http://breakpad.appspot.com/332001/diff/9001/10011
File src/common/memory_range_unittest.cc (right):

http://breakpad.appspot.com/332001/diff/9001/10011#newcode43
Line 43: const u_int8_t* const kNullPointer = NULL;
On 2011/12/15 12:40:30, Ted Mielczarek wrote:
> It seems a little unnecessary to define a constant for NULL. Is this just to
> placate the Google Test macros?

Done.

The googletest CmpHelperEQ/CmpHelperNE helpers sometimes have some issue
resolving NULL to the right template type when compared with gcc 4.6, e.g.

./src/testing/gtest/include/gtest/gtest.h: In function 'testing::AssertionResult
testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&)
[with T1 = long int, T2 = const void*]'

But it seems to work fine here, so I've changed to use NULL directly.

[kmixter]

lgtm once Ted is OK with it

[Michael Krebs]

http://breakpad.appspot.com/332001/diff/10014/9011
File src/common/memory_range.h (right):

http://breakpad.appspot.com/332001/diff/10014/9011#newcode90
Line 90: sub_offset + sub_length <= length_;
I think this expression is somewhat confusing.  At first, I figured it could be
reduced to "sub_offset >= 0 && sub_length >= 0 && sub_offset + sub_length <=
length".  But it occurred to me that this might actually be checking for
overflow in the sub_offset+sub_length calculation, while assuming they are both
at least zero (i.e. since size_t should be unsigned).

If you are guarding against overflow, I would put the "sub_offset + sub_length
>= sub_offset" in a separate "if (!...) return false" and attach a comment to
it.  If you are not assuming the sub_* values are at least zero (although size_t
is supposed to be unsigned), then I would put a separate check for those as
well.

I don't think the "sub_offset < length_" is needed.  The only way I believe it
can be false while "sub_offset + sub_length <= length_" is still true (barring
any overflow) is if sub_offset==length_ and sub_length==0, but it would seem
consistent to actually return true for that.

[Ben Chan]

http://breakpad.appspot.com/332001/diff/10014/9011
File src/common/memory_range.h (right):

http://breakpad.appspot.com/332001/diff/10014/9011#newcode90
Line 90: sub_offset + sub_length <= length_;
On 2011/12/16 03:55:58, Michael Krebs wrote:
> I think this expression is somewhat confusing.  At first, I figured it could
be
> reduced to "sub_offset >= 0 && sub_length >= 0 && sub_offset + sub_length <=
> length".  But it occurred to me that this might actually be checking for
> overflow in the sub_offset+sub_length calculation, while assuming they are
both
> at least zero (i.e. since size_t should be unsigned).
> 
> If you are guarding against overflow, I would put the "sub_offset + sub_length
> >= sub_offset" in a separate "if (!...) return false" and attach a comment to
> it.  If you are not assuming the sub_* values are at least zero (although
size_t
> is supposed to be unsigned), then I would put a separate check for those as
> well.
> 
> I don't think the "sub_offset < length_" is needed.  The only way I believe it
> can be false while "sub_offset + sub_length <= length_" is still true (barring
> any overflow) is if sub_offset==length_ and sub_length==0, but it would seem
> consistent to actually return true for that.
> 

I originally separated the checks but felt that it's harder to make sense out of
individual checks. The two conditions described in the comment seem to convey
the concept better. So I keep the comment in that form and literally translate
the conditions into an expression.
 
// 1. sub_offset is within [ 0 .. length_ - 1 ]
   sub_offset < length_   (sub_offset_ >= 0 implicitly checked due to size_t)

// 2. sub_offset + sub_length is within [ sub_offset .. length_ ]
   sub_offset + sub_length >= sub_offset && sub_offset + sub_length <= length_

"sub_offset + sub_length >= sub_offset" may seem redundant but it prevents
overflow or wrap-around issues if a negative value being passed as unsigned
size_t.

The "sub_offset < length_" check explicitly disallows "sub_offset==length_ and
sub_length==0", which is necessary as I want to ensure GetData() and
GetArrayElement never return a non-NULL pointer that goes beyond the end of the
buffer. A NULL pointer can be checked.

[Ted Mielczarek]

LGTM

[Michael Krebs]

The purist part of me wants to allow GetData() to return a pointer to the end of
the range when the length is zero (see my comment), but I don't need to be a
stickler about it.  If you think the way it is now makes MemoryRange a safer
implementation for users of it, I can respect that.  So LGTM with comment.

http://breakpad.appspot.com/332001/diff/10014/9011
File src/common/memory_range.h (right):

http://breakpad.appspot.com/332001/diff/10014/9011#newcode90
Line 90: sub_offset + sub_length <= length_;
> I originally separated the checks but felt that it's harder to make sense out
of
> individual checks. The two conditions described in the comment seem to convey
> the concept better. So I keep the comment in that form and literally translate
> the conditions into an expression.

It could be that it's just me that found it confusing.


> The "sub_offset < length_" check explicitly disallows "sub_offset==length_ and
> sub_length==0", which is necessary as I want to ensure GetData() and
> GetArrayElement never return a non-NULL pointer that goes beyond the end of
the
> buffer. A NULL pointer can be checked.

Why not let GetData() return a non-NULL pointer for that case (i.e. when
sub_length==0)?  It, for example, would allow someone to have an "end" pointer
to the memory range.  I found
http://stackoverflow.com/questions/988158/take-the-address-of-a-one-past-the-...
which explains why this particular case can be legal semantics for normal
pointers.