Preserve section and data alignment in libchrome.<ver>.so.

Preserve section and data alignment in libchrome.<ver>.so.

Current packing preserves 4-byte alignment (in practice, 8-byte alignment,
because a whole number of R_ARM_RELATIVE relocations is removed).  However
this is insufficient for some NEON vector instructions.  For example:

  18b0c:       f42c026f        vld1.16 {d0-d3}, [ip :128]

requires a 16-byte alignment, and fails with SIGBUS if not aligned.

The NDK toolchain aligns .text and .data to 256 bytes.  Retain this by
padding the non-packed (non-R_ARM_RELATIVE) relocations with sufficient
R_ARM_NONE entries so that 256 byte alignment is preserved.  Update
test data so that enough R_ARM_RELATIVE entries are present for packing
to reduce file size.

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

[rmcilroy, 2014-06-18 09:52:29]

lgtm if comments are addressed (or are not applicable).

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

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:828: if (is_padding_rel_dyn_) {
Could you just do this in the "else" block of the above "if
(!is_padding_rel_dyn_)" ? I think this would be clearer if possible.

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:837: ResizeSection(elf_,
rel_dyn_section_, bytes);
Do you need to pad this to kPreserveAlignment as well to ensure that the
sections following  .rel.dyn are also aligned?

[simonb1, 2014-06-18 11:30:20]

Feedback addressed in patch set 2.  Thanks for the review.

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

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:828: if (is_padding_rel_dyn_) {
On 2014/06/18 09:52:29, rmcilroy wrote:
> Could you just do this in the "else" block of the above "if
> (!is_padding_rel_dyn_)" ? I think this would be clearer if possible.

Done.

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:837: ResizeSection(elf_,
rel_dyn_section_, bytes);
On 2014/06/18 09:52:29, rmcilroy wrote:
> Do you need to pad this to kPreserveAlignment as well to ensure that the
> sections following  .rel.dyn are also aligned?

Shouldn't.  The R_ARM_NONE padding in other_relocations (line 781) is what
ensures that sections following .rel.dyn retain their present alignments.  Put
differently, I'm ensuring that the hole removed from the file does not disturb
any alignments; that means aligning the hole size rather than the remaining
data.  (The alignment of .rel.dyn itself is 4 bytes, and relocation elements are
each 8 bytes.)

[simonb1, 2014-06-18 11:30:52]

The CQ bit was checked by simonb@google.com

[commit-bot: I haz the power, 2014-06-18 11:32:22]

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

[rmcilroy, 2014-06-18 13:01:01]

The CQ bit was unchecked by rmcilroy@chromium.org

[rmcilroy, 2014-06-18 13:01:07]

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

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:837: ResizeSection(elf_,
rel_dyn_section_, bytes);
On 2014/06/18 11:30:19, simonb1 wrote:
> On 2014/06/18 09:52:29, rmcilroy wrote:
> > Do you need to pad this to kPreserveAlignment as well to ensure that the
> > sections following  .rel.dyn are also aligned?
> 
> Shouldn't.  The R_ARM_NONE padding in other_relocations (line 781) is what
> ensures that sections following .rel.dyn retain their present alignments.  Put
> differently, I'm ensuring that the hole removed from the file does not disturb
> any alignments; that means aligning the hole size rather than the remaining
> data.  (The alignment of .rel.dyn itself is 4 bytes, and relocation elements
are
> each 8 bytes.)

Sorry - see below.

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:844: RewriteSectionData(data,
packed_data, packed_bytes);
Sorry I got the comment on the wrong line - I was meaning here, should we be
ensuring the size added to android.rel.dyn keeps all the following sections
aligned?

[simonb1, 2014-06-18 13:29:09]

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

https://codereview.chromium.org/341483002/diff/1/tools/relocation_packer/src/...
tools/relocation_packer/src/elf_file.cc:844: RewriteSectionData(data,
packed_data, packed_bytes);
On 2014/06/18 13:01:07, rmcilroy wrote:
> Sorry I got the comment on the wrong line - I was meaning here, should we be
> ensuring the size added to android.rel.dyn keeps all the following sections
> aligned?

Happily, no.  Libbfd locates .android.rel.dyn as the penultimate section.  That
is, at then end of data sections and just before the final .shstrtab section. 
And both .android.rel.dyn and .shstrtab have alignment of 1.

[simonb1, 2014-06-18 13:29:25]

The CQ bit was checked by simonb@google.com

[commit-bot: I haz the power, 2014-06-18 13:30:55]

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

[commit-bot: I haz the power, 2014-06-18 17:01:11]

Change committed as 278105