Support for unpacked ARM packed relocations.

Support for unpacked ARM packed relocations.

Adds support for unpacking and applying R_ARM_RELATIVE relocations
previously packed by the relocation_packer tool into the special
.android.rel.dyn section.

Strategy: If special dynamic tags are present, open the file, read the
contents of .android.rel.dyn, LEB128 decode and run length decode,
and apply each R_ARM_RELATIVE relocation produced.  If special dynamic
tags are not present, do nothing.

BUG=385553
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279393

[rmcilroy, 2014-06-18 10:56:05]

https://codereview.chromium.org/340523003/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp
(right):

https://codereview.chromium.org/340523003/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:73:
#define DT_ANDROID_ARM_REL_SIZE (DT_LOPROC + 1)
nit - just move this up to the preceding ifdef __arm__ with the comment
mentioning these are for packed relocations support?

https://codereview.chromium.org/340523003/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:283:
const char* full_path CRAZY_UNUSED,
Rather than passing the full path here, and having to re-open the file to get
the packed relocations, how about just extracting the packed relocations (if
they exist) in SharedLibrary::Load()?  This would be cleaner and more consistent
with the other relocations IMO.

[simonb1, 2014-06-18 15:41:28]

https://codereview.chromium.org/340523003/diff/40001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp
(right):

https://codereview.chromium.org/340523003/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:73:
#define DT_ANDROID_ARM_REL_SIZE (DT_LOPROC + 1)
On 2014/06/18 10:56:05, rmcilroy wrote:
> nit - just move this up to the preceding ifdef __arm__ with the comment
> mentioning these are for packed relocations support?

Moved to crazy_linker_shared_library.cpp in patch set 4.

https://codereview.chromium.org/340523003/diff/40001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.cpp:283:
const char* full_path CRAZY_UNUSED,
On 2014/06/18 10:56:05, rmcilroy wrote:
> Rather than passing the full path here, and having to re-open the file to get
> the packed relocations, how about just extracting the packed relocations (if
> they exist) in SharedLibrary::Load()?  This would be cleaner and more
consistent
> with the other relocations IMO.

Done.  It's a bit more diffuse, so greater occurrence of #ifdef __arm__.

It also doesn't remove the re-open file niggle.  The loader class is the only
thing that opens the file, and only briefly, and it doesn't parse the dynamic
section.  So we only find out that we have a .android.rel.dyn a fair while after
the loader has closed the file.  Shouldn't be a noticeable perf issue though;
loading a library is lengthy enough that one added open() ought to be
immaterial.

[rmcilroy, 2014-06-19 10:49:44]

This is looking better IMO - I prefer the separation between reading the file
and applying the relocations.  I have a suggestion on moving the packing
relocations to a separate class rather than mixing it in with the ElfRelocations
class, let me know what you think.

https://codereview.chromium.org/340523003/diff/60001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/340523003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:203:
delete [] arm_packed_relocs_;
I think you could still use your ScopedBuffer class to avoid having to call
"delete [] packed_data both here and in ReadArmPackedRelocs, and using a
release() operation to get it out of ReadArmPackedRelocs - WDYT?

