crazy linker: convert relocation unpacking to Android style.

crazy linker: convert relocation unpacking to Android style.

Replace relocation unpacking code with functions that understand
the packing format generated by the Android relocation packer.

Switch to using the Android relocation packer when packing during
build.

BUG=385553
Committed: https://crrev.com/e5acfbccce659364606758b687b90488e2f44be2
Cr-Commit-Position: refs/heads/master@{#333293}

[simonb (inactive), 2015-04-08 16:45:24]

simonb@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2015-04-13 10:36:10]

Looks good overall.  A few unit-test would be nice though.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp
(right):

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:457:
decoder = new Sleb128Decoder(data_start, data_size);
nit - add a DCHECK that android_relocations_[2] == 'S'

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:479:
AndroidPackedRelocationGroupFlags group_flags(decoder->pop_front());
Maybe we could just package up the different group types to an object, which
get's passed the decoder and pulls out the size, flags and any optional header
fields and creates an appropriate AndroidRelocationGroup object based on these?

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:507:
relocation.r_addend += decoder->pop_front();
If you do the suggestion above then the AndroidRelocationGroup could have a
method which process each relocation based on the appropriate flags, WDYT?

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:526:
namespace {
Could we just make these private static functions instead of static functions in
an anonymous namespace?

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:582:
return true;
Are non DT_REL / DT_RELA relocations allowed to go through this function without
being handled?

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:916:
bool ElfRelocations::ApplyRelaReloc(const ELF::Rela* rela,
Could you template this with ApplyRelReloc? Not sure if this would be a net
simplifcation (plus more templating...) but there is a fair amount of code
shared here.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:1063:
struct RelocateAndroidRelocationArgs {
nit - move down to just above RelocateAndroidRelocation

[simonb (inactive), 2015-04-28 14:39:42]

Thanks for the comments.  Updates in patch set 4, PTAL when ready.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp
(right):

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:457:
decoder = new Sleb128Decoder(data_start, data_size);
On 2015/04/13 10:36:10, rmcilroy wrote:
> nit - add a DCHECK that android_relocations_[2] == 'S'

Gone -- the android packer no longer generates unsigned LEB128 packing.
https://codereview.chromium.org/1099343004/

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:479:
AndroidPackedRelocationGroupFlags group_flags(decoder->pop_front());
On 2015/04/13 10:36:10, rmcilroy wrote:
> Maybe we could just package up the different group types to an object, which
> get's passed the decoder and pulls out the size, flags and any optional header
> fields and creates an appropriate AndroidRelocationGroup object based on
these?

I'd like to keep this code close to that of the android relocation packer, so
that it is easier to visually check for correctness.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi...

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:507:
relocation.r_addend += decoder->pop_front();
On 2015/04/13 10:36:10, rmcilroy wrote:
> If you do the suggestion above then the AndroidRelocationGroup could have a
> method which process each relocation based on the appropriate flags, WDYT?

Acknowledged.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:526:
namespace {
On 2015/04/13 10:36:10, rmcilroy wrote:
> Could we just make these private static functions instead of static functions
in
> an anonymous namespace?

We could.  Not sure what it would buy us though.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:582:
return true;
On 2015/04/13 10:36:10, rmcilroy wrote:
> Are non DT_REL / DT_RELA relocations allowed to go through this function
without
> being handled?

Relocations are either DT_REL or DT_RELA, but cannot be neither.  Enforced at
lines 331-338 above.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:916:
bool ElfRelocations::ApplyRelaReloc(const ELF::Rela* rela,
On 2015/04/13 10:36:10, rmcilroy wrote:
> Could you template this with ApplyRelReloc? Not sure if this would be a net
> simplifcation (plus more templating...) but there is a fair amount of code
> shared here.

Possible, but the result doesn't aid readability much, and forces templating
onto ApplyResolvedRel[a]Reloc() also.  This is part of the original crazy linker
code, so I've left it for now.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:1063:
struct RelocateAndroidRelocationArgs {
On 2015/04/13 10:36:10, rmcilroy wrote:
> nit - move down to just above RelocateAndroidRelocation

Done.

[rmcilroy, 2015-05-01 14:15:22]

lgtm once Arm64 issues are addressed.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp
(right):

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:479:
AndroidPackedRelocationGroupFlags group_flags(decoder->pop_front());
On 2015/04/28 14:39:41, simonb wrote:
> On 2015/04/13 10:36:10, rmcilroy wrote:
> > Maybe we could just package up the different group types to an object, which
> > get's passed the decoder and pulls out the size, flags and any optional
header
> > fields and creates an appropriate AndroidRelocationGroup object based on
> these?
> 
> I'd like to keep this code close to that of the android relocation packer, so
> that it is easier to visually check for correctness.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/androi...

Ok, SGTM.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:526:
namespace {
On 2015/04/28 14:39:41, simonb wrote:
> On 2015/04/13 10:36:10, rmcilroy wrote:
> > Could we just make these private static functions instead of static
functions
> in
> > an anonymous namespace?
> 
> We could.  Not sure what it would buy us though.

It just seems strange to have a mix of private functions and anonymous namespace
ones, but I don't really mind this is fine.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:582:
return true;
On 2015/04/28 14:39:41, simonb wrote:
> On 2015/04/13 10:36:10, rmcilroy wrote:
> > Are non DT_REL / DT_RELA relocations allowed to go through this function
> without
> > being handled?
> 
> Relocations are either DT_REL or DT_RELA, but cannot be neither.  Enforced at
> lines 331-338 above.

I see, that's fine.

https://codereview.chromium.org/1072533002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:916:
bool ElfRelocations::ApplyRelaReloc(const ELF::Rela* rela,
On 2015/04/28 14:39:41, simonb wrote:
> On 2015/04/13 10:36:10, rmcilroy wrote:
> > Could you template this with ApplyRelReloc? Not sure if this would be a net
> > simplifcation (plus more templating...) but there is a fair amount of code
> > shared here.
> 
> Possible, but the result doesn't aid readability much, and forces templating
> onto ApplyResolvedRel[a]Reloc() also.  This is part of the original crazy
linker
> code, so I've left it for now.

Acknowledged.

[simonb (inactive), 2015-06-05 16:56:05]

Arm64 issue solved by:
https://codereview.chromium.org/1155973005/

Patch set 5 adds gyp file changes to switch builds over to the Android packer. 
Should now be good-to-go.

When ready, PTAL at additions made after prior LGTM.  Thanks.

[rmcilroy, 2015-06-05 17:12:09]

On 2015/06/05 16:56:05, simonb wrote:
> Arm64 issue solved by:
> https://codereview.chromium.org/1155973005/
> 
> Patch set 5 adds gyp file changes to switch builds over to the Android packer.

> Should now be good-to-go.
> 
> When ready, PTAL at additions made after prior LGTM.  Thanks.

Looks great, LGMT!

[simonb (inactive), 2015-06-08 10:23:02]

The CQ bit was checked by simonb@chromium.org

[simonb (inactive), 2015-06-08 10:23:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1072533002/#ps100001
(title: "Rebase to master.")

[commit-bot: I haz the power, 2015-06-08 10:23:28]

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

[commit-bot: I haz the power, 2015-06-08 10:30:35]

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

[commit-bot: I haz the power, 2015-06-08 10:30:37]

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

[simonb (inactive), 2015-06-08 10:33:41]

simonb@chromium.org changed reviewers:
+ thakis@chromium.org

[simonb (inactive), 2015-06-08 10:33:41]

Adding thakis@ for build LGTM:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    build/all.gyp
    build/android/gyp/pack_arm_relocations.py
    build/android/pack_arm_relocations.gypi

[Nico, 2015-06-08 16:47:55]

build lgtm

[simonb (inactive), 2015-06-08 17:01:34]

The CQ bit was checked by simonb@chromium.org

[simonb (inactive), 2015-06-08 17:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1072533002/#ps120001
(title: "Updates to gn to match gyp.")

[commit-bot: I haz the power, 2015-06-08 17:03:02]

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

[commit-bot: I haz the power, 2015-06-08 18:11:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-08 18:12:28]

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