[Courgette] Clean up Disassembler; fix ELF Memory leaks.

[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206
Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}

Committed: https://crrev.com/dda11d069213d53aef9bf3bc018b02f3aef7177b
Cr-Commit-Position: refs/heads/master@{#380987}

[huangs, 2016-02-05 21:13:38]

Description was changed from

==========
[Courgette] Cleanup Disassembler; fixing ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines, shying away from control flow
and logic changes.
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines, shying away from control flow
and logic changes.
==========

[huangs, 2016-02-05 21:15:02]

huangs@chromium.org changed reviewers:
+ grt@chromium.org, wfh@google.com

[huangs, 2016-02-05 21:15:02]

Lots of small changes in Disassembler.  Want to get them out of the way before
changing control flow to use LabelManager.  PTAL.  Thank!

[huangs, 2016-02-05 21:19:05]

https://codereview.chromium.org/1676683002/diff/1/courgette/disassembler.h
File courgette/disassembler.h (right):

https://codereview.chromium.org/1676683002/diff/1/courgette/disassembler.h#ne...
courgette/disassembler.h:19: class Disassembler : public AddressTranslator {
Going against "composition over inheritance", but in this case this the
complexity cost of having an AddressTranslator structure parallel to
Disassembler is not worthwhile.

[grt (UTC plus 2), 2016-02-11 16:55:33]

grt@chromium.org changed reviewers:
+ wfh@chromium.org
- wfh@google.com

[grt (UTC plus 2), 2016-02-11 16:55:34]

a few tiny comments to start with.

https://codereview.chromium.org/1676683002/diff/1/courgette/image_utils.h
File courgette/image_utils.h (right):

https://codereview.chromium.org/1676683002/diff/1/courgette/image_utils.h#new...
courgette/image_utils.h:29: using FileOffset = size_t;
is ptrdiff_t (the type resulting from subtracting one pointer from another) more
appropriate for a file offset?

https://codereview.chromium.org/1676683002/diff/1/courgette/image_utils.h#new...
courgette/image_utils.h:30: const FileOffset kNoFileOffset = 0xFFFFFFFFU;
this should be something like PTRDIFF_MAX (if FileOffset is a ptrdiff_t) or
UINTPTR_MAX (if FileOffset is a size_t).

https://codereview.chromium.org/1676683002/diff/1/courgette/image_utils.h#new...
courgette/image_utils.h:33: class AddressTranslator {
will this eventually:
- be implemented by types other than Disassembler
- be consumed by other functions
?

https://codereview.chromium.org/1676683002/diff/1/courgette/rel32_finder_win3...
File courgette/rel32_finder_win32_x86.cc (right):

https://codereview.chromium.org/1676683002/diff/1/courgette/rel32_finder_win3...
courgette/rel32_finder_win32_x86.cc:101: start_rva <= target_rva && target_rva <
end_rva) {
is it valid for end_rva to be > image_end_rva_? can this be simplified to
"target_rva < end_rva && target_rva >= start_rva"?

[huangs, 2016-02-12 01:08:42]

Updated, still need to remove relocs_start_rva from Rel32FinderWin32X86.

https://chromiumcodereview.appspot.com/1676683002/diff/1/courgette/image_utils.h
File courgette/image_utils.h (right):

https://chromiumcodereview.appspot.com/1676683002/diff/1/courgette/image_util...
courgette/image_utils.h:29: using FileOffset = size_t;
Seems more correct.  However, when I implement this, I get signed-unsigned
comparison error, especially for comparing against uint32_t size values from
image sections.  I'd rather keep as-is?

https://chromiumcodereview.appspot.com/1676683002/diff/1/courgette/image_util...
courgette/image_utils.h:30: const FileOffset kNoFileOffset = 0xFFFFFFFFU;
Done (UINTPTR_MAX for size_t).

https://chromiumcodereview.appspot.com/1676683002/diff/1/courgette/image_util...
courgette/image_utils.h:33: class AddressTranslator {
Yes. The plan is to create multiple subclasses ( per {abs32, rel32} per
architecture) of class RvaVisitor to translate from RVA locations to RVA
targets.  These classes need to translate addresses, so each Disassembler will
pass *this (as AddressTranslator) to each RvaVisitor on instantiation.

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

https://chromiumcodereview.appspot.com/1676683002/diff/1/courgette/rel32_find...
courgette/rel32_finder_win32_x86.cc:101: start_rva <= target_rva && target_rva <
end_rva) {
The original code intends to separate the "is RVA valid" and "is RVA in range"
semantics, but I suppose this is weak enough to be overridden by non-redundancy.

Note that this renders |image_end_rva_| useless.  I'll do this removal of
|image_end_rva_| (also from tests) in a follow-up patch.

[huangs, 2016-02-12 02:21:02]

I meant removing image_end_rva_ from Rel32FinderWin32X86_Basic.

[huangs, 2016-02-26 16:27:20]

Ping for review; PTAL. Thanks!

[huangs, 2016-02-29 20:44:55]

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

[huangs, 2016-03-08 18:22:38]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines, shying away from control flow
and logic changes.
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines, shying away from control flow
and logic changes.

BUG=579206
==========

[huangs, 2016-03-08 18:22:39]

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

[huangs, 2016-03-08 18:23:30]

huangs@chromium.org changed reviewers:
+ andrewhayden@chromium.org
- picksi@chromium.org

[huangs, 2016-03-08 18:24:09]

Ping reviewers.

+andrewhayden@, PTAL ELF and/or ARM -related changes.  Thanks!

[Will Harris, 2016-03-08 19:06:04]

lgtm % nits - no functional changes found. Nice restructuring.

[Will Harris, 2016-03-08 19:09:07]

sorry pushed wrong button here are the nits/comments.

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
File courgette/disassembler_elf_32.cc (right):

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
courgette/disassembler_elf_32.cc:107:
SectionBody(static_cast<int>(header_->e_shstrndx)));
not sure why this cast is needed - e_shstrndx is an Elf32_Half which is
uint16_t.

Why don't SectionBody and SectionHeader just take Elf32_Half?

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
File courgette/disassembler_win32_x64.cc (right):

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
courgette/disassembler_win32_x64.cc:526: (is_rip_relative ||
is git cl format okay with this indenting on line 526?

[huangs, 2016-03-09 17:53:30]

Thanks!

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
File courgette/disassembler_elf_32.cc (right):

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
courgette/disassembler_elf_32.cc:107:
SectionBody(static_cast<int>(header_->e_shstrndx)));
Nice! Done.

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
File courgette/disassembler_win32_x64.cc (right):

https://chromiumcodereview.appspot.com/1676683002/diff/60001/courgette/disass...
courgette/disassembler_win32_x64.cc:526: (is_rip_relative ||
"git cl format" recommends single relative indent.  Going to apply some other
recommendations as well.

[huangs, 2016-03-09 21:30:24]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 21:30:46]

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

[commit-bot: I haz the power, 2016-03-09 21:40:21]

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

[commit-bot: I haz the power, 2016-03-09 21:40:22]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[huangs, 2016-03-09 23:10:52]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 23:11:18]

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

[commit-bot: I haz the power, 2016-03-09 23:16:55]

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

[commit-bot: I haz the power, 2016-03-09 23:16:56]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[huangs, 2016-03-10 06:57:21]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 06:57:42]

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

[commit-bot: I haz the power, 2016-03-10 07:15:33]

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

[commit-bot: I haz the power, 2016-03-10 07:15:34]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[huangs, 2016-03-10 07:27:03]

The CQ bit was checked by huangs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 07:27:20]

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

