Add gyp machinery to build with packed ARM relative relocations.

Add gyp machinery to build with packed ARM relative relocations.

Add gypi and python files to support packing ARM relative relocations
during the build process.

Define a use_relocation_packer gyp variable to turn ARM relocation
packing on and off (currently set to 0).

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

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

[rmcilroy, 2014-06-27 11:14:55]

Looks good overall, a couple of comments.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
File build/android/gyp/pack_arm_relocations.py (right):

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:6: 
nit - please add a docstring comment here describing the package.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:16: 
nit - single newline here (I believe, though still double newline between
functions)

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:25: with tempfile.NamedTemporaryFile()
as stream:
nit - /s/stream/section_contents_tmp

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:49: help='Pack relocations if true,
otherwise plain file copy')
Pack relocations if '1', otherwise plain file copy (also maybe just make this a
"choices" type with ['0', '1'] as the choices).

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:72: if enable_packing and library !=
'libchromium_android_linker.so':
This hardcoded name is getting sprinkled around the codebase - could we figure
out a way to have it as a gyp variable which is passed to this tool, as well as
being passed to Anton's rezip tool to avoid being hardcoded in both tools?

https://codereview.chromium.org/358863002/diff/1/build/android/pack_arm_reloc...
File build/android/pack_arm_relocations.gypi (right):

https://codereview.chromium.org/358863002/diff/1/build/android/pack_arm_reloc...
build/android/pack_arm_relocations.gypi:45: 'message': 'Copying libraries (no
packing) for <(_target_name)',
nit - (no relocation packing)

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode98
build/java_apk.gypi:98: 'pack_stamp': '<(intermediate_dir)/pack.stamp',
nit - pack_relocations.stamp

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode324
build/java_apk.gypi:324: 'stripped_libraries_dir': '<(SHARED_INTERMEDIATE_DIR)',
I'm not entirely familiar with the output directory structure, but I think we
should be  using a new directory here rather than re-using
SHARED_INTERMEDIATE_DIR (which presumably holds the unstriped version
currently).

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode344
build/java_apk.gypi:344: ['use_chromium_linker == 1 and use_relocation_packer ==
1', {
Can you make it an error to have "use_relocation_packer == 1" and
"use_chromium_linker == 0" instead of just disabling packing?

[simonb (inactive), 2014-06-30 16:23:23]

Thanks, fixes in patch set 2.  Ptal.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
File build/android/gyp/pack_arm_relocations.py (right):

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:6: 
On 2014/06/27 11:14:54, rmcilroy wrote:
> nit - please add a docstring comment here describing the package.

Done.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:16: 
On 2014/06/27 11:14:54, rmcilroy wrote:
> nit - single newline here (I believe, though still double newline between
> functions)

Done.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:25: with tempfile.NamedTemporaryFile()
as stream:
On 2014/06/27 11:14:54, rmcilroy wrote:
> nit - /s/stream/section_contents_tmp

Doesn't seem to offer much benefit.  Let me know if you strongly disagree.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:49: help='Pack relocations if true,
otherwise plain file copy')
On 2014/06/27 11:14:54, rmcilroy wrote:
> Pack relocations if '1', otherwise plain file copy (also maybe just make this
a
> "choices" type with ['0', '1'] as the choices). 

Done.  Looks better, thanks.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:72: if enable_packing and library !=
'libchromium_android_linker.so':
On 2014/06/27 11:14:54, rmcilroy wrote:
> This hardcoded name is getting sprinkled around the codebase - could we figure
> out a way to have it as a gyp variable which is passed to this tool, as well
as
> being passed to Anton's rezip tool to avoid being hardcoded in both tools?

Done.

https://codereview.chromium.org/358863002/diff/1/build/android/pack_arm_reloc...
File build/android/pack_arm_relocations.gypi (right):

https://codereview.chromium.org/358863002/diff/1/build/android/pack_arm_reloc...
build/android/pack_arm_relocations.gypi:45: 'message': 'Copying libraries (no
packing) for <(_target_name)',
On 2014/06/27 11:14:54, rmcilroy wrote:
> nit - (no relocation packing)

Done.

