win: Add memory map range intersection helper

util/win/process_info.h

Index: util/win/process_info.h
diff --git a/util/win/process_info.h b/util/win/process_info.h
index 5af28bd762bddabcc62f2f8be5e7cb88150f7ff0..37bad539a3985d1b55c7211e6f679c0026a99984 100644
--- a/util/win/process_info.h
+++ b/util/win/process_info.h
@@ -23,6 +23,7 @@
 
 #include "base/basictypes.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/numeric/checked_range.h"
 #include "util/win/address_types.h"
 
 namespace crashpad {
@@ -52,6 +53,7 @@ class ProcessInfo {
   // \brief Contains information about a range of pages in the virtual address
   //    space of a process.
   struct MemoryInfo {
+    MemoryInfo();
     explicit MemoryInfo(const MEMORY_BASIC_INFORMATION& mbi);
     ~MemoryInfo();
 
@@ -157,6 +159,23 @@ class ProcessInfo {
   DISALLOW_COPY_AND_ASSIGN(ProcessInfo);
 };
 
+//! \brief Given a process memory map, gets a set of regions that correspond to

[Mark Mentovai, 2015/09/29 21:35:50]

“set” is slightly confusing when this returns a vector and there’s also
std::set.

[scottmg, 2015/09/30 17:40:26]

Done.

+//!     the readable regions (that are not `MEM_FREE`).
+//!
+//! \param[in] address The base address of the range being identified
+//! \param[in] size The size of the range being identified.
+//! \param[in] memory_info Memory map of a process, normally retrieved from
+//!     ProcessInfo::MemoryInformation().

[Mark Mentovai, 2015/09/29 21:35:50]

Since you mention MemoryInformation() and otherwise discuss the memory map, you
should also discuss coalescing.

[scottmg, 2015/09/30 17:40:26]

After renaming the function, I tried to rephrase the description so that it
makes it clear that it's retrieving the parts of the target process's memory
that are part of the input range, so that they'd be expected to be coalesced.

+//!
+//! \return A list of ranges corresponding to the portion of the range

[Mark Mentovai, 2015/09/29 21:35:50]

If the first mapping starts lower than address, will its base address be
adjusted upwards (and size adjusted downwards) to start at the address specified
by the user? If the last mapping ends higher than address + size, will its size
be adjusted downwards?

[scottmg, 2015/09/30 17:40:26]

I think this is addressed by the rename/description too.

+//!     identified by \a address and \a size that are readable based on the
+//!     memory map.
+std::vector<CheckedRange<WinVMAddress, WinVMSize>>

[Mark Mentovai, 2015/09/29 21:35:50]

I’m definitely cool with CheckedRange in the implementation, but I’m not so sure
about using it for the return value, especially when you haven’t really used
much of the range features or checked features of it. Maybe you’d be better off
with a simple std::pair<>. I can probably be talked into/out of however you want
to handle this, though.

[scottmg, 2015/09/30 17:40:26]

The main reason is that I don't like pair.first/.second in the receiving code. I
could make another (unchecked) Range, but there doesn't seem to be much upside?

[Mark Mentovai, 2015/10/01 17:38:58]

scottmg wrote:
It’s OK, now that you’re using the checking features of CheckedRange, it makes
more sense.

+GetAccessibleRangesInMemoryMap(

[Mark Mentovai, 2015/09/29 21:35:50]

Accessible → Readable? Based on the documentation above, I’d expect “readable”,
but based on the implementation, not so much.

[scottmg, 2015/09/30 17:40:26]

Renamed.

+    WinVMAddress address,
+    WinVMSize size,
+    const std::vector<ProcessInfo::MemoryInfo>& memory_info);

[Mark Mentovai, 2015/09/29 21:35:50]

It’s quirky for this to be a loose function that takes a vector<MemoryInfo>
instead of being a member of ProcessInfo that can get the memory map itself. I
can see that you probably did that for testability, but outside of testing, I
don’t know if that’s a great design. (As always, speak up if you had other
reasons.)

An alternative that still keeps the flexibility for testing would be to make
this a nonstatic method that’s backed by either
 - a private static helper using FRIEND_TEST (but that probably won’t work in
our configuration)
 - this function, advertising why it’s exposed in its documentation, and
advising the use of the member function instead

[scottmg, 2015/09/30 17:40:26]

Yes, it was primarily for testing. Added a member and noted that it should be
preferred.

+
 }  // namespace crashpad
 
 #endif  // CRASHPAD_UTIL_WIN_PROCESS_INFO_H_