[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32 parsing to .text.

[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32 parsing to .text.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we restrict rel32 parsing to ".text" section
(heuristic). Also updating ELF test results.

BUG=601948
Committed: https://crrev.com/3da0dd9308f66ae1122bbaefda2c99a4f90372d4
Cr-Commit-Position: refs/heads/master@{#390506}

[huangs, 2016-04-27 18:00:00]

Description was changed from

==========
[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32
parsing to .text section.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we implement a heuristic of only extracting
rel32 from ".text" section. Also updated ELF test results for.

BUG=601948
==========

to

==========
[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32
parsing to .text.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we restrict rel32 parsing to ".text" section
(heuristic). Also updating ELF test results.

BUG=601948
==========

[huangs, 2016-04-27 18:00:00]

huangs@chromium.org changed reviewers:
+ andrewhayden@chromium.org, wfh@chromium.org

[huangs, 2016-04-27 18:02:53]

Please note that this affects ELF results.  For production (Win32) this affects
nacl_irt_x86_32.nexe: Fixing bug (1) blots patch because now it can finally
properly parse bogus sections! But what (1) giveth (2) taketh away -- so should
not have net change.

What's more interesting is that this may improve Courgette for ELF-ARM.

PTAL.

[Andrew Hayden (chromium.org), 2016-04-28 10:13:55]

I am not qualified to review this from knowledge of the ELF standard, but
logically this ell-gee-tee-emm. What's up with the small changes to the absolute
values in the unit test code?

[huangs, 2016-04-28 14:31:16]

For disassembler_elf_32_x86_unittest.cc: the number of (ELF) rel32 addresses
found reduced because bogus ones disappeared. Abs32 is unchanged because they're
from parsed from relocation section.

For encode_decode_unittest.cc: the above reduction shrunk Courgette assembly
file sizes.

[Andrew Hayden (chromium.org), 2016-04-28 15:07:45]

On 2016/04/28 14:31:16, huangs wrote:
> For disassembler_elf_32_x86_unittest.cc: the number of (ELF) rel32 addresses
> found reduced because bogus ones disappeared. Abs32 is unchanged because
they're
> from parsed from relocation section.
> 
> For encode_decode_unittest.cc: the above reduction shrunk Courgette assembly
> file sizes.

Makes sense. Thank you for the explanation.

[Will Harris, 2016-04-28 19:45:49]

https://codereview.chromium.org/1928683002/diff/1/courgette/disassembler_elf_...
File courgette/disassembler_elf_32.cc (right):

https://codereview.chromium.org/1928683002/diff/1/courgette/disassembler_elf_...
courgette/disassembler_elf_32.cc:552: VLOG(1) << "Warning: Found no rel32
addresses. Missing .text section?";
what does this warning mean if it's seen? is there any action that a user of
courgette can take?

[huangs, 2016-04-28 19:53:45]

https://codereview.chromium.org/1928683002/diff/1/courgette/disassembler_elf_...
File courgette/disassembler_elf_32.cc (right):

https://codereview.chromium.org/1928683002/diff/1/courgette/disassembler_elf_...
courgette/disassembler_elf_32.cc:552: VLOG(1) << "Warning: Found no rel32
addresses. Missing .text section?";
The warning says "your ELF file is weird because it's missing .text section". 
This should be a rare occurrence, but it's worth noting since otherwise
Courgette will silently produce suboptimal patches.  

On seeing this message, the user can reflect on why they going against
convention.  If there's a good reason, then we should revisit the heuristic.

[Will Harris, 2016-04-28 20:04:02]

lgtm

https://codereview.chromium.org/1928683002/diff/1/courgette/disassembler_elf_...
File courgette/disassembler_elf_32.cc (right):

https://codereview.chromium.org/1928683002/diff/1/courgette/disassembler_elf_...
courgette/disassembler_elf_32.cc:552: VLOG(1) << "Warning: Found no rel32
addresses. Missing .text section?";
On 2016/04/28 19:53:45, huangs wrote:
> The warning says "your ELF file is weird because it's missing .text section". 
> This should be a rare occurrence, but it's worth noting since otherwise
> Courgette will silently produce suboptimal patches.  
> 
> On seeing this message, the user can reflect on why they going against
> convention.  If there's a good reason, then we should revisit the heuristic.

Acknowledged.

[huangs, 2016-04-28 20:05:13]

Thanks! Committing soon.

[huangs, 2016-04-28 20:06:27]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2016-04-28 20:06:55]

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

[commit-bot: I haz the power, 2016-04-28 22:14:54]

Description was changed from

==========
[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32
parsing to .text.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we restrict rel32 parsing to ".text" section
(heuristic). Also updating ELF test results.

BUG=601948
==========

to

==========
[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32
parsing to .text.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we restrict rel32 parsing to ".text" section
(heuristic). Also updating ELF test results.

BUG=601948
==========

[commit-bot: I haz the power, 2016-04-28 22:14:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-30 17:22:01]

Patchset 1 (id:??) landed as
https://crrev.com/3da0dd9308f66ae1122bbaefda2c99a4f90372d4
Cr-Commit-Position: refs/heads/master@{#390506}

[commit-bot: I haz the power, 2016-04-30 17:57:57]

Description was changed from

==========
[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32
parsing to .text.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we restrict rel32 parsing to ".text" section
(heuristic). Also updating ELF test results.

BUG=601948
==========

to

==========
[Courgette] ELF: Fix abs32 / rel32 ordering in ParseFile() and restrict rel32
parsing to .text.

This CL fixes 2 problems in Courgette ELF parsing:
1. ParseFile() scans through file bytes in file offset order while
   visiting abs32 and rel32 locations in lockstep. However, these
   locations were previously not sorted by file offset, resulting in
   some abs32 and rel32 locations being ignored.
2. ParseRel32RelocsFromSections() is too permissive, and extracts bogus
   rel32 addresses from non-code.

To solve (1) we sort abs32 and rel32 addresses by file offset in
ParseFile(). To solve (2) we restrict rel32 parsing to ".text" section
(heuristic). Also updating ELF test results.

BUG=601948

Committed: https://crrev.com/3da0dd9308f66ae1122bbaefda2c99a4f90372d4
Cr-Commit-Position: refs/heads/master@{#390506}
==========