Refactor rel32 searching process for x64 to make it more similar to x86.

Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=617965
Committed: https://crrev.com/7a43c3ede6d7db6e9ec25c80db8fbcd5ed80ad0f
Cr-Commit-Position: refs/heads/master@{#398326}

[etiennep, 2016-05-26 18:17:58]

PTAL

[huangs, 2016-05-26 18:34:33]

huangs@chromium.org changed reviewers:
+ huangs@chromium.org

[huangs, 2016-05-26 18:34:34]

High level comments.

https://codereview.chromium.org/2008253004/diff/1/courgette/assembly_program.cc
File courgette/assembly_program.cc (right):

https://codereview.chromium.org/2008253004/diff/1/courgette/assembly_program....
courgette/assembly_program.cc:409: /*case REL32ARM: {
Not sure what this is about?

https://codereview.chromium.org/2008253004/diff/1/courgette/encode_decode_uni...
File courgette/encode_decode_unittest.cc (right):

https://codereview.chromium.org/2008253004/diff/1/courgette/encode_decode_uni...
courgette/encode_decode_unittest.cc:86: TestAssembleToStreamDisassemble(file,
809776);
So algorithm changed?  Please separate pure refactoring from functional changes
in separate CLs.

https://codereview.chromium.org/2008253004/diff/1/courgette/rel32_finder.cc
File courgette/rel32_finder.cc (right):

https://codereview.chromium.org/2008253004/diff/1/courgette/rel32_finder.cc#n...
courgette/rel32_finder.cc:5: #include <stdint.h>
Do you need this include?

https://codereview.chromium.org/2008253004/diff/1/courgette/rel32_finder_x64_...
File courgette/rel32_finder_x64_unittest.cc (right):

https://codereview.chromium.org/2008253004/diff/1/courgette/rel32_finder_x64_...
courgette/rel32_finder_x64_unittest.cc:23: // Helper class to load and execute a
Rel32FinderX64 test case.
There's a lot of duplicate code with rel32_finder_x32_unittest.cc.  Perhaps just
have a single

rel32_finder_unittest.cc

and do everything there (possibly with parameters)?

https://codereview.chromium.org/2008253004/diff/1/courgette/rel32_finder_x64_...
courgette/rel32_finder_x64_unittest.cc:102: // We extract up to 7 bytes per
line. The remaining are ignored.
Keep it at 6; you can wrap instruction to next line, e.g.:
  00401091: 48 8B 05 00 00 00   mov     rax, 00401098
            00

https://codereview.chromium.org/2008253004/diff/1/courgette/testdata/rel32_x6...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/1/courgette/testdata/rel32_x6...
courgette/testdata/rel32_x64_03.txt:16: 00401003: E8 F8 EF FF FF        call   
00400000  # RVA start, outside .text
Fix misalignments.

[huangs, 2016-05-26 18:38:50]

Also, please mention Courgette somewhere in the title.

[etiennep, 2016-05-26 20:50:10]

I've undo the changes in the algorithm to make a separate CL. Addresses that
would have been detected in unit tests are commented out to be reinserted when
they are supported.
PTAnL

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/assembly_p...
File courgette/assembly_program.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/assembly_p...
courgette/assembly_program.cc:409: /*case REL32ARM: {
On 2016/05/26 18:34:34, huangs wrote:
> Not sure what this is about?

Whoops, no relation. Took it off.

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/encode_dec...
File courgette/encode_decode_unittest.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/encode_dec...
courgette/encode_decode_unittest.cc:86: TestAssembleToStreamDisassemble(file,
809776);
On 2016/05/26 18:34:34, huangs wrote:
> So algorithm changed?  Please separate pure refactoring from functional
changes
> in separate CLs.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/rel32_find...
File courgette/rel32_finder.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/rel32_find...
courgette/rel32_finder.cc:5: #include <stdint.h>
On 2016/05/26 18:34:34, huangs wrote:
> Do you need this include?

Nop.

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/rel32_find...
File courgette/rel32_finder_x64_unittest.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/rel32_find...
courgette/rel32_finder_x64_unittest.cc:23: // Helper class to load and execute a
Rel32FinderX64 test case.
On 2016/05/26 18:34:34, huangs wrote:
> There's a lot of duplicate code with rel32_finder_x32_unittest.cc.  Perhaps
just
> have a single
> 
> rel32_finder_unittest.cc
> 
> and do everything there (possibly with parameters)?

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/rel32_find...
courgette/rel32_finder_x64_unittest.cc:102: // We extract up to 7 bytes per
line. The remaining are ignored.
On 2016/05/26 18:34:34, huangs wrote:
> Keep it at 6; you can wrap instruction to next line, e.g.:
>   00401091: 48 8B 05 00 00 00   mov     rax, 00401098
>             00

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/testdata/r...
File courgette/testdata/rel32_x64_03.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/1/courgette/testdata/r...
courgette/testdata/rel32_x64_03.txt:16: 00401003: E8 F8 EF FF FF        call   
00400000  # RVA start, outside .text
On 2016/05/26 18:34:34, huangs wrote:
> Fix misalignments.

Done.

[huangs, 2016-05-26 22:11:18]

Need to
git add rel32_finder_unittest.cc
?

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder.cc
File courgette/rel32_finder.cc (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder....
courgette/rel32_finder.cc:7: #include "courgette/rel32_finder.h"
NIT: This should be at top since this is the "main .h".  You can do this by
deleting <stding.h> include though, since it's already in the main .h.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder.h
File courgette/rel32_finder.h (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder....
courgette/rel32_finder.h:36: // - Whose targets are in [|start_rva|, |end_rva|).
Update comment re. role of |size_of_image|?

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder....
courgette/rel32_finder.h:42: uint32_t size_of_image_,
|size_of_image|.  Trailing underscore is for private member variables of a
class.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
File courgette/rel32_finder_x64.cc (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:19: // subtract 'pointer_to_rva'.
NIT: |adjust_pointer_to_rva|.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:58: if ((*p & 0xFB) == 0x48 && *(p + 1) == 0x8D &&
Comments on these instructions?

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:82: // overlap.  Skip four bytes past the abs32
reloc.
NIT: Remove space before "Skip".

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
File courgette/rel32_finder_x64.h (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.h:18: // The basic implementation performs naive scan
for rel32 JMP and Jcc opcodes
Update comment.  Should also mention the extra stuff it does beyond JMP and Jcc.
 Perhaps move this to before Rel32FinderX64::Find.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
File courgette/rel32_finder_x86.h (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x86.h:18: // The basic implementation performs naive scan
for rel32 JMP and Jcc opcodes
Update comment (this is no longer the "basic" Rel32 finder).

Perhaps JMP/Jcc is too much implementation detail here.  Move them to before
Rel32FinderX86::Find in the .cc file?

[huangs, 2016-05-26 22:40:56]

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder.h
File courgette/rel32_finder.h (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder....
courgette/rel32_finder.h:20: Rel32Finder() = default;
If the default constructor is unused (perhaps by std::vector) then we may as
well exclude it.  I'm open to third opinion though (e.g., ask grt@).

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder....
courgette/rel32_finder.h:46: const RVA relocs_start_rva_ = 0;
Remove " = 0" if we remove default constructor.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
File courgette/rel32_finder_x64.cc (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:87: 
Why did the comment disappear?

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:91: if (target_rva < size_of_image &&  // Subsumes
rva != kUnassignedRVA.
target_rva < size_of_image

seems kinda bogus and we should revisit this later.  Leave it alone in this
refactoring CL.  But one thing we can do is to remove |size_of_image| from
Rel32FinderX64::Find() interface, and save as a member variable of
Rel32FinderX64, to be initialized at construction.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
File courgette/rel32_finder_x64.h (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x64.h:22: Rel32FinderX64() = default;
Remove default constructor if it turns out we don't need them.

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
File courgette/rel32_finder_x86.h (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/rel32_finder_...
courgette/rel32_finder_x86.h:22: Rel32FinderX86() = default;
Remove default constructor if it turns out we don't need them.

https://codereview.chromium.org/2008253004/diff/20001/courgette/testdata/rel3...
File courgette/testdata/rel32_x64_02.txt (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_02.txt:87: #107B
NIT: Space after "#" for consistency.  Also update the other test files.

https://codereview.chromium.org/2008253004/diff/20001/courgette/testdata/rel3...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/20001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_03.txt:1: # Test target validity: only accept
target RVA in [1000, 3000).
Why is there no rel32_x64_04.txt to match rel32_x86_04.txt?

https://codereview.chromium.org/2008253004/diff/20001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_03.txt:31: 00401048: FF 15 B1 EF FF FF  call       
003FFFFF # 1 byte before image
NIT: align #

https://codereview.chromium.org/2008253004/diff/20001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_03.txt:33: 00401054: FF 15 A5 3F 00 00  call       
00404FFF # 1 byte before end of image
If you're concerned with wrapping:
# 1 byte before end

[etiennep, 2016-05-27 18:12:34]

PTAnL

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
File courgette/rel32_finder.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder.cc:7: #include "courgette/rel32_finder.h"
On 2016/05/26 22:11:17, huangs wrote:
> NIT: This should be at top since this is the "main .h".  You can do this by
> deleting <stding.h> include though, since it's already in the main .h.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
File courgette/rel32_finder.h (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder.h:20: Rel32Finder() = default;
On 2016/05/26 22:40:55, huangs wrote:
> If the default constructor is unused (perhaps by std::vector) then we may as
> well exclude it.  I'm open to third opinion though (e.g., ask grt@).

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder.h:36: // - Whose targets are in [|start_rva|, |end_rva|).
On 2016/05/26 22:11:17, huangs wrote:
> Update comment re. role of |size_of_image|?

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder.h:42: uint32_t size_of_image_,
On 2016/05/26 22:11:17, huangs wrote:
> |size_of_image|.  Trailing underscore is for private member variables of a
> class.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder.h:46: const RVA relocs_start_rva_ = 0;
On 2016/05/26 22:40:55, huangs wrote:
> Remove " = 0" if we remove default constructor.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
File courgette/rel32_finder_x64.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.cc:19: // subtract 'pointer_to_rva'.
On 2016/05/26 22:11:17, huangs wrote:
> NIT: |adjust_pointer_to_rva|.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.cc:58: if ((*p & 0xFB) == 0x48 && *(p + 1) == 0x8D &&
On 2016/05/26 22:11:17, huangs wrote:
> Comments on these instructions?

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.cc:82: // overlap.  Skip four bytes past the abs32
reloc.
On 2016/05/26 22:11:17, huangs wrote:
> NIT: Remove space before "Skip".

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.cc:87: 
On 2016/05/26 22:40:55, huangs wrote:
> Why did the comment disappear?

Idk, it's back now!

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.cc:91: if (target_rva < size_of_image &&  // Subsumes
rva != kUnassignedRVA.
On 2016/05/26 22:40:55, huangs wrote:
> target_rva < size_of_image
> 
> seems kinda bogus and we should revisit this later.  Leave it alone in this
> refactoring CL.  But one thing we can do is to remove |size_of_image| from
> Rel32FinderX64::Find() interface, and save as a member variable of
> Rel32FinderX64, to be initialized at construction.

I moved size_of_image as an member variable. It would be tempting to put
abs32_locations as a member variable as well, since it's only of use for this
implementation (full disassemble wouldn't need to check collisions)

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
File courgette/rel32_finder_x64.h (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.h:18: // The basic implementation performs naive scan
for rel32 JMP and Jcc opcodes
On 2016/05/26 22:11:17, huangs wrote:
> Update comment.  Should also mention the extra stuff it does beyond JMP and
Jcc.
>  Perhaps move this to before Rel32FinderX64::Find.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x64.h:22: Rel32FinderX64() = default;
On 2016/05/26 22:40:55, huangs wrote:
> Remove default constructor if it turns out we don't need them.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
File courgette/rel32_finder_x86.h (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x86.h:18: // The basic implementation performs naive scan
for rel32 JMP and Jcc opcodes
On 2016/05/26 22:11:17, huangs wrote:
> Update comment (this is no longer the "basic" Rel32 finder).
> 
> Perhaps JMP/Jcc is too much implementation detail here.  Move them to before
> Rel32FinderX86::Find in the .cc file?

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/rel32_...
courgette/rel32_finder_x86.h:22: Rel32FinderX86() = default;
On 2016/05/26 22:40:55, huangs wrote:
> Remove default constructor if it turns out we don't need them.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/testda...
File courgette/testdata/rel32_x64_02.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/testda...
courgette/testdata/rel32_x64_02.txt:87: #107B
On 2016/05/26 22:40:55, huangs wrote:
> NIT: Space after "#" for consistency.  Also update the other test files.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/testda...
File courgette/testdata/rel32_x64_03.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/testda...
courgette/testdata/rel32_x64_03.txt:31: 00401048: FF 15 B1 EF FF FF  call       
003FFFFF # 1 byte before image
On 2016/05/26 22:40:55, huangs wrote:
> NIT: align #

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/20001/courgette/testda...
courgette/testdata/rel32_x64_03.txt:33: 00401054: FF 15 A5 3F 00 00  call       
00404FFF # 1 byte before end of image
On 2016/05/26 22:40:55, huangs wrote:
> If you're concerned with wrapping:
> # 1 byte before end

Done.

[huangs, 2016-05-30 05:48:35]

More comments, please see accompanying email for tool.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
File courgette/rel32_finder_unittest.cc (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_unittest.cc:35: template <class Finder>
Pretty cool use of template.  :)  In this instance though, the template
propagated to Rel32FinderTest::RunTest() and that's odd.

I think it would be cleaner to go "low tech": Use member variable

std::scoped_ptr<Rel32Finder> finder_;

Then add new config in each .txt test file to specify type, e.g.:

# Processor type
x86

Then in LoadTestFromString(), get this string  with ReadNonEmptyLine(), compare
and instantiate |finder_| accordingly.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_unittest.cc:165: }
Curious why there's no rel32_x64_04.txt?

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
File courgette/rel32_finder_x64.cc (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:17: //  rip mov/lea
NIT: Capitalize MOV/LEA for consistency (Jcc is fine though).  Also apply to
comments elsewhere in the file.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:27: // subtract 'adjust_pointer_to_rva'.
NIT: |adjust_pointer_to_rva|, now that it's a variable.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:65: // TODO(etiennep): Many rip mov/lea variants
are not detected.
NIT: ... Experiment and fix.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:67: if ((p[0] & 0xFB) == 0x48 &&  // Dst reg :
48/4C [rax-rdi]/[r8-r15]
Src or Dst?

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:69: (p[2] & 0xC7) == 0x05) {  // Dst reg : [05-3D]
NIT:
Dst reg: [05,0D,...,3D] = [r8-r15]

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:70: // rip relative lea QWORD
QWORD is 64-bits, but we only read 32-bits for this?

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:73: } else if ((p[0] & 0xFB) == 0x48 &&  // Dst
reg : 48/4C [rax-rdi]/[r8-r15]
Note that the two logic can be combined:

if ((p[1] == 0x8D || p[1] == 0x8B) &&  // LEA or MOV
    (p[0] & 0xFB) == 0x48 &&  // ...

Since you're refactoring, leaving as-is is a good idea.  Can add TODO though.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.cc:103: if (target_rva < size_of_image_ &&  //
Subsumes rva != kUnassignedRVA.
Re. your Patch Set #2 comment on moving |abs32_locations|: This is still needed
if we do some disassembly.  Disassembly is not fool-proof (e.g. switch
statements may have look up tables inlined in code).  Having overlapping abs32
and rel32 would break Courgette.  However, we can do this as a post-processing
step?  Anyway, leaving as-is for now is fine.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
File courgette/rel32_finder_x64.h (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x64.h:17: // This implementation performs naive scan for
opcodes having rel32
NIT: Unwrap comment.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
File courgette/rel32_finder_x86.cc (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x86.cc:23: // subtract 'adjust_pointer_to_rva'.
NIT: |adjust_pointer_to_rva|.

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
File courgette/rel32_finder_x86.h (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/rel32_finder_...
courgette/rel32_finder_x86.h:17: // This implementation performs naive scan for
opcodes having rel32
NIT: Unwrap comments.

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
File courgette/testdata/rel32_x64_01.txt (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_01.txt:13: Program:
Doh!  I forgot that I wrote a poor-man's disassembler script: It takes binary
data, pumps into dummy.cc, compiles, using CL, callls DUMPBIN, and filters the
results.  Mailed that to you, in case you want to check result and formatting.

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_01.txt:36: 00401079: 8B 05 00 00 00 00  mov        
eax, 00401079
This should be
mov   eax, [rip+00000000]

please change throughout.

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
File courgette/testdata/rel32_x64_02.txt (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_02.txt:39: 00401079: 8B 05 00 00 00 00  mov        
eax, 00401079
mov   eax, [rip+00000000]

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_02.txt:59: 004010C9: 5D                  pop       
ebp
NIT: Aligment.

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/80001/courgette/testdata/rel3...
courgette/testdata/rel32_x64_03.txt:40: 0040107E: 8D 05 7B EF FF FF  lea        
eax, 003FFFFF
lea  eax, [eip-00001085]  # 1 byte before image

etc.

[etiennep, 2016-05-30 17:07:31]

PTAnL

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
File courgette/rel32_finder_unittest.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_unittest.cc:35: template <class Finder>
On 2016/05/30 05:48:34, huangs wrote:
> Pretty cool use of template.  :)  In this instance though, the template
> propagated to Rel32FinderTest::RunTest() and that's odd.
> 
> I think it would be cleaner to go "low tech": Use member variable
> 
> std::scoped_ptr<Rel32Finder> finder_;
> 
> Then add new config in each .txt test file to specify type, e.g.:
> 
> # Processor type
> x86
> 
> Then in LoadTestFromString(), get this string  with ReadNonEmptyLine(),
compare
> and instantiate |finder_| accordingly.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_unittest.cc:165: }
On 2016/05/30 05:48:34, huangs wrote:
> Curious why there's no rel32_x64_04.txt?

I don't think testing false positive is relevant.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
File courgette/rel32_finder_x64.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:17: //  rip mov/lea
On 2016/05/30 05:48:34, huangs wrote:
> NIT: Capitalize MOV/LEA for consistency (Jcc is fine though).  Also apply to
> comments elsewhere in the file.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:27: // subtract 'adjust_pointer_to_rva'.
On 2016/05/30 05:48:34, huangs wrote:
> NIT: |adjust_pointer_to_rva|, now that it's a variable.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:65: // TODO(etiennep): Many rip mov/lea variants
are not detected.
On 2016/05/30 05:48:34, huangs wrote:
> NIT: ... Experiment and fix.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:67: if ((p[0] & 0xFB) == 0x48 &&  // Dst reg :
48/4C [rax-rdi]/[r8-r15]
On 2016/05/30 05:48:34, huangs wrote:
> Src or Dst?

Dst

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:69: (p[2] & 0xC7) == 0x05) {  // Dst reg : [05-3D]
On 2016/05/30 05:48:34, huangs wrote:
> NIT:
> Dst reg: [05,0D,...,3D] = [r8-r15]

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:70: // rip relative lea QWORD
On 2016/05/30 05:48:34, huangs wrote:
> QWORD is 64-bits, but we only read 32-bits for this?

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:73: } else if ((p[0] & 0xFB) == 0x48 &&  // Dst
reg : 48/4C [rax-rdi]/[r8-r15]
On 2016/05/30 05:48:34, huangs wrote:
> Note that the two logic can be combined:
> 
> if ((p[1] == 0x8D || p[1] == 0x8B) &&  // LEA or MOV
>     (p[0] & 0xFB) == 0x48 &&  // ...
> 
> Since you're refactoring, leaving as-is is a good idea.  Can add TODO though.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.cc:103: if (target_rva < size_of_image_ &&  //
Subsumes rva != kUnassignedRVA.
On 2016/05/30 05:48:34, huangs wrote:
> Re. your Patch Set #2 comment on moving |abs32_locations|: This is still
needed
> if we do some disassembly.  Disassembly is not fool-proof (e.g. switch
> statements may have look up tables inlined in code).  Having overlapping abs32
> and rel32 would break Courgette.  However, we can do this as a post-processing
> step?  Anyway, leaving as-is for now is fine.

Will talk about in next cl!

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
File courgette/rel32_finder_x64.h (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x64.h:17: // This implementation performs naive scan for
opcodes having rel32
On 2016/05/30 05:48:34, huangs wrote:
> NIT: Unwrap comment.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
File courgette/rel32_finder_x86.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x86.cc:23: // subtract 'adjust_pointer_to_rva'.
On 2016/05/30 05:48:35, huangs wrote:
> NIT: |adjust_pointer_to_rva|.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
File courgette/rel32_finder_x86.h (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/rel32_...
courgette/rel32_finder_x86.h:17: // This implementation performs naive scan for
opcodes having rel32
On 2016/05/30 05:48:35, huangs wrote:
> NIT: Unwrap comments.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
File courgette/testdata/rel32_x64_01.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
courgette/testdata/rel32_x64_01.txt:13: Program:
On 2016/05/30 05:48:35, huangs wrote:
> Doh!  I forgot that I wrote a poor-man's disassembler script: It takes binary
> data, pumps into dummy.cc, compiles, using CL, callls DUMPBIN, and filters the
> results.  Mailed that to you, in case you want to check result and formatting.

Acknowledged.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
courgette/testdata/rel32_x64_01.txt:36: 00401079: 8B 05 00 00 00 00  mov        
eax, 00401079
On 2016/05/30 05:48:35, huangs wrote:
> This should be
> mov   eax, [rip+00000000]
> 
> please change throughout.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
File courgette/testdata/rel32_x64_02.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
courgette/testdata/rel32_x64_02.txt:39: 00401079: 8B 05 00 00 00 00  mov        
eax, 00401079
On 2016/05/30 05:48:35, huangs wrote:
> mov   eax, [rip+00000000]

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
courgette/testdata/rel32_x64_02.txt:59: 004010C9: 5D                  pop       
ebp
On 2016/05/30 05:48:35, huangs wrote:
> NIT: Aligment.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
File courgette/testdata/rel32_x64_03.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/80001/courgette/testda...
courgette/testdata/rel32_x64_03.txt:40: 0040107E: 8D 05 7B EF FF FF  lea        
eax, 003FFFFF
On 2016/05/30 05:48:35, huangs wrote:
> lea  eax, [eip-00001085]  # 1 byte before image
> 
> etc.

Done.

[huangs, 2016-05-30 18:14:48]

Comment NITs and suggested fixes to test data.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
File courgette/rel32_finder_x64.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x64.cc:60: // rip relative call/jmp
NIT: CALL/JMP.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x64.cc:69: p[1] == 0x8D &&           // lea
NIT: LEA

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x64.cc:76: p[1] == 0x8B &&           // mov
NIT: MOV

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
File courgette/rel32_finder_x86.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x86.cc:23: // subtract 'adjust_pointer_to_rva'.
NIT: |adjust_pointer_to_rva|.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x64_01.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_01.txt:1: # Test all rel32 jump instructions.
NIT: Update comment:
# Test all rel32 jump instructions and supported rip accesses.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_01.txt:36: 0040106D: FF 15 00 00 00 00  call       
00401073
These are indirect, RIP-relative calls, i.e., load QWORD PTR at [rip+??], THEN
call the 64-address address.

0040106D: FF 15 00 00 00 00  call        [rip+00000000]  # 00401073
00401073: FF 25 00 00 00 00  jmp         [rip+00000000]  # 00401079

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x64_02.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:39: 0040106D: FF 15 00 00 00 00  call       
00401073
Indirect, RIP-relative calls

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:67: 105A  # Straddles across two
instruction.
NIT (my bad): "instructions."

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:68: 105F  # Same, but covers the op code
(and not the target) of the second.
The test checks collision with abs32 (and .reloc), so we should throw them in
(otherwise it's just identical to rel32_x64_01.txt).  Suggested addition:

10BA  # Straddles across two instructions, but covers the op code (and not the
target) of the second.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:98: 10B7
# 10B7

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x64_03.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_03.txt:33: 00401048: FF 15 B1 EF FF FF  call       
003FFFFF  # 1 byte before image
These are indirect, RIP-relative calls, i.e., load QWORD PTR at [rip+??], THEN
call the 64-address address.

00401048: FF 15 B1 EF FF FF  call        [rip-0000104F]  # 003FFFFF
                                                   # 1 byte before image
0040104E: FF 15 AC EF FF FF  call        [rip-00401054]  # 00400000
                                                   # In image

etc.

Same with jmp.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_03.txt:42: 0040107E: 8D 05 7B EF FF FF  lea        
eax,[rip-00001085]
I mentioned that "rip+..." should be used, but having the target address makes
it clearer, so have it in comments?

lea         eax,[rip-00001085]  # 003FFFFF
                        # 1 byte before image
lea         eax,[rip-0000108A]  # 00400000
                        # In image
lea         eax,[rip+00003F6F]  # 004FFFF0
                        # 1 byte before end
lea         eax,[rip+00003F6A]  # 00500000
                        # Outside image

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x86_02.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x86_02.txt:45: 105A  # Straddles across two
instruction.
NIT (my bad): "instructions."

[etiennep, 2016-06-01 17:23:41]

PTAnL

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
File courgette/rel32_finder_x64.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x64.cc:60: // rip relative call/jmp
On 2016/05/30 18:14:47, huangs wrote:
> NIT: CALL/JMP.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x64.cc:69: p[1] == 0x8D &&           // lea
On 2016/05/30 18:14:47, huangs wrote:
> NIT: LEA

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x64.cc:76: p[1] == 0x8B &&           // mov
On 2016/05/30 18:14:47, huangs wrote:
> NIT: MOV

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
File courgette/rel32_finder_x86.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/rel32...
courgette/rel32_finder_x86.cc:23: // subtract 'adjust_pointer_to_rva'.
On 2016/05/30 18:14:47, huangs wrote:
> NIT: |adjust_pointer_to_rva|.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x64_01.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_01.txt:1: # Test all rel32 jump instructions.
On 2016/05/30 18:14:47, huangs wrote:
> NIT: Update comment:
> # Test all rel32 jump instructions and supported rip accesses.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_01.txt:36: 0040106D: FF 15 00 00 00 00  call       
00401073
On 2016/05/30 18:14:47, huangs wrote:
> These are indirect, RIP-relative calls, i.e., load QWORD PTR at [rip+??], THEN
> call the 64-address address.
> 
> 0040106D: FF 15 00 00 00 00  call        [rip+00000000]  # 00401073
> 00401073: FF 25 00 00 00 00  jmp         [rip+00000000]  # 00401079

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x64_02.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:39: 0040106D: FF 15 00 00 00 00  call       
00401073
On 2016/05/30 18:14:48, huangs wrote:
> Indirect, RIP-relative calls

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:67: 105A  # Straddles across two
instruction.
On 2016/05/30 18:14:48, huangs wrote:
> NIT (my bad): "instructions."

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:68: 105F  # Same, but covers the op code
(and not the target) of the second.
On 2016/05/30 18:14:47, huangs wrote:
> The test checks collision with abs32 (and .reloc), so we should throw them in
> (otherwise it's just identical to rel32_x64_01.txt).  Suggested addition:
> 
> 10BA  # Straddles across two instructions, but covers the op code (and not the
> target) of the second.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_02.txt:98: 10B7
On 2016/05/30 18:14:47, huangs wrote:
> # 10B7

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x64_03.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_03.txt:33: 00401048: FF 15 B1 EF FF FF  call       
003FFFFF  # 1 byte before image
On 2016/05/30 18:14:48, huangs wrote:
> These are indirect, RIP-relative calls, i.e., load QWORD PTR at [rip+??], THEN
> call the 64-address address.
> 
> 00401048: FF 15 B1 EF FF FF  call        [rip-0000104F]  # 003FFFFF
>                                                    # 1 byte before image
> 0040104E: FF 15 AC EF FF FF  call        [rip-00401054]  # 00400000
>                                                    # In image
> 
> etc.
> 
> Same with jmp.

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x64_03.txt:42: 0040107E: 8D 05 7B EF FF FF  lea        
eax,[rip-00001085]
On 2016/05/30 18:14:48, huangs wrote:
> I mentioned that "rip+..." should be used, but having the target address makes
> it clearer, so have it in comments?
> 
> lea         eax,[rip-00001085]  # 003FFFFF
>                         # 1 byte before image
> lea         eax,[rip-0000108A]  # 00400000
>                         # In image
> lea         eax,[rip+00003F6F]  # 004FFFF0
>                         # 1 byte before end
> lea         eax,[rip+00003F6A]  # 00500000
>                         # Outside image
> 

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
File courgette/testdata/rel32_x86_02.txt (right):

https://chromiumcodereview.appspot.com/2008253004/diff/100001/courgette/testd...
courgette/testdata/rel32_x86_02.txt:45: 105A  # Straddles across two
instruction.
On 2016/05/30 18:14:48, huangs wrote:
> NIT (my bad): "instructions."

Done.

https://chromiumcodereview.appspot.com/2008253004/diff/120001/courgette/rel32...
File courgette/rel32_finder_x64.cc (right):

https://chromiumcodereview.appspot.com/2008253004/diff/120001/courgette/rel32...
courgette/rel32_finder_x64.cc:89: while (abs32_pos != abs32_locations.end() &&
*abs32_pos < rel32_rva - 3)
I believe this should be (rel32_rva - 7)

https://chromiumcodereview.appspot.com/2008253004/diff/120001/courgette/rel32...
courgette/rel32_finder_x64.cc:97: p += (*abs32_pos + 4) - current_rva;
and this +8

[huangs, 2016-06-01 20:57:21]

Awesome work!  LGTM with NITs on test data.

https://codereview.chromium.org/2008253004/diff/120001/courgette/rel32_finder...
File courgette/rel32_finder_x64.cc (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/rel32_finder...
courgette/rel32_finder_x64.cc:89: while (abs32_pos != abs32_locations.end() &&
*abs32_pos < rel32_rva - 3)
You're right.  Let's do this in a follow-up.

https://codereview.chromium.org/2008253004/diff/120001/courgette/rel32_finder...
courgette/rel32_finder_x64.cc:97: p += (*abs32_pos + 4) - current_rva;
Ditto.

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_01.txt (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_01.txt:38: 00401079: 8B 05 00 00 00 00  mov        
eax, [rip+00000000]
NIT: No space between "," and "[" (that's DUMPBIN's style, and in the other
files, too).

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_02.txt (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_02.txt:41: 00401079: 8B 05 00 00 00 00  mov        
eax, [rip+00000000]
NIT: No space between "," and "[" (that's DUMPBIN's style, and in the other
files, too).

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_02.txt:70: 10BA  # Same, but covers the op code
(and not the
NIT: Unwrap comment?

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:41: 00401060: FF 15 6A 88 C8 88  call       
[rip+88C8886A]      # 88C898D0
NIT: # 88C898D0 is incorrect.  Note that we're adding 32-bit, so staring with 8
will become negative!  I'd suggest:

00401060: FF 15 00 00 00 60  call [rip+60000000]  # 60401066

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:51: 0040107E: 8D 05 7B EF FF FF  lea        
eax,[rip-00001085]  # 003FFFF
NIT: missing F:
003FFFFF

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:72: 1074
1074  # x64 parsing ignores .text boundaries. TODO(eteinnep): Fix.

[etiennep, 2016-06-03 13:03:41]

Description was changed from

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.
In addition, a new type of instruction is detected by Rel32Finder64:
DWORD version of mov and lea (QWORD version was already being detected).
This may reduces patch size for Chrome64 by a small amount (~1%).

BUG=
==========

to

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=
==========

[etiennep, 2016-06-03 13:03:52]

The CQ bit was checked by etiennep@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 13:03:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2008253004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2008253004/140001

[commit-bot: I haz the power, 2016-06-03 14:27:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 14:27:09]

Dry run: This issue passed the CQ dry run.

[etiennep, 2016-06-03 15:39:40]

Ready to commit?

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_01.txt (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_01.txt:38: 00401079: 8B 05 00 00 00 00  mov        
eax, [rip+00000000]
On 2016/06/01 20:57:20, huangs wrote:
> NIT: No space between "," and "[" (that's DUMPBIN's style, and in the other
> files, too).

Done.

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_02.txt (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_02.txt:41: 00401079: 8B 05 00 00 00 00  mov        
eax, [rip+00000000]
On 2016/06/01 20:57:20, huangs wrote:
> NIT: No space between "," and "[" (that's DUMPBIN's style, and in the other
> files, too).

Done.

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_02.txt:70: 10BA  # Same, but covers the op code
(and not the
On 2016/06/01 20:57:20, huangs wrote:
> NIT: Unwrap comment?

Done.

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:41: 00401060: FF 15 6A 88 C8 88  call       
[rip+88C8886A]      # 88C898D0
On 2016/06/01 20:57:21, huangs wrote:
> NIT: # 88C898D0 is incorrect.  Note that we're adding 32-bit, so staring with
8
> will become negative!  I'd suggest:
> 
> 00401060: FF 15 00 00 00 60  call [rip+60000000]  # 60401066

Done.

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:51: 0040107E: 8D 05 7B EF FF FF  lea        
eax,[rip-00001085]  # 003FFFF
On 2016/06/01 20:57:20, huangs wrote:
> NIT: missing F:
> 003FFFFF

Done.

https://codereview.chromium.org/2008253004/diff/120001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:72: 1074
On 2016/06/01 20:57:21, huangs wrote:
> 1074  # x64 parsing ignores .text boundaries. TODO(eteinnep): Fix.

Added comment but no need to fix.

[huangs, 2016-06-03 15:59:06]

LGTM with small change.

https://codereview.chromium.org/2008253004/diff/140001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/140001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:41: 00401060: FF 15 6A 88 C8 88  call       
[rip+60000000]      # 60401066
Please also change the assembly bytes.

[etiennep, 2016-06-03 17:17:11]

etiennep@google.com changed reviewers:
+ wfh@chromium.org

[etiennep, 2016-06-03 17:17:12]

PTAL

https://codereview.chromium.org/2008253004/diff/140001/courgette/testdata/rel...
File courgette/testdata/rel32_x64_03.txt (right):

https://codereview.chromium.org/2008253004/diff/140001/courgette/testdata/rel...
courgette/testdata/rel32_x64_03.txt:41: 00401060: FF 15 6A 88 C8 88  call       
[rip+60000000]      # 60401066
On 2016/06/03 15:59:05, huangs wrote:
> Please also change the assembly bytes.

Done.

[Will Harris, 2016-06-03 21:41:41]

can you add a BUG=, otherwise defer to huangs existing lgtm

[etiennep, 2016-06-07 14:54:45]

Description was changed from

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=
==========

to

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=617965
==========

[etiennep, 2016-06-07 14:55:45]

The CQ bit was checked by etiennep@google.com

[etiennep, 2016-06-07 14:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from huangs@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2008253004/#ps160001 (title: "Correct far
away ptr in rel32_x64_03.txt test case")

[commit-bot: I haz the power, 2016-06-07 14:56:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2008253004/160001

[commit-bot: I haz the power, 2016-06-07 17:40:52]

Description was changed from

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=617965
==========

to

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=617965
==========

[commit-bot: I haz the power, 2016-06-07 17:40:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-07 17:42:32]

Description was changed from

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=617965
==========

to

==========
Refactor rel32 searching process for x64 to make it more similar to x86.

I created a base class Rel32Finder from which derives both Rel32FinderX86 and
Rel32Finder64.
This makes ParseRel32RelocsFromSection method more similar in x64 and x86.
Note that I removed the Win32 part of the name because it may be used by
DisassemblerElf.
I created test cases for Rel32FinderX64 which extends those for Rel32FinderX86.

BUG=617965

Committed: https://crrev.com/7a43c3ede6d7db6e9ec25c80db8fbcd5ed80ad0f
Cr-Commit-Position: refs/heads/master@{#398326}
==========

[commit-bot: I haz the power, 2016-06-07 17:42:34]

Patchset 9 (id:??) landed as
https://crrev.com/7a43c3ede6d7db6e9ec25c80db8fbcd5ed80ad0f
Cr-Commit-Position: refs/heads/master@{#398326}