Adding support for overlapping ranges to RangeMap.

Adding support for overlapping ranges to RangeMap.

When enabled, adding of a new range that overlaps with an existing one can be a successful operation.  The range which ends at the higher address will be shrunk down by moving its start position to a higher address so that it does not overlap anymore.

This change is required to fix http://crbug/611824.  The actual fix will come in a separate CL.

R=mmandlis@chromium.org

Committed: https://chromium.googlesource.com/breakpad/breakpad/+/240ed57ee1ac6a87b91526b8331717d494801826

[ivanpe, 2016-06-02 21:00:19]

Description was changed from

==========
Adding support for overlapping ranges to RangeMap.

When enabled, adding of a new range that overlaps with an existing one can be
a successful operation.  The range which ends at the higher address will be
shrunk down by moving its start position to a higher address so that it does
not overlap anymore.
==========

to

==========
Adding support for overlapping ranges to RangeMap.

When enabled, adding of a new range that overlaps with an existing one can be a
successful operation.  The range which ends at the higher address will be shrunk
down by moving its start position to a higher address so that it does not
overlap anymore.

This change is required to fix http://crbug/611824.  The actual fix will come in
a separate CL.
==========

[ivanpe, 2016-06-02 21:00:19]

ivanpe@chromium.org changed reviewers:
+ mark@chromium.org, mmandlis@chromium.org, primiano@chromium.org

[Mark Mentovai, 2016-06-02 21:43:53]

I’m going on vacation for about a week, and won’t be able to do follow-ups
reviews before you want to get this checked in, most likely. So I scrambled to
get you this feedback now!

https://codereview.chromium.org/2029953003/diff/1/aclocal.m4
File aclocal.m4 (right):

