Add a host tool to pack R_ARM_RELATIVE relocations in libchrome.<ver>.so.

Add a host tool to pack R_ARM_RELATIVE relocations in libchrome.<ver>.so.

R_ARM_RELATIVE relocations are the bulk of dynamic relocations (the .rel.dyn
section) in libchrome.<version>.so.  The ELF standard representation of them
is wasteful.

Packing uses run length encoding to store them more efficiently.  Packed
relocations are placed in a new .android.rel.dyn section.  Packing reduces
the footprint of libchrome.<version>.so in the filesystem, in APK downloads,
and in memory when loaded on the device.

A packed libchrome.<version>.so is designed so that it can be loaded directly
on Android, but requires the explicit support of a crazy linker that has been
extended to understand packed relocations.

A packed libchrome.<version>.so cannot currently be used with the standard
Android runtime linker.

See README.TXT and src/*.h for design and implementation notes.

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

[rmcilroy, 2014-06-02 15:16:34]

Overall looks pretty good.  Lots of comments, but most are small (and it is a
massive CL).

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/LICENSE
File tools/relocation_packer/LICENSE (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/LICE...
tools/relocation_packer/LICENSE:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
nit - 2014

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/LICE...
tools/relocation_packer/LICENSE:31: * All rights reserved.
Do we need the Android licence too? Seems strange to have two licences in the
LICENCE file.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
File tools/relocation_packer/README.TXT (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:9: relocations are placed in a new
.android.rel.dyn section.  Packing reduces
Would it be safer to put this in a section called .crazy.rel.dyn or something
(assuming they are going to be unpacked by the crazy linker), just in case
android decides to add their own section at some point with the name
.android.rel.dyn.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:11: and in memory when loaded on the device.
"in memory when loaded on the device" - is this true?  I thought it was unpacked
during loading so would take the same amount of RAM?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:15: extended to understand packed
relocations.
nit - mention there is only Arm support currently

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:29: Requires the explicit support of an
extended crazy linker.  Adds the
nit - Unpacking requires the...

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:72: pack-relocations
libchrome.<version>.so.packed
Could the tool do the adding and removing of sections itself, or is that too
much work?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:77: pack-relocations -u unpackable
From the gyp file, looks like the executable is "relocation_packer" rather than
pack-relocations?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:91: .android.rel.dyn data.
What if there are not two free slots?  Can we force there to be enough free
slots?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
File tools/relocation_packer/relocation_packer.gyp (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:8: 'target_name':
'pack_relocations',
nit - lib_relocation_packer ?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:15: 'src',
Is this needed?  Doesn't seem to be required in the unittest target.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:22:
'src/relocation_packer_rle.cc',
optional nit - maybe avoid the "relocation_packer_" prefix on every filename
since it's already in a directory called "relocation_packer".  Maybe also extend
"rle" to "run_length_encoder.cc".

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:41: '-lelf',
Do you need this?  I though depending on the libelf target would do this for
you.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_debug.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.cc:14: Logger*
Logger::GetInstance() {
nit - can you mention somewhere that this is explicitly not thread-safe

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_debug.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.h:7: // Log messages to
stdout.  LOG() prints normal user messages, VLOG)
VLOG()

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.h:19: #undef NDEBUG
I don't think you should be undef'ing NDEBUG in a header.  If you are doing this
in order to get assert defined to do something even if NDEBUG is set, then you
could do what /base does (cause a DebuggerBreak if Check fails), or only undef
NDEBUG while importing assert.h.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.h:63: Logger() {
::memset(this, 0, sizeof(*this)); }
no need for "::" global namespace prefix (throughout CL).  In this particulary
instance I think it would be clearer just to explicitly set
"is_verbose_(false)".

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:33: // buffer.  Relies
on program exit to free allocated data.
I'm not sure I like relying on program exit to free the allocated data.  Can't
we free data->d_buf, or is that potentially allocated a different way?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:46: void
ResizeSection(Elf* elf,
This function is pretty massive and could benefit from splitting into a couple
of sub-functions.  Maybe: AdjustsElfHeadersForHole, AdjustSectionHeadersForHole,
AdjustDynamicSectionForHole, AdjustDynSymSectionForHole,
AdjustRelPltSectionForHole and AdjustSymTabSectionForHole?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:65: Elf_Data* data =
elf_getdata(section, NULL);
nit - section_data

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:66: CHECK(data &&
elf_getdata(section, data) == NULL);
From the man page it looks like the "&& elf_getdata(dynamic_section, data) ==
NULL" is checking there are no further data buffers - could you add a comment to
that effect since it's note terribly clear just reading the code.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:138: Elf_Scn*
symtab_section = NULL;
Maybe you could just save these sections away as fields (initialized in Load(),
and updated as necessary here) and have this be a private method on ElfFile?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:185: CHECK(section !=
dynamic_section);
nit - do this check at the top of the function

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:186: data =
elf_getdata(dynamic_section, NULL);
nit - dynamic_section_data

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:187: CHECK(data &&
elf_getdata(dynamic_section, data) == NULL);
ditto

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:246: void*
section_data = &dynamics[0];
nit - new_dynamic_section_data

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:252: data =
elf_getdata(dynsym_section, NULL);
nit - dynsym_section_data (and similar for others below)

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:375: case 0: type =
"NULL"; break;
nit - you use PT_DYNANIC below, could you use PT_* constant's here instead of
the integer literals please?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:390: VLOG("  p_memsz =
%u\n", program_header->p_memsz);
Please extract the debugging code (from line 373 to line 390) to a separate
function (e.g, VerboseLogProgramHeader())

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:412: bool is_debug =
false;
nit - has_debug_section

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:445: }
nit - extract verbose debugging code (lines 418-421 and 439-445) as something
like VerboseLogSection()

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:473: namespace {
nit - I would prefer a single anonymous namespace at the top of the file with
all the helper functions.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:477: // found cannot
be the last one in the vector.
As I commented in the Readme, this seems fragile, is there any way we can
guarantee there will be enough free slots?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:480: for (size_t i =
0; i < dynamics->size() - 1; ++i) {
nit - add a comment:
// The vector should always end with a free (unused) element, and so 
// we cannot replace the final element.  Therefore, only loop until 
// the penultimate element.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:497: void
ApplyRelocations(Elf* elf,
Should this maybe be called AdjustRelocationTargets?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:524: if
(is_encompassed) {
nit - just inline is_encompassed caluclation into the if statement

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:670: const void*
section_data = &other_relocations[0];
nit - new_section_data (or new_data)

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:678: CHECK(data &&
elf_getdata(android_rel_dyn_section_, data) == NULL);
You do this CHECK a lot, maybe have a wrapper function which does the
elf_getdata and the CHECK

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:681:
CHECK(data->d_size > 0 && data->d_buf);
nit - do this check in ResizeSection?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:702:
RewriteSectionData(data, dynamics_data, dynamics_bytes);
nit - lines 686-702 seem like they could be a separate function.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:716: for (size_t i =
0; i < dynamics->size() - 1; ++i) {
If you can only remove items from the end of the vector, shouldn't you start the
loop from the end, and assert that the entry is DT_NULL or the expected tag to
ensure you don't early-terminate the vector?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:66: class ElfFile {
How about "class ArmElfFile : public ElfFile" (with the platform independent
parts in the ElfFile superclass)? This would make porting easier.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:74: static const
Elf32_Sword DT_ANDROID_ARM_REL_SIZE = DT_LOPROC + 1;
Do these (and kStubIdentifier above) need to be in the header?  They only seem
to be used in the relocation_packer_elf_file.cc file.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:82: bool Load(int fd);
nit - mention what is returned on success or failure (for all functions here).

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:99: void Flush();
Is there a reason to have Load and Flush as separate public functions? Why not
just have the constructor take the fd, and have
PackRelocations/UnpackRelocations explicitly Open / Flush the file themselves?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:35: return
*relocs_so && *packed_relocs_so;
Couldn't you just include these both as binary files in a data directory rather
than have them expressed as a header file?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:39:
fclose(temporary);
delete the temp files?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:72: #if
DEBUG_TEST_DATA
This is never defined - can we just remove this test now?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:103:
ASSERT_EQ(0, lseek(fileno(relocs_so), 0, SEEK_SET));
How about doing the lseek at the end of FileContentsEqual so that
FileContentsEqual is idempotent? Also, any reason it's not fseek?

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file_unittest_relocs.cc
(right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest_relocs.cc:27:
struct Foo {
How about putting this in a data directory (along with the so files) and call it
elf_file_unittest_sample.cc

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_leb128.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_leb128.h:33: void
GetEncoding(std::vector<uint8_t>* encoding) { *encoding = encoding_; }
nit - just return the pointer rather than having an out arg.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_leb128.h:53: void
Dequeue(std::vector<uint32_t>* values);
DequeueAll (and maybe EnqueueAll above) to make it clear this retrieve's all
remaining values

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_rle.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_rle.cc:68: size_t start,
nit - start_idx (I got confused between the "initial" address and "start" index)

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_rle.cc:76: for (size_t i = start;
i < start + (pairs_count << 1); i += 2) {
nit - might be clearer if you pass (pairs_count << 1) as the argument in
::Decode and avoid doing the calculation here.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_rle.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_rle.h:49: //   pairs=15 start=..
count=6,delta=4   count=16,delta=8
It seems like this should be applicable to all Elf32_Rel objects, is this right?
 If so, could you make the comments for this class less Arm specific (they can
still give Arm examples, just make it clear it works on all Elf32_Rel objects). 
This can be done in a seperate CL (e.g., for Arm64) if you prefer.

[simonb (inactive), 2014-06-04 16:40:35]

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/LICENSE
File tools/relocation_packer/LICENSE (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/LICE...
tools/relocation_packer/LICENSE:1: // Copyright (c) 2013 The Chromium Authors.
All rights reserved.
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - 2014

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/LICE...
tools/relocation_packer/LICENSE:31: * All rights reserved.
On 2014/06/02 15:16:35, rmcilroy wrote:
> Do we need the Android licence too? Seems strange to have two licences in the
> LICENCE file.

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
File tools/relocation_packer/README.TXT (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:9: relocations are placed in a new
.android.rel.dyn section.  Packing reduces
On 2014/06/02 15:16:35, rmcilroy wrote:
> Would it be safer to put this in a section called .crazy.rel.dyn or something
> (assuming they are going to be unpacked by the crazy linker), just in case
> android decides to add their own section at some point with the name
> .android.rel.dyn.

The idea (or at least the hope) is that this will become a part of the standard
android runtime linker, at which point it is no longer 'crazy'.

I don't think a collision with a future real android sections seems likely, but
if we want to rename this then a more descriptive nam could be
'.android.packed.rel.dyn' or similar.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:11: and in memory when loaded on the device.
On 2014/06/02 15:16:35, rmcilroy wrote:
> "in memory when loaded on the device" - is this true?  I thought it was
unpacked
> during loading so would take the same amount of RAM?

This should be true.  The current relocations are part of mapped memory.  Packed
relocations can be read (iterated) out of the .so, processed as each is
generated, and then discarded; no longer needed.  Because we've removed them
from the mapped image this means it gets smaller, and so takes less device ram.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:15: extended to understand packed
relocations.
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - mention there is only Arm support currently

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:29: Requires the explicit support of an
extended crazy linker.  Adds the
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - Unpacking requires the...

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:72: pack-relocations
libchrome.<version>.so.packed
On 2014/06/02 15:16:35, rmcilroy wrote:
> Could the tool do the adding and removing of sections itself, or is that too
> much work?

It's icky work that it is better to avoid.  Adding the section means adding a
new string to name the section, which means increasing the size of .shstrtab
which means shuffling a lot of the contents of the file around (again).  Objcopy
can do this part for us, so it seems useful to let it.  Yes, it means that
usability of the tool is a touch more ragged than it might be, but since it
isn't intended to be run by people, only from gunk at the bottom of gyp/ninja
actions, I think it's okay to do it this way.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:77: pack-relocations -u unpackable
On 2014/06/02 15:16:35, rmcilroy wrote:
> From the gyp file, looks like the executable is "relocation_packer" rather
than
> pack-relocations? 

Done.  (Resulted from a last-minute change to the gyp file.)

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:91: .android.rel.dyn data.
On 2014/06/02 15:16:35, rmcilroy wrote:
> What if there are not two free slots?  Can we force there to be enough free
> slots?

We can, but... growing the table means resizing its section, and that leads to a
catch-22 where the thing that resizes the section wants change the contents of
this section.  Put differently, the code that writes to these unused slots is
called from stuff already in the middle of resizing a different section.

In practice we can always arrange for the linker to reserve us some slots.  From
gold(1):

  --spare-dynamic-tags COUNT
      Dynamic tag slots to reserve (default 5)

http://permalink.gmane.org/gmane.comp.gnu.binutils/46762

I have updated the code to reference this linker flag.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
File tools/relocation_packer/relocation_packer.gyp (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:8: 'target_name':
'pack_relocations',
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - lib_relocation_packer ?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:15: 'src',
On 2014/06/02 15:16:35, rmcilroy wrote:
> Is this needed?  Doesn't seem to be required in the unittest target.

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:22:
'src/relocation_packer_rle.cc',
On 2014/06/02 15:16:35, rmcilroy wrote:
> optional nit - maybe avoid the "relocation_packer_" prefix on every filename
> since it's already in a directory called "relocation_packer".  Maybe also
extend
> "rle" to "run_length_encoder.cc".

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/relo...
tools/relocation_packer/relocation_packer.gyp:41: '-lelf',
On 2014/06/02 15:16:35, rmcilroy wrote:
> Do you need this?  I though depending on the libelf target would do this for
> you.

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_debug.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.cc:14: Logger*
Logger::GetInstance() {
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - can you mention somewhere that this is explicitly not thread-safe

Done (in debug.h).

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_debug.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.h:7: // Log messages to
stdout.  LOG() prints normal user messages, VLOG)
On 2014/06/02 15:16:35, rmcilroy wrote:
> VLOG()

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.h:19: #undef NDEBUG
On 2014/06/02 15:16:35, rmcilroy wrote:
> I don't think you should be undef'ing NDEBUG in a header.  If you are doing
this
> in order to get assert defined to do something even if NDEBUG is set, then you
> could do what /base does (cause a DebuggerBreak if Check fails), or only undef
> NDEBUG while importing assert.h.

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_debug.h:63: Logger() {
::memset(this, 0, sizeof(*this)); }
On 2014/06/02 15:16:35, rmcilroy wrote:
> no need for "::" global namespace prefix (throughout CL).  In this particulary
> instance I think it would be clearer just to explicitly set
> "is_verbose_(false)".

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:33: // buffer.  Relies
on program exit to free allocated data.
On 2014/06/02 15:16:35, rmcilroy wrote:
> I'm not sure I like relying on program exit to free the allocated data.  Can't
> we free data->d_buf, or is that potentially allocated a different way?

data->d_buf is allocated by libelf (unless we've been here already), and libelf
will free it on exit.  If we want to gc our allocations we'll need to keep a
track of arrays allocated and written into data->d_buf fields, and free them in
the destructor (called from main only on program exit!) and/or when we find one
while overwriting.  Possible, but semantically the same as relying on program
exit to clean up.  Let me know if you want to add it, though.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:46: void
ResizeSection(Elf* elf,
On 2014/06/02 15:16:35, rmcilroy wrote:
> This function is pretty massive and could benefit from splitting into a couple
> of sub-functions.  Maybe: AdjustsElfHeadersForHole,
AdjustSectionHeadersForHole,
> AdjustDynamicSectionForHole, AdjustDynSymSectionForHole,
> AdjustRelPltSectionForHole and AdjustSymTabSectionForHole?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:66: CHECK(data &&
elf_getdata(section, data) == NULL);
On 2014/06/02 15:16:35, rmcilroy wrote:
> From the man page it looks like the "&& elf_getdata(dynamic_section, data) ==
> NULL" is checking there are no further data buffers - could you add a comment
to
> that effect since it's note terribly clear just reading the code. 

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:138: Elf_Scn*
symtab_section = NULL;
On 2014/06/02 15:16:35, rmcilroy wrote:
> Maybe you could just save these sections away as fields (initialized in
Load(),
> and updated as necessary here) and have this be a private method on ElfFile?

Slightly fiddly.  See if you like the refactored code better.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:185: CHECK(section !=
dynamic_section);
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - do this check at the top of the function

Can't; dynamic_section is assigned in the loop above and just exited.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:375: case 0: type =
"NULL"; break;
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - you use PT_DYNANIC below, could you use PT_* constant's here instead of
> the integer literals please?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:390: VLOG("  p_memsz =
%u\n", program_header->p_memsz);
On 2014/06/02 15:16:35, rmcilroy wrote:
> Please extract the debugging code (from line 373 to line 390) to a separate
> function (e.g, VerboseLogProgramHeader())

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:412: bool is_debug =
false;
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - has_debug_section 

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:445: }
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - extract verbose debugging code (lines 418-421 and 439-445) as something
> like VerboseLogSection()

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:473: namespace {
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - I would prefer a single anonymous namespace at the top of the file with
> all the helper functions.

I prefer helper functions to be defined close to the items they help (preferably
nested, but not an option in C++).

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:477: // found cannot
be the last one in the vector.
On 2014/06/02 15:16:35, rmcilroy wrote:
> As I commented in the Readme, this seems fragile, is there any way we can
> guarantee there will be enough free slots?

gold --spare-dynamic-tags.  Code updated.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:480: for (size_t i =
0; i < dynamics->size() - 1; ++i) {
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - add a comment:
> // The vector should always end with a free (unused) element, and so 
> // we cannot replace the final element.  Therefore, only loop until 
> // the penultimate element.

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:497: void
ApplyRelocations(Elf* elf,
On 2014/06/02 15:16:35, rmcilroy wrote:
> Should this maybe be called AdjustRelocationTargets?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:524: if
(is_encompassed) {
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - just inline is_encompassed caluclation into the if statement

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:678: CHECK(data &&
elf_getdata(android_rel_dyn_section_, data) == NULL);
On 2014/06/02 15:16:35, rmcilroy wrote:
> You do this CHECK a lot, maybe have a wrapper function which does the
> elf_getdata and the CHECK

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:681:
CHECK(data->d_size > 0 && data->d_buf);
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - do this check in ResizeSection?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:702:
RewriteSectionData(data, dynamics_data, dynamics_bytes);
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - lines 686-702 seem like they could be a separate function.

Left as is for now.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:716: for (size_t i =
0; i < dynamics->size() - 1; ++i) {
On 2014/06/02 15:16:35, rmcilroy wrote:
> If you can only remove items from the end of the vector, shouldn't you start
the
> loop from the end, and assert that the entry is DT_NULL or the expected tag to
> ensure you don't early-terminate the vector?

Rewritten to remove the matching entry and shuffle following elements up.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:66: class ElfFile {
On 2014/06/02 15:16:35, rmcilroy wrote:
> How about "class ArmElfFile : public ElfFile" (with the platform independent
> parts in the ElfFile superclass)? This would make porting easier.

There's no current plan to port to other architectures; there is a possibility
of extending this in future to arm64.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:74: static const
Elf32_Sword DT_ANDROID_ARM_REL_SIZE = DT_LOPROC + 1;
On 2014/06/02 15:16:35, rmcilroy wrote:
> Do these (and kStubIdentifier above) need to be in the header?  They only seem
> to be used in the relocation_packer_elf_file.cc file.

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:82: bool Load(int fd);
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - mention what is returned on success or failure (for all functions here).

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:99: void Flush();
On 2014/06/02 15:16:35, rmcilroy wrote:
> Is there a reason to have Load and Flush as separate public functions? Why not
> just have the constructor take the fd, and have
> PackRelocations/UnpackRelocations explicitly Open / Flush the file themselves?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:35: return
*relocs_so && *packed_relocs_so;
On 2014/06/02 15:16:35, rmcilroy wrote:
> Couldn't you just include these both as binary files in a data directory
rather
> than have them expressed as a header file?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:39:
fclose(temporary);
On 2014/06/02 15:16:35, rmcilroy wrote:
> delete the temp files?

No need.  From tmpfile(1): "The file will be automatically deleted when it is
closed or the program terminates."  These files have no visible directory
entries.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:72: #if
DEBUG_TEST_DATA
On 2014/06/02 15:16:35, rmcilroy wrote:
> This is never defined - can we just remove this test now?

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file_unittest.cc:103:
ASSERT_EQ(0, lseek(fileno(relocs_so), 0, SEEK_SET));
On 2014/06/02 15:16:35, rmcilroy wrote:
> How about doing the lseek at the end of FileContentsEqual so that
> FileContentsEqual is idempotent? Also, any reason it's not fseek?

Can't call ASSERT_EQ from a namespace-local function (because it returns a
value).

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_leb128.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_leb128.h:33: void
GetEncoding(std::vector<uint8_t>* encoding) { *encoding = encoding_; }
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - just return the pointer rather than having an out arg.

More awkward for the current client code.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_leb128.h:53: void
Dequeue(std::vector<uint32_t>* values);
On 2014/06/02 15:16:35, rmcilroy wrote:
> DequeueAll (and maybe EnqueueAll above) to make it clear this retrieve's all
> remaining values

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_rle.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_rle.cc:68: size_t start,
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - start_idx (I got confused between the "initial" address and "start"
index)

Switched to receiving start_index and end_index, direct loop bounds.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_rle.cc:76: for (size_t i = start;
i < start + (pairs_count << 1); i += 2) {
On 2014/06/02 15:16:35, rmcilroy wrote:
> nit - might be clearer if you pass (pairs_count << 1) as the argument in
> ::Decode and avoid doing the calculation here.

See above.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_rle.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_rle.h:49: //   pairs=15 start=..
count=6,delta=4   count=16,delta=8
On 2014/06/02 15:16:35, rmcilroy wrote:
> It seems like this should be applicable to all Elf32_Rel objects, is this
right?
>  If so, could you make the comments for this class less Arm specific (they can
> still give Arm examples, just make it clear it works on all Elf32_Rel
objects). 
> This can be done in a seperate CL (e.g., for Arm64) if you prefer.

Noted.

[rmcilroy, 2014-06-07 11:49:07]

Looks really good. Couple of last comments, but lgtm once these are addressed.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
File tools/relocation_packer/README.TXT (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:9: relocations are placed in a new
.android.rel.dyn section.  Packing reduces
On 2014/06/04 16:40:35, simonb wrote:
> On 2014/06/02 15:16:35, rmcilroy wrote:
> > Would it be safer to put this in a section called .crazy.rel.dyn or
something
> > (assuming they are going to be unpacked by the crazy linker), just in case
> > android decides to add their own section at some point with the name
> > .android.rel.dyn.
> 
> The idea (or at least the hope) is that this will become a part of the
standard
> android runtime linker, at which point it is no longer 'crazy'.
> 
> I don't think a collision with a future real android sections seems likely,
but
> if we want to rename this then a more descriptive nam could be
> '.android.packed.rel.dyn' or similar.
> 

Should be fine as is.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:77: pack-relocations -u unpackable
On 2014/06/04 16:40:35, simonb wrote:
> On 2014/06/02 15:16:35, rmcilroy wrote:
> > From the gyp file, looks like the executable is "relocation_packer" rather
> than
> > pack-relocations? 
> 
> Done.  (Resulted from a last-minute change to the gyp file.)

relocation_packer (not relocations_packer) ;)

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:138: Elf_Scn*
symtab_section = NULL;
On 2014/06/04 16:40:35, simonb wrote:
> On 2014/06/02 15:16:35, rmcilroy wrote:
> > Maybe you could just save these sections away as fields (initialized in
> Load(),
> > and updated as necessary here) and have this be a private method on ElfFile?
> 
> Slightly fiddly.  See if you like the refactored code better.

This looks better, thanks.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:473: namespace {
On 2014/06/04 16:40:35, simonb wrote:
> On 2014/06/02 15:16:35, rmcilroy wrote:
> > nit - I would prefer a single anonymous namespace at the top of the file
with
> > all the helper functions.
> 
> I prefer helper functions to be defined close to the items they help
(preferably
> nested, but not an option in C++).

In that case, please move ResizeSection (and all associated helper functions)
down here since they are only used by PackRelocations and UnpackRelocations.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.h (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.h:66: class ElfFile {
On 2014/06/04 16:40:35, simonb wrote:
> On 2014/06/02 15:16:35, rmcilroy wrote:
> > How about "class ArmElfFile : public ElfFile" (with the platform independent
> > parts in the ElfFile superclass)? This would make porting easier.
> 
> There's no current plan to port to other architectures; there is a possibility
> of extending this in future to arm64.

Arm64 is a different architecture.  I'm fine with this being left until / if
this is ported to Arm64 though.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/README.TXT (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/README.TXT:15: extended to understand packed
relocations.  The crazy linker currently only
/s/The crazy linker currently only.../Currently packed relocations are only
supported on Arm.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/relocation_packer.gyp (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/relocation_packer.gyp:60:
'src/elf_file_unittest_relocs_packed.so',
nit - put these files in a "test_data" directory (not src)

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:398: if (symtab_section) {
could you pull this section out into an AdjustSymTabSectionForHole function too?

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:578: LOG("Missing .android.rel.dyn
section (not split/packed?)\n");
ERROR: Missing required .android.rel.dyn section.  See README.TXT for usage
instructions.  (also include "ERROR:" in messages above.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:757:
packer.PackRelativeRelocations(relative_relocations, &packed);
How about making PackRelativeRelocations return a bool if it was successful or
not, and then do the Log/return false if it failed here rather than indirectly
testing whether packed is empty?

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file_unittest.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file_unittest.cc:18: #define str(s) #s
All uppercase for macro names.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file_unittest_relocs.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file_unittest_relocs.cc:28: 
Move this file out of src and into test_data alongside the .so's please.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/leb128.h (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/leb128.h:27: void Enqueue(uint32_t value);
Newline

[simonb (inactive), 2014-06-09 14:39:19]

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
File tools/relocation_packer/README.TXT (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:9: relocations are placed in a new
.android.rel.dyn section.  Packing reduces
On 2014/06/07 11:49:06, rmcilroy wrote:
> On 2014/06/04 16:40:35, simonb wrote:
> > On 2014/06/02 15:16:35, rmcilroy wrote:
> > > Would it be safer to put this in a section called .crazy.rel.dyn or
> something
> > > (assuming they are going to be unpacked by the crazy linker), just in case
> > > android decides to add their own section at some point with the name
> > > .android.rel.dyn.
> > 
> > The idea (or at least the hope) is that this will become a part of the
> standard
> > android runtime linker, at which point it is no longer 'crazy'.
> > 
> > I don't think a collision with a future real android sections seems likely,
> but
> > if we want to rename this then a more descriptive nam could be
> > '.android.packed.rel.dyn' or similar.
> > 
> 
> Should be fine as is.

Okay.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/READ...
tools/relocation_packer/README.TXT:77: pack-relocations -u unpackable
On 2014/06/07 11:49:06, rmcilroy wrote:
> On 2014/06/04 16:40:35, simonb wrote:
> > On 2014/06/02 15:16:35, rmcilroy wrote:
> > > From the gyp file, looks like the executable is "relocation_packer" rather
> > than
> > > pack-relocations? 
> > 
> > Done.  (Resulted from a last-minute change to the gyp file.)
> 
> relocation_packer (not relocations_packer) ;)

Done.

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
File tools/relocation_packer/src/relocation_packer_elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/relocation_packer_elf_file.cc:473: namespace {
On 2014/06/07 11:49:06, rmcilroy wrote:
> On 2014/06/04 16:40:35, simonb wrote:
> > On 2014/06/02 15:16:35, rmcilroy wrote:
> > > nit - I would prefer a single anonymous namespace at the top of the file
> with
> > > all the helper functions.
> > 
> > I prefer helper functions to be defined close to the items they help
> (preferably
> > nested, but not an option in C++).
> 
> In that case, please move ResizeSection (and all associated helper functions)
> down here since they are only used by PackRelocations and UnpackRelocations.

Good point.  Done.  Also moved AddDynamicEntry and RemoveDynamicEntry so that
they are adjacent.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/README.TXT (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/README.TXT:15: extended to understand packed
relocations.  The crazy linker currently only
On 2014/06/07 11:49:07, rmcilroy wrote:
> /s/The crazy linker currently only.../Currently packed relocations are only
> supported on Arm.

Done.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/relocation_packer.gyp (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/relocation_packer.gyp:60:
'src/elf_file_unittest_relocs_packed.so',
On 2014/06/07 11:49:07, rmcilroy wrote:
> nit - put these files in a "test_data" directory (not src)

Done.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:398: if (symtab_section) {
On 2014/06/07 11:49:07, rmcilroy wrote:
> could you pull this section out into an AdjustSymTabSectionForHole function
too?

Doh.  Already pulled out, just forgot to call it in place of the code factored
out.  Fixed.  Thanks for catching.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:578: LOG("Missing .android.rel.dyn
section (not split/packed?)\n");
On 2014/06/07 11:49:07, rmcilroy wrote:
> ERROR: Missing required .android.rel.dyn section.  See README.TXT for usage
> instructions.  (also include "ERROR:" in messages above.

Done.  (#relocation_packer --help' prints full instructions so I referred to
that instead.)

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:757:
packer.PackRelativeRelocations(relative_relocations, &packed);
On 2014/06/07 11:49:07, rmcilroy wrote:
> How about making PackRelativeRelocations return a bool if it was successful or
> not, and then do the Log/return false if it failed here rather than indirectly
> testing whether packed is empty?

Insufficient relocations to form a run is a pathological case that never occurs
in practice.  INIT_ARRAY and FINI_ARRAY alone provide the requisite minimum. 
Even compiling /dev/null produces more than two:

$ /usr/bin/arm-linux-gnueabi-g++ -shared -o /tmp/t.so -xc++ /dev/null
$ /usr/bin/arm-linux-gnueabi-readelf -a /tmp/t.so | grep R_ARM_RELATIVE
00008f14  00000017 R_ARM_RELATIVE   
00008f18  00000017 R_ARM_RELATIVE   
00009020  00000017 R_ARM_RELATIVE

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file_unittest.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file_unittest.cc:18: #define str(s) #s
On 2014/06/07 11:49:07, rmcilroy wrote:
> All uppercase for macro names.

Done.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file_unittest_relocs.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file_unittest_relocs.cc:28: 
On 2014/06/07 11:49:07, rmcilroy wrote:
> Move this file out of src and into test_data alongside the .so's please.

Done.

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/leb128.h (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/leb128.h:27: void Enqueue(uint32_t value);
On 2014/06/07 11:49:07, rmcilroy wrote:
> Newline

Done.

[simonb (inactive), 2014-06-09 14:46:49]

The CQ bit was checked by simonb@chromium.org

[rmcilroy, 2014-06-09 14:47:23]

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
File tools/relocation_packer/src/elf_file.cc (right):

https://codereview.chromium.org/310483003/diff/40001/tools/relocation_packer/...
tools/relocation_packer/src/elf_file.cc:757:
packer.PackRelativeRelocations(relative_relocations, &packed);
On 2014/06/09 14:39:19, simonb wrote:
> On 2014/06/07 11:49:07, rmcilroy wrote:
> > How about making PackRelativeRelocations return a bool if it was successful
or
> > not, and then do the Log/return false if it failed here rather than
indirectly
> > testing whether packed is empty?
> 
> Insufficient relocations to form a run is a pathological case that never
occurs
> in practice.  INIT_ARRAY and FINI_ARRAY alone provide the requisite minimum. 
> Even compiling /dev/null produces more than two:
> 
> $ /usr/bin/arm-linux-gnueabi-g++ -shared -o /tmp/t.so -xc++ /dev/null
> $ /usr/bin/arm-linux-gnueabi-readelf -a /tmp/t.so | grep R_ARM_RELATIVE
> 00008f14  00000017 R_ARM_RELATIVE   
> 00008f18  00000017 R_ARM_RELATIVE   
> 00009020  00000017 R_ARM_RELATIVE   

I figured this was the case - the suggestion wasn't for safety (that's already
dealt with by the vector size check), just for code clarity on the caller side.
Don't mind too much though.

[commit-bot: I haz the power, 2014-06-09 14:47:35]

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

[simonb (inactive), 2014-06-09 15:04:06]

The CQ bit was unchecked by simonb@chromium.org

[simonb (inactive), 2014-06-09 15:10:04]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-06-09 15:10:32]

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

[simonb (inactive), 2014-06-09 16:42:25]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-06-09 16:43:08]

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

[Andrew Hayden (chromium.org), 2014-06-09 16:44:44]

For posterity the separate dcommit of the binaries for testing is here:
https://codereview.chromium.org/310483003/
https://src.chromium.org/viewvc/chrome?revision=275818&view=revision

[Andrew Hayden (chromium.org), 2014-06-09 16:45:33]

Er, the code review for the dcommit:
https://codereview.chromium.org/325783002/

[commit-bot: I haz the power, 2014-06-10 08:48:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[commit-bot: I haz the power, 2014-06-10 11:50:55]

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

[commit-bot: I haz the power, 2014-06-10 11:50:56]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[simonb (inactive), 2014-06-10 12:30:02]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-06-10 12:36:35]

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

[commit-bot: I haz the power, 2014-06-10 15:57:54]

Change committed as 276043