Add a SectionOffsetAddress class.

Add a SectionOffsetAddress class.

Pdb files contain address represented as segment/offset
pairs. This CL introduces a class to encapsulate this
concept.

It has the same interface as AddressImpl, except for the
operator- that accepts another address of the same type.
The class is a lightweight wrapper for 2 integers, which
can be freely copied.

BUG=
R=chrisha@chromium.org

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

[fdoray, 2014-07-31 19:32:08]

Could you review this CL?

I've not found other representations of a segment/offset in the existing code.
BlockGraph::Reference might be the most similar, but it's still very different.

To you knowledge, are "PdbAddress" used outside of PDBs? If so, I could move the
class somewhere else (in pe_lib maybe?).

Thanks.

[chrisha, 2014-07-31 20:33:31]

Take a look at core/address.h.


On Thu, Jul 31, 2014 at 3:32 PM, <fdoray@chromium.org> wrote:

> Reviewers: chrisha, etienneb, Sébastien Marchand,
>
> Message:
> Could you review this CL?
>
> I've not found other representations of a segment/offset in the existing
> code. BlockGraph::Reference might be the most similar, but it's still
> very different.
>
> To you knowledge, are "PdbAddress" used outside of PDBs? If so, I could
> move the class somewhere else (in pe_lib maybe?).
>
> Thanks.
>
> Description:
> Add a PdbAddress class.
>
> Pdb files contain address represented as segment/offset pairs. This
> CL introduces a class to encapsulate this concept.
>
> The operator< will be useful to output symbols in the right order when
> producing a PDB public stream.
>
> BUG=
>
> Please review this at https://codereview.appspot.com/114610045/
>
> Affected files (+121, -0 lines):
>   M syzygy/pdb/pdb.gyp
>   A syzygy/pdb/pdb_address.h
>   A syzygy/pdb/pdb_address_unittest.cc
>
>
>

[fdoray, 2014-07-31 20:48:13]

Do you suggest that I move my code to core/*?

I could use a template to let the user change the type used to represent the
segment and the offset.

[chrisha, 2014-07-31 21:44:36]

Yeah, that's the route I'd try first. Otherwise I'd simply define a
SectionOffsetAddress class directly, but with the same interface. Build
that around a "struct SectionOffset { uint32 section_id; uint32 offset; }"
with comparison operators and it should work with the rest of the
infrastructure.

Then I'd expand on PEFile and add appropriate "Translate" functions.


On Thu, Jul 31, 2014 at 4:48 PM, <fdoray@chromium.org> wrote:

> Do you suggest that I move my code to core/*?
>
> I could use a template to let the user change the type used to represent
> the segment and the offset.
>
> https://codereview.appspot.com/114610045/
>

[fdoray, 2014-08-01 18:49:36]

Could you take another look?

A SectionOffsetAddress has the same methods as an AddressImpl, modulo:
- No int32 operator-(const AddressImpl<type>& other) const, because it doesn't
make sense for that kind of address.
- New methods:
  - set_section_id() / section_id()
  - set_offset() / offset()

[chrisha, 2014-08-01 19:32:23]

Care to update your CL description and title while you're at it?

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
File syzygy/core/section_offset_address.cc (right):

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.cc:1: // Copyright 2011 Google Inc. All
Rights Reserved.
2014*

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.cc:13: // limitations under the License.
Blank line here.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
File syzygy/core/section_offset_address.h (right):

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:1: // Copyright 2011 Google Inc. All Rights
Reserved.
2014*

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:42: uint32 offset;
Maybe endow the SectionOffset with comparison operators, and make
SectionOffsetAddress use these? That was the .value() is comparable as well.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:50: SectionOffsetAddress(const
SectionOffsetAddress& other)  // NOLINT
We generally put a comment that we have a non-explicit copy constructor
specifically for STL container compatibility.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:59: return offset() < other.offset();
Keep the implementations out of the class definition. They can stay in the
header file, but preferably put them below the class definition. That way the
API can be seen from a quick glance without having to wade through
implementation detail.

I know that AddressImpl doesn't do this, but it should :)

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:131: return
SectionOffsetAddress(section_id(), aligned_offset);
Rather than reimplementing all of these you could punt to using AddressImpl.
After all, the various alignment functions only use the 'offset' parameter.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
File syzygy/core/section_offset_address_unittest.cc (right):

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address_unittest.cc:1: // Copyright 2011 Google Inc.
All Rights Reserved.
2014*

[fdoray, 2014-08-04 18:45:26]

All done.
Could you take another look?
Thanks!

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
File syzygy/core/section_offset_address.cc (right):

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.cc:1: // Copyright 2011 Google Inc. All
Rights Reserved.
On 2014/08/01 19:32:22, chrisha wrote:
> 2014*

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.cc:13: // limitations under the License.
On 2014/08/01 19:32:22, chrisha wrote:
> Blank line here.

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
File syzygy/core/section_offset_address.h (right):

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:1: // Copyright 2011 Google Inc. All Rights
Reserved.
On 2014/08/01 19:32:22, chrisha wrote:
> 2014*

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:42: uint32 offset;
On 2014/08/01 19:32:22, chrisha wrote:
> Maybe endow the SectionOffset with comparison operators, and make
> SectionOffsetAddress use these? That was the .value() is comparable as well.

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:50: SectionOffsetAddress(const
SectionOffsetAddress& other)  // NOLINT
On 2014/08/01 19:32:22, chrisha wrote:
> We generally put a comment that we have a non-explicit copy constructor
> specifically for STL container compatibility.

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:59: return offset() < other.offset();
On 2014/08/01 19:32:23, chrisha wrote:
> Keep the implementations out of the class definition. They can stay in the
> header file, but preferably put them below the class definition. That way the
> API can be seen from a quick glance without having to wade through
> implementation detail.
> 
> I know that AddressImpl doesn't do this, but it should :)

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address.h:131: return
SectionOffsetAddress(section_id(), aligned_offset);
On 2014/08/01 19:32:23, chrisha wrote:
> Rather than reimplementing all of these you could punt to using AddressImpl.
> After all, the various alignment functions only use the 'offset' parameter.

Done.

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
File syzygy/core/section_offset_address_unittest.cc (right):

https://codereview.appspot.com/114610045/diff/60001/syzygy/core/section_offse...
syzygy/core/section_offset_address_unittest.cc:1: // Copyright 2011 Google Inc.
All Rights Reserved.
On 2014/08/01 19:32:23, chrisha wrote:
> 2014*

Done.

[chrisha, 2014-08-04 20:04:32]

lgtm!

[fdoray, 2014-08-04 21:18:37]

Committed patchset #8 manually as 2224.