https://codereview.chromium.org/2029953003/diff/1/aclocal.m4#newcode1
aclocal.m4:1: # generated automatically by aclocal 1.14.1 -*- Autoconf -*-
Don’t downgrade if you can help it. Makefile.in too.

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
File src/processor/basic_code_modules.cc (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
src/processor/basic_code_modules.cc:48: BasicCodeModules::BasicCodeModules(const
CodeModules *that) : main_address_(0) {
Explicitly initialize map_ even if it’s just to _().

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
File src/processor/basic_code_modules.h (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
src/processor/basic_code_modules.h:44: #include <cstddef>
Use <stddef.h> unless you want to call it std::uint64_t, which you do not.

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
src/processor/basic_code_modules.h:79: RangeMap<uint64_t, linked_ptr<const
CodeModule>> map_;
I’m not sure we can guarantee C++11 in here. If not, leave the goofy spaced-out
> >.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map-inl.h
File src/processor/range_map-inl.h (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map-inl...
src/processor/range_map-inl.h:124: // This is most appropriate for a DEBUG
channel, but since none exists now
Rewrap this comment.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h
File src/processor/range_map.h (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
src/processor/range_map.h:69: bool StoreRange(const AddressType &base, const
AddressType &delta,
Is anything ever stored with a delta other than 0, now or in the future? If
delta is just modified internally when shrink-down is on, but is always 0 here,
then we can eliminate this parameter.

If nonzero deltas are only used internally, provide a private method
(StoreRangeInternal()?) that accepts a delta argument, and remove the delta
argument from this public version. Have the public one call the internal private
version by passing 0 for the delta.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
src/processor/range_map.h:138: // Whether overlapping ranges can be shrunk down.
 If true, then adding
Don’t duplicate the comment from the setter. Either put it here or there, but
not both.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
src/processor/range_map.h:154: 
Lose the blank line here, and actually at the end of a lot of (maybe all?) files
you’re touching.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map_shr...
File src/processor/range_map_shrink_down_unittest.cc (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map_shr...
src/processor/range_map_shrink_down_unittest.cc:272: }
If storing ranges with a nonzero delta is supported from the public interface,
you should test it.

[ivanpe, 2016-06-02 22:54:02]

Done. PTAL.

https://codereview.chromium.org/2029953003/diff/1/aclocal.m4
File aclocal.m4 (right):

https://codereview.chromium.org/2029953003/diff/1/aclocal.m4#newcode1
aclocal.m4:1: # generated automatically by aclocal 1.14.1 -*- Autoconf -*-
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Don’t downgrade if you can help it. Makefile.in too.

Good point.  I was able to install the same version of automake and use it.

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
File src/processor/basic_code_modules.cc (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
src/processor/basic_code_modules.cc:48: BasicCodeModules::BasicCodeModules(const
CodeModules *that) : main_address_(0) {
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Explicitly initialize map_ even if it’s just to _().

Done.

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
File src/processor/basic_code_modules.h (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
src/processor/basic_code_modules.h:44: #include <cstddef>
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Use <stddef.h> unless you want to call it std::uint64_t, which you do not.

Done.

https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
src/processor/basic_code_modules.h:79: RangeMap<uint64_t, linked_ptr<const
CodeModule>> map_;
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> I’m not sure we can guarantee C++11 in here. If not, leave the goofy
spaced-out
> > >.

Done.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map-inl.h
File src/processor/range_map-inl.h (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map-inl...
src/processor/range_map-inl.h:124: // This is most appropriate for a DEBUG
channel, but since none exists now
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Rewrap this comment.

Done.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h
File src/processor/range_map.h (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
src/processor/range_map.h:69: bool StoreRange(const AddressType &base, const
AddressType &delta,
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Is anything ever stored with a delta other than 0, now or in the future? If
> delta is just modified internally when shrink-down is on, but is always 0
here,
> then we can eliminate this parameter.
> 
> If nonzero deltas are only used internally, provide a private method
> (StoreRangeInternal()?) that accepts a delta argument, and remove the delta
> argument from this public version. Have the public one call the internal
private
> version by passing 0 for the delta.

Yes, I agree that moving the delta to a private method would be better.
Initially I thought that we should keep it for future use, but after revisiting
this idea, I think that even if we decide to set a "delta" from the breakpad
client (which is likely not happening) it will have a slightly different meaning
so we are not going to use this same parameter for that anyways.

I moved it to a private StoreRangeInternal() method.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
src/processor/range_map.h:138: // Whether overlapping ranges can be shrunk down.
 If true, then adding
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Don’t duplicate the comment from the setter. Either put it here or there, but
> not both.

Done.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
src/processor/range_map.h:154: 
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> Lose the blank line here, and actually at the end of a lot of (maybe all?)
files
> you’re touching.

Done.

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map_shr...
File src/processor/range_map_shrink_down_unittest.cc (right):

https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map_shr...
src/processor/range_map_shrink_down_unittest.cc:272: }
On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> If storing ranges with a nonzero delta is supported from the public interface,
> you should test it.

I disabled it.  Now it is supported only by a private method.

[ivanpe, 2016-06-02 22:56:03]

On 2016/06/02 22:54:02, ivanpe wrote:
> Done. PTAL.
> 
> https://codereview.chromium.org/2029953003/diff/1/aclocal.m4
> File aclocal.m4 (right):
> 
> https://codereview.chromium.org/2029953003/diff/1/aclocal.m4#newcode1
> aclocal.m4:1: # generated automatically by aclocal 1.14.1 -*- Autoconf -*-
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Don’t downgrade if you can help it. Makefile.in too.
> 
> Good point.  I was able to install the same version of automake and use it.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
> File src/processor/basic_code_modules.cc (right):
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
> src/processor/basic_code_modules.cc:48:
BasicCodeModules::BasicCodeModules(const
> CodeModules *that) : main_address_(0) {
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Explicitly initialize map_ even if it’s just to _().
> 
> Done.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
> File src/processor/basic_code_modules.h (right):
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
> src/processor/basic_code_modules.h:44: #include <cstddef>
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Use <stddef.h> unless you want to call it std::uint64_t, which you do not.
> 
> Done.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/basic_code_mo...
> src/processor/basic_code_modules.h:79: RangeMap<uint64_t, linked_ptr<const
> CodeModule>> map_;
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > I’m not sure we can guarantee C++11 in here. If not, leave the goofy
> spaced-out
> > > >.
> 
> Done.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map-inl.h
> File src/processor/range_map-inl.h (right):
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map-inl...
> src/processor/range_map-inl.h:124: // This is most appropriate for a DEBUG
> channel, but since none exists now
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Rewrap this comment.
> 
> Done.
> 
> https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h
> File src/processor/range_map.h (right):
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
> src/processor/range_map.h:69: bool StoreRange(const AddressType &base, const
> AddressType &delta,
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Is anything ever stored with a delta other than 0, now or in the future? If
> > delta is just modified internally when shrink-down is on, but is always 0
> here,
> > then we can eliminate this parameter.
> > 
> > If nonzero deltas are only used internally, provide a private method
> > (StoreRangeInternal()?) that accepts a delta argument, and remove the delta
> > argument from this public version. Have the public one call the internal
> private
> > version by passing 0 for the delta.
> 
> Yes, I agree that moving the delta to a private method would be better.
> Initially I thought that we should keep it for future use, but after
revisiting
> this idea, I think that even if we decide to set a "delta" from the breakpad
> client (which is likely not happening) it will have a slightly different
meaning
> so we are not going to use this same parameter for that anyways.
> 
> I moved it to a private StoreRangeInternal() method.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
> src/processor/range_map.h:138: // Whether overlapping ranges can be shrunk
down.
>  If true, then adding
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Don’t duplicate the comment from the setter. Either put it here or there,
but
> > not both.
> 
> Done.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map.h#n...
> src/processor/range_map.h:154: 
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > Lose the blank line here, and actually at the end of a lot of (maybe all?)
> files
> > you’re touching.
> 
> Done.
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map_shr...
> File src/processor/range_map_shrink_down_unittest.cc (right):
> 
>
https://codereview.chromium.org/2029953003/diff/1/src/processor/range_map_shr...
> src/processor/range_map_shrink_down_unittest.cc:272: }
> On 2016/06/02 21:43:53, Mark Mentovai (emoji for OOO) wrote:
> > If storing ranges with a nonzero delta is supported from the public
interface,
> > you should test it.
> 
> I disabled it.  Now it is supported only by a private method.

Mark, if you cannot take another look, I'll only wait for Maria and Primiano for
the second round.  Have a nice vacation!

[mmandlis, 2016-06-02 23:50:59]

https://codereview.chromium.org/2029953003/diff/20001/src/processor/basic_sou...
File src/processor/basic_source_line_resolver.cc (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/basic_sou...
src/processor/basic_source_line_resolver.cc:214: if
(func->lines.RetrieveRange(address, &line, &line_base, NULL, NULL)) {
please, add comments for the NULL parameters.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/minidump.cc
File src/processor/minidump.cc (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/minidump....
src/processor/minidump.cc:2675: if (!range_map_->RetrieveRange(address,
&module_index, NULL, NULL, NULL)) {
nit - please, add comments for the NULL parameters.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
File src/processor/range_map-inl.h (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:74: << HexString(base) << "+" << HexString(size)
should you add the |delta| to the log as well?

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:79: // Ensure that this range does not overlap
with another one already in the
You might want to rephrase the comment "Ensure that this range does not overlap"
- ensure doesn't sound quire right anymore.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:91: return StoreRangeInternal(base +
additional_delta,
in the comments you explained that the address with the highest end address will
be shrunk. is it always the one that is being saved? can it be another entry
that is already in the map?

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:93: size - additional_delta, entry);
is the recursive call really necessary, or could you just use updated local
variables base, delta and size? or can there be more than one level of
recursion? if yes, can you give an example of addresses it would happen for?

thanks.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:95: // AddressType other_base =
iterator_base->second.base();
there was originally this comment explaining:
"
// The processor hits this case too frequently with common symbol files.	      
// this is an error.
// This is most appropriate for a DEBUG channel, but since none exists now	     
 // simply comment out this logging."

it got removed with your change. Without it, it is not clear why we are keeping
these commented-out lines.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map.h
File src/processor/range_map.h (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map.h:68: // shrink down the range which ends at a hugher
address.
typo - higher

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map.h:74: // non-NULL, are set to the base, delta, and size
of the entry's range.
could you, please, add comment on what entry_delta is?

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map.h:97: AddressType *entry_size) const;
optional - probably not worth doing, but just smth to consider.
All methods here take entry_base, entry_delta and entry_size. Maybe it's worth
defining an EntryAddress class with these 3 fields?

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
File src/processor/range_map_shrink_down_unittest.cc (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map_shrink_down_unittest.cc:1: // Copyright (c) 2010, Google
Inc.
2016?

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map_shrink_down_unittest.cc:124: TEST(RangeMap,
TestShinkDown_CompletelyContained_2) {
nit - maybe use more descriptive test case names, rather than _1 and _2?

[Mark Mentovai, 2016-06-03 00:30:13]

Thanks! I won’t be able to make another pass before my trip, but I see that
Maria’s already on it. No need to wait for further review from me.

[ivanpe, 2016-06-03 03:56:56]

Done. PTAL.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/basic_sou...
File src/processor/basic_source_line_resolver.cc (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/basic_sou...
src/processor/basic_source_line_resolver.cc:214: if
(func->lines.RetrieveRange(address, &line, &line_base, NULL, NULL)) {
On 2016/06/02 23:50:59, mmandlis wrote:
> please, add comments for the NULL parameters.

Done.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
File src/processor/range_map-inl.h (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:74: << HexString(base) << "+" << HexString(size)
On 2016/06/02 23:50:59, mmandlis wrote:
> should you add the |delta| to the log as well?

Done.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:79: // Ensure that this range does not overlap
with another one already in the
On 2016/06/02 23:50:59, mmandlis wrote:
> You might want to rephrase the comment "Ensure that this range does not
overlap"
> - ensure doesn't sound quire right anymore.

By default, shrink-down is off, so in the common case this is still valid.  The
comments that follow provide more details about the cases where shrink-down is
on.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:91: return StoreRangeInternal(base +
additional_delta,
On 2016/06/02 23:50:59, mmandlis wrote:
> in the comments you explained that the address with the highest end address
will
> be shrunk. is it always the one that is being saved? can it be another entry
> that is already in the map?

Yes, the one that is shrunk can already be in the map.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:93: size - additional_delta, entry);
On 2016/06/02 23:50:59, mmandlis wrote:
> is the recursive call really necessary, or could you just use updated local
> variables base, delta and size? or can there be more than one level of
> recursion? if yes, can you give an example of addresses it would happen for?
> 
> thanks.

I feel it is cleaner and easier to understand with recursion.  You may have
multiple ranges that overlap the beginning of the new range in whcih case you
may need to shrink several times in a row until all the conflicts are resolved.

I added a test case to specifically cover this this scenario (the last test
case).

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map-inl.h:95: // AddressType other_base =
iterator_base->second.base();
On 2016/06/02 23:50:59, mmandlis wrote:
> there was originally this comment explaining:
> "
> // The processor hits this case too frequently with common symbol files.	     

> // this is an error.
> // This is most appropriate for a DEBUG channel, but since none exists now	   
 
>  // simply comment out this logging."
> 
> it got removed with your change. Without it, it is not clear why we are
keeping
> these commented-out lines.

Good point.  Added it back.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map.h
File src/processor/range_map.h (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map.h:68: // shrink down the range which ends at a hugher
address.
On 2016/06/02 23:50:59, mmandlis wrote:
> typo - higher

Done.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map.h:74: // non-NULL, are set to the base, delta, and size
of the entry's range.
On 2016/06/02 23:50:59, mmandlis wrote:
> could you, please, add comment on what entry_delta is?

Done.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map.h:97: AddressType *entry_size) const;
On 2016/06/02 23:50:59, mmandlis wrote:
> optional - probably not worth doing, but just smth to consider.
> All methods here take entry_base, entry_delta and entry_size. Maybe it's worth
> defining an EntryAddress class with these 3 fields?

Acknowledged.

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
File src/processor/range_map_shrink_down_unittest.cc (right):

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map_shrink_down_unittest.cc:1: // Copyright (c) 2010, Google
Inc.
On 2016/06/02 23:50:59, mmandlis wrote:
> 2016?

Doh!

https://codereview.chromium.org/2029953003/diff/20001/src/processor/range_map...
src/processor/range_map_shrink_down_unittest.cc:124: TEST(RangeMap,
TestShinkDown_CompletelyContained_2) {
On 2016/06/02 23:50:59, mmandlis wrote:
> nit - maybe use more descriptive test case names, rather than _1 and _2?

Done.

[mmandlis, 2016-06-03 05:20:43]

lgtm

please, address comments before submitting.

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map.h
File src/processor/range_map.h (right):

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
src/processor/range_map.h:74: // non-NULL, are set to the base, delta, and size
of the entry's range.i
nit - accidental "i" character at the end of the line.

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
File src/processor/range_map_shrink_down_unittest.cc (right):

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
src/processor/range_map_shrink_down_unittest.cc:72: TEST(RangeMap,
TestShinkDown_SameRange) {
For completeness sake, could you, please, add a test case when shrinkdown is
enabled, but there is no overlap actually?

(When shrink down is disabled, and there is an overlap - this is tested in the
other test file, right?)

[ivanpe, 2016-06-06 05:24:26]

On 2016/06/03 05:20:43, mmandlis wrote:
> lgtm
> 
> please, address comments before submitting.
> 
>
https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map.h
> File src/processor/range_map.h (right):
> 
>
https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
> src/processor/range_map.h:74: // non-NULL, are set to the base, delta, and
size
> of the entry's range.i
> nit - accidental "i" character at the end of the line.
> 
>
https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
> File src/processor/range_map_shrink_down_unittest.cc (right):
> 
>
https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
> src/processor/range_map_shrink_down_unittest.cc:72: TEST(RangeMap,
> TestShinkDown_SameRange) {
> For completeness sake, could you, please, add a test case when shrinkdown is
> enabled, but there is no overlap actually?
> 
> (When shrink down is disabled, and there is an overlap - this is tested in the
> other test file, right?)

All done.  Committing shortly.

[ivanpe, 2016-06-06 05:24:44]

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map.h
File src/processor/range_map.h (right):

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
src/processor/range_map.h:74: // non-NULL, are set to the base, delta, and size
of the entry's range.i
On 2016/06/03 05:20:43, mmandlis wrote:
> nit - accidental "i" character at the end of the line.

Done.

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
File src/processor/range_map_shrink_down_unittest.cc (right):

https://codereview.chromium.org/2029953003/diff/60001/src/processor/range_map...
src/processor/range_map_shrink_down_unittest.cc:72: TEST(RangeMap,
TestShinkDown_SameRange) {
On 2016/06/03 05:20:43, mmandlis wrote:
> For completeness sake, could you, please, add a test case when shrinkdown is
> enabled, but there is no overlap actually?
> 
> (When shrink down is disabled, and there is an overlap - this is tested in the
> other test file, right?)

I added a test case with no overlap (conflict).  The second test case you
mentioned is already covered in a different test file (range_map_unittest.cc).

[ivanpe, 2016-06-06 05:41:14]

Description was changed from

==========
Adding support for overlapping ranges to RangeMap.

When enabled, adding of a new range that overlaps with an existing one can be a
successful operation.  The range which ends at the higher address will be shrunk
down by moving its start position to a higher address so that it does not
overlap anymore.

This change is required to fix http://crbug/611824.  The actual fix will come in
a separate CL.
==========

to

==========
Adding support for overlapping ranges to RangeMap.

When enabled, adding of a new range that overlaps with an existing one can be a
successful operation.  The range which ends at the higher address will be shrunk
down by moving its start position to a higher address so that it does not
overlap anymore.

This change is required to fix http://crbug/611824.  The actual fix will come in
a separate CL.

R=mmandlis@chromium.org

Committed:
https://chromium.googlesource.com/breakpad/breakpad/+/240ed57ee1ac6a87b91526b...
==========

[ivanpe, 2016-06-06 05:41:19]

Committed patchset #6 (id:100001) manually as
240ed57ee1ac6a87b91526b8331717d494801826 (presubmit successful).