[commit-bot: I haz the power, 2016-03-10 08:03:04]

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

[commit-bot: I haz the power, 2016-03-10 08:03:05]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[huangs, 2016-03-10 18:46:36]

The CQ bit was checked by huangs@chromium.org

[huangs, 2016-03-10 18:46:37]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1676683002/#ps230001 (title: "Param fix
for ProgramSegmentHeader(); fix comments.")

[commit-bot: I haz the power, 2016-03-10 18:47:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1676683002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1676683002/230001

[commit-bot: I haz the power, 2016-03-10 20:13:25]

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

[commit-bot: I haz the power, 2016-03-10 20:13:27]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[huangs, 2016-03-12 19:44:48]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines, shying away from control flow
and logic changes.

BUG=579206
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206
==========

[huangs, 2016-03-12 19:45:52]

The CQ bit was checked by huangs@chromium.org

[huangs, 2016-03-12 19:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1676683002/#ps270001 (title: "Sync.")

[commit-bot: I haz the power, 2016-03-12 19:46:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1676683002/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1676683002/270001

[commit-bot: I haz the power, 2016-03-12 20:56:22]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206
==========

[commit-bot: I haz the power, 2016-03-12 20:56:23]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2016-03-12 20:57:17]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}
==========

[commit-bot: I haz the power, 2016-03-12 20:57:19]

Patchset 15 (id:??) landed as
https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}

[scottmg, 2016-03-12 23:54:19]

A revert of this CL (patchset #15 id:270001) has been created in
https://codereview.chromium.org/1792603006/ by scottmg@chromium.org.

The reason for reverting is: Regressed linux sizes (iostream maybe?)

https://build.chromium.org/p/chromium/builders/Linux/builds/72899/steps/sizes....

[huangs, 2016-03-13 16:24:43]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}
==========

[huangs, 2016-03-14 15:08:49]

Recommitting.

[huangs, 2016-03-14 15:09:13]

The CQ bit was checked by huangs@chromium.org

[huangs, 2016-03-14 15:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1676683002/#ps290001 (title: "Wrap
#include <iostream> under #if COURGETTE_HISTOGRAM_TARGETS.")

[commit-bot: I haz the power, 2016-03-14 15:09:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1676683002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1676683002/290001

[commit-bot: I haz the power, 2016-03-14 16:35:50]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}
==========

[commit-bot: I haz the power, 2016-03-14 16:35:51]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2016-03-14 16:37:39]

Description was changed from

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}
==========

to

==========
[Courgette] Clean up Disassembler; fix ELF Memory leaks.

Cleaning up code surrounding Disassembler:
- Extract AddressTranslator interface to be used across subclasses.
- Use FileOffset = size_t by context.
- Detailed comments & TODOs in DisassemblerElf32ARM.
- Fix DisassemblerElf32ARM memory leaks.
- Lots of superficial stylistic changes.

Except for AddressTranslator routines and unit tests, shying away
from control flow and logic changes.

BUG=579206

Committed: https://crrev.com/58b822d441f5c982e879e536fa3c1cbac8fd339a
Cr-Commit-Position: refs/heads/master@{#380881}

Committed: https://crrev.com/dda11d069213d53aef9bf3bc018b02f3aef7177b
Cr-Commit-Position: refs/heads/master@{#380987}
==========

[commit-bot: I haz the power, 2016-03-14 16:37:40]

Patchset 16 (id:??) landed as
https://crrev.com/dda11d069213d53aef9bf3bc018b02f3aef7177b
Cr-Commit-Position: refs/heads/master@{#380987}