https://codereview.chromium.org/340523003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:388:
return false;
I'm not sure the PackedRelocs really fits well in ElfRelocations since it is
doing the relocations in a completely different way.  How about moving all the
packed relocation processing to a separate class (ArmPackedRelocations) which
takes in the arm_packed_relocs_ and applies the relocations, e.g., here it would
be:
  if (arm_packed_relocs_) {
    ArmPackedRelocations packedRelocations;
    
    if (!packedRelocations.Init(&packed_relocations_, error)) {
       return false;
  
    if (!packedRelocations.ApplyAll(error))
     return false;	
  }

This would keep all the packed relocation processing in a single place, WDYT?

[simonb (inactive), 2014-06-23 14:51:32]

Update.  Ptal, thanks.

https://codereview.chromium.org/340523003/diff/60001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/340523003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:203:
delete [] arm_packed_relocs_;
On 2014/06/19 10:49:44, rmcilroy wrote:
> I think you could still use your ScopedBuffer class to avoid having to call
> "delete [] packed_data both here and in ReadArmPackedRelocs, and using a
> release() operation to get it out of ReadArmPackedRelocs - WDYT?

Done.

https://codereview.chromium.org/340523003/diff/60001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:388:
return false;
On 2014/06/19 10:49:43, rmcilroy wrote:
> I'm not sure the PackedRelocs really fits well in ElfRelocations since it is
> doing the relocations in a completely different way.  How about moving all the
> packed relocation processing to a separate class (ArmPackedRelocations) which
> takes in the arm_packed_relocs_ and applies the relocations, e.g., here it
would
> be:
>   if (arm_packed_relocs_) {
>     ArmPackedRelocations packedRelocations;
>     
>     if (!packedRelocations.Init(&packed_relocations_, error)) {
>        return false;
>   
>     if (!packedRelocations.ApplyAll(error))
>      return false;	
>   }
> 
> This would keep all the packed relocation processing in a single place, WDYT?

Addressed by moving ApplyArmPackedRelocs() inside of ApplyAll() and adding
RegisterArmPackedRelocs() as a way to indicate that there are packed relocations
to apply.  See if you like this better.

[rmcilroy, 2014-06-24 09:47:22]

Couple of nits, and one suggesting for using ScopedBuffer in ElfRelocations, but
otherwise lgtm.

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.h
(right):

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.h:55: bool
ApplyArmPackedRelocs(Error* error);
nit - make this private

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:193:
return NULL;
close(fd)

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:200:
return NULL;
ditto

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:205:
return packed_data;
nit - just "return buffer.Release();"

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:220:
delete [] arm_packed_relocs_;
nit - you could make arm_packed_relocs_ a ScopedBuffer too?  (and maybe
Release() it to relocations.RegisterArmPackedRelocs(), and store it as a
ScopedBuffer there too?

[simonb (inactive), 2014-06-24 11:43:56]

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.h
(right):

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_elf_relocations.h:55: bool
ApplyArmPackedRelocs(Error* error);
On 2014/06/24 09:47:22, rmcilroy wrote:
> nit - make this private

Done.

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:193:
return NULL;
On 2014/06/24 09:47:22, rmcilroy wrote:
> close(fd)

Not needed.  FileDescriptor is scoped (calls Close() in its destructor).

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:200:
return NULL;
On 2014/06/24 09:47:22, rmcilroy wrote:
> ditto

Ditto.

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:205:
return packed_data;
On 2014/06/24 09:47:22, rmcilroy wrote:
> nit - just "return buffer.Release();"

I find this more readable as it clearly documents what's being returned.  The
compiler should produce the same assembly from both variants.

https://codereview.chromium.org/340523003/diff/80001/third_party/android_craz...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:220:
delete [] arm_packed_relocs_;
On 2014/06/24 09:47:22, rmcilroy wrote:
> nit - you could make arm_packed_relocs_ a ScopedBuffer too?  (and maybe
> Release() it to relocations.RegisterArmPackedRelocs(), and store it as a
> ScopedBuffer there too?

Not really.  ScopedBuffer is designed to be stack-allocated and with scope
lifetime.  arm_packed_relocs_ here has a lifetime longer than that
(specifically, the same as the enclosing and heap-allocated SharedLibrary).

Ideally what we want here would be std::vector, but the crazy linker does not
use the STL.  It does have its own Vector, but it is a subset of std::vector and
in particular lacks any Assign().  To use it would mean extending Vector and
then adding more unit tests; not difficult, but it enlarges the scope of this
change a fair bit.

[simonb (inactive), 2014-06-24 11:49:31]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-06-24 11:49:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonb@chromium.org/340523003/100001

[commit-bot: I haz the power, 2014-06-24 13:58:22]

Change committed as 279393