https://codereview.chromium.org/358863002/diff/1/build/android/pack_arm_reloc...
build/android/pack_arm_relocations.gypi:45: 'message': 'Copying libraries (no
packing) for <(_target_name)',
On 2014/06/27 11:14:54, rmcilroy wrote:
> nit - (no relocation packing)

Done.

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode98
build/java_apk.gypi:98: 'pack_stamp': '<(intermediate_dir)/pack.stamp',
On 2014/06/27 11:14:54, rmcilroy wrote:
> nit - pack_relocations.stamp

Done.

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode324
build/java_apk.gypi:324: 'stripped_libraries_dir': '<(SHARED_INTERMEDIATE_DIR)',
On 2014/06/27 11:14:55, rmcilroy wrote:
> I'm not entirely familiar with the output directory structure, but I think we
> should be  using a new directory here rather than re-using
> SHARED_INTERMEDIATE_DIR (which presumably holds the unstriped version
> currently).

SHARED_INTERMEDIATE_DIR doesn't currently hold unstripped libs.  These are in
SHARED_LIB_DIR.  Previous to this change the strip action stripped and copied
from SHARED_LIB_DIR into the apk staging dir.  Now the strip and copy outputs to
an intermediate dir, and relocation packing does the final copy out to apk
staging.  SHARED_INTERMEDIATE_DIR is the directory gyp supplies for
communicating between actions.

