|
|
Description[Courgette] Fix ELF reference sorting.
This CL addresses 2 reference sorting issues in DisassemblerElf32:
(1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to
|abs_offsets| (file offsets), but we sort |abs32_locations_|, which
is redundant. Actually we should sort |abs_offsets|.
(2) Cleanup: |rel32_relocations_| stores rel32 references sorted by
RVA, but in ParseFile() we re-sort these in offset order. Previously
Disassemble() optimizes away redundant sorts, but this makes the
code less robust. We de-optimize this a little potentially redundant
sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA
outside of ParseFile().
This CL also makes Disassemble() more uniform, to prepare for
refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not
experience issue since it assumes RVA order is same as file offset
order (this assumption has not has not caused problems so far).
BUG=660980
Review-Url: https://codereview.chromium.org/2744373004
Cr-Commit-Position: refs/heads/master@{#458650}
Committed: https://chromium.googlesource.com/chromium/src/+/c615c911eea856986f8daaab73b7f30860234009
Patch Set 1 #Patch Set 2 : Sync. #
Messages
Total messages: 21 (11 generated)
Description was changed from ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order -- so far this has not caused problem. ========== to ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order -- so far this has not caused problem. BUG=660980 ==========
Description was changed from ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order -- so far this has not caused problem. BUG=660980 ========== to ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes ParseFile() more uniform, to set up refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order -- so far this has not caused problem. BUG=660980 ==========
Description was changed from ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes ParseFile() more uniform, to set up refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order -- so far this has not caused problem. BUG=660980 ========== to ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes Disassemble() more uniform, to prepare for refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order. So far this has not caused problems. BUG=660980 ==========
huangs@chromium.org changed reviewers: + chrisha@chromium.org, wfh@chromium.org
Bug fix before more refactoring CLs. PTAL. Thanks!
lgtm
Description was changed from ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize a little this by performing more sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes Disassemble() more uniform, to prepare for refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order. So far this has not caused problems. BUG=660980 ========== to ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize this a little potentially redundant sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes Disassemble() more uniform, to prepare for refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order (this assumption has not has not caused problems so far). BUG=660980 ==========
Ping wfh@, PTAL. Thanks@
I looked at this after chris did, and certainly lgtm but did not comment since a) you are an owner of this code and are therefore trusted to make changes autonomously b) you didn't ask specifically which parts you wanted me to look at and which parts you wanted chrisha to look at and c) chrisha had already lgtmed.
when adding two or more reviewers please specify the expectations of each reviewer. thanks.
Ah okay. Thanks!
The CQ bit was checked by huangs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org, wfh@chromium.org Link to the patchset: https://codereview.chromium.org/2744373004/#ps20001 (title: "Sync.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by huangs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490157677335460, "parent_rev": "ba7b76c799d7ed719aba76ced546935ddf3da9fd", "commit_rev": "c615c911eea856986f8daaab73b7f30860234009"}
Message was sent while issue was closed.
Description was changed from ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize this a little potentially redundant sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes Disassemble() more uniform, to prepare for refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order (this assumption has not has not caused problems so far). BUG=660980 ========== to ========== [Courgette] Fix ELF reference sorting. This CL addresses 2 reference sorting issues in DisassemblerElf32: (1) Bug fix: In ParseFile(), |abs32_locations_| (RVAs) is translated to |abs_offsets| (file offsets), but we sort |abs32_locations_|, which is redundant. Actually we should sort |abs_offsets|. (2) Cleanup: |rel32_relocations_| stores rel32 references sorted by RVA, but in ParseFile() we re-sort these in offset order. Previously Disassemble() optimizes away redundant sorts, but this makes the code less robust. We de-optimize this a little potentially redundant sort-by-RVA, to assert that |rel32_locations_| is sorted by RVA outside of ParseFile(). This CL also makes Disassemble() more uniform, to prepare for refactoring in a follow-up. Meanwhile, DisassemblerWin32 does not experience issue since it assumes RVA order is same as file offset order (this assumption has not has not caused problems so far). BUG=660980 Review-Url: https://codereview.chromium.org/2744373004 Cr-Commit-Position: refs/heads/master@{#458650} Committed: https://chromium.googlesource.com/chromium/src/+/c615c911eea856986f8daaab73b7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c615c911eea856986f8daaab73b7... |