I've amended the code to create a new subdirectory under
SHARED_INTERMEDIATE_DIR, just for communicating between strip_native_libraries
and pack_arm_relocations.  Hopefully this makes what is happening clearer.

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode344
build/java_apk.gypi:344: ['use_chromium_linker == 1 and use_relocation_packer ==
1', {
On 2014/06/27 11:14:55, rmcilroy wrote:
> Can you make it an error to have "use_relocation_packer == 1" and
> "use_chromium_linker == 0" instead of just disabling packing?

Gyp appears to lack any mechanism to raise an exception or otherwise 'error
out'.

[rmcilroy, 2014-07-01 11:13:03]

Looks good overall, thanks.

lgtm once trybots are green and comments addressed.

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
File build/android/gyp/pack_arm_relocations.py (right):

https://codereview.chromium.org/358863002/diff/1/build/android/gyp/pack_arm_r...
build/android/gyp/pack_arm_relocations.py:25: with tempfile.NamedTemporaryFile()
as stream:
On 2014/06/30 16:23:23, simonb wrote:
> On 2014/06/27 11:14:54, rmcilroy wrote:
> > nit - /s/stream/section_contents_tmp
> 
> Doesn't seem to offer much benefit.  Let me know if you strongly disagree.

To me it makes the code more readable.  "stream.write('NULL')" doesn't convey
much to me, but "section_contents_tmp.write('NULL')" explains to me what it's
doing.  I don't strongly care though.

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode324
build/java_apk.gypi:324: 'stripped_libraries_dir': '<(SHARED_INTERMEDIATE_DIR)',
On 2014/06/30 16:23:23, simonb wrote:
> On 2014/06/27 11:14:55, rmcilroy wrote:
> > I'm not entirely familiar with the output directory structure, but I think
we
> > should be  using a new directory here rather than re-using
> > SHARED_INTERMEDIATE_DIR (which presumably holds the unstriped version
> > currently).
> 
> SHARED_INTERMEDIATE_DIR doesn't currently hold unstripped libs.  These are in
> SHARED_LIB_DIR.  Previous to this change the strip action stripped and copied
> from SHARED_LIB_DIR into the apk staging dir.  Now the strip and copy outputs
to
> an intermediate dir, and relocation packing does the final copy out to apk
> staging.  SHARED_INTERMEDIATE_DIR is the directory gyp supplies for
> communicating between actions.
> 
> I've amended the code to create a new subdirectory under
> SHARED_INTERMEDIATE_DIR, just for communicating between strip_native_libraries
> and pack_arm_relocations.  Hopefully this makes what is happening clearer.

This is perfect, thanks for making the change.

https://codereview.chromium.org/358863002/diff/1/build/java_apk.gypi#newcode344
build/java_apk.gypi:344: ['use_chromium_linker == 1 and use_relocation_packer ==
1', {
On 2014/06/30 16:23:23, simonb wrote:
> On 2014/06/27 11:14:55, rmcilroy wrote:
> > Can you make it an error to have "use_relocation_packer == 1" and
> > "use_chromium_linker == 0" instead of just disabling packing?
> 
> Gyp appears to lack any mechanism to raise an exception or otherwise 'error
> out'.
> 

I was afraid of that.. Thanks for checking.

https://codereview.chromium.org/358863002/diff/40001/build/android/gyp/pack_a...
File build/android/gyp/pack_arm_relocations.py (right):

https://codereview.chromium.org/358863002/diff/40001/build/android/gyp/pack_a...
build/android/gyp/pack_arm_relocations.py:85: suppress_packing = []
nit - just add a default to the parser option?

https://codereview.chromium.org/358863002/diff/40001/build/android/pack_arm_r...
File build/android/pack_arm_relocations.gypi (right):

https://codereview.chromium.org/358863002/diff/40001/build/android/pack_arm_r...
build/android/pack_arm_relocations.gypi:14: #        'suppress_packing': 'names
of libraries explicitly not packed',
nit - 'suppress_packing' sounds like the opposite of the enable_packing flag. 
How about 'exclude_packing_list'?  Update here and in the python tool.

https://codereview.chromium.org/358863002/diff/40001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/358863002/diff/40001/build/java_apk.gypi#newc...
build/java_apk.gypi:57: #    linker, so use_chromium_linker must also be
enabled.
remove one of these.

[simonb (inactive), 2014-07-01 14:20:29]

Fixes in patch set 4.

https://codereview.chromium.org/358863002/diff/40001/build/android/gyp/pack_a...
File build/android/gyp/pack_arm_relocations.py (right):

https://codereview.chromium.org/358863002/diff/40001/build/android/gyp/pack_a...
build/android/gyp/pack_arm_relocations.py:85: suppress_packing = []
On 2014/07/01 11:13:03, rmcilroy wrote:
> nit - just add a default to the parser option?

Done.

https://codereview.chromium.org/358863002/diff/40001/build/android/pack_arm_r...
File build/android/pack_arm_relocations.gypi (right):

https://codereview.chromium.org/358863002/diff/40001/build/android/pack_arm_r...
build/android/pack_arm_relocations.gypi:14: #        'suppress_packing': 'names
of libraries explicitly not packed',
On 2014/07/01 11:13:03, rmcilroy wrote:
> nit - 'suppress_packing' sounds like the opposite of the enable_packing flag. 
> How about 'exclude_packing_list'?  Update here and in the python tool.

Done.

https://codereview.chromium.org/358863002/diff/40001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/358863002/diff/40001/build/java_apk.gypi#newc...
build/java_apk.gypi:57: #    linker, so use_chromium_linker must also be
enabled.
On 2014/07/01 11:13:03, rmcilroy wrote:
> remove one of these.

Oops, thanks.  Done.

[simonb (inactive), 2014-07-03 12:37:40]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-07-03 12:39:05]

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

[commit-bot: I haz the power, 2014-07-03 17:21:06]

Change committed as 281286

[simonb (inactive), 2014-07-04 11:25:42]

Added fix for sporadic 'no such file' error on builds.  Ptal, thanks.

Code change is:

diff --git a/build/android/gyp/pack_arm_relocations.py
b/build/android/gyp/pack_arm_relocations.py
index 9fbd977..f117ad7 100755
--- a/build/android/gyp/pack_arm_relocations.py
+++ b/build/android/gyp/pack_arm_relocations.py
@@ -86,6 +86,8 @@ def main():
   with open(options.libraries_file, 'r') as libfile:
     libraries = json.load(libfile)
 
+  build_utils.MakeDirectory(options.packed_libraries_dir)
+
   for library in libraries:
     library_path = os.path.join(options.stripped_libraries_dir, library)
     output_path = os.path.join(options.packed_libraries_dir, library)

[rmcilroy, 2014-07-04 14:34:47]

lgtm

[simonb (inactive), 2014-07-04 14:36:18]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-07-04 14:37:32]

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

[commit-bot: I haz the power, 2014-07-04 16:17:50]

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

[commit-bot: I haz the power, 2014-07-04 19:58:39]

Change committed as 281433