Build V8 for Android IA

Build V8 for Android IA

Committed: https://code.google.com/p/v8/source/detail?r=12122

[haitao.feng, 2012-07-17 14:54:36]

This patch set enables building V8 for Android IA.

Usage:
  export ANDROID_NDK_ROOT=$(NDK location)
  . tools/android-envsetup.sh ia32 (or arm)
  make (ia32|arm).android.(debug|release)

Currently it only works on Linux platform. Mac will be added soon.

The .check target does not work. The corresponding scripts should be updated as
ia32 is added. Another patch will solve this problem.

The ANDROID_MODES targets are not supported in Makefile. Do we really want to
build ARM and IA32 together with make android.(debug|release)? Actually "make
all" does not work for ARM as it needs a cross toolchain.

[Jakob Kummerow, 2012-07-17 16:29:57]

Great, I like this approach! But I do have a few comments.

On 2012/07/17 14:54:36, haitao.feng wrote:
> The .check target does not work. The corresponding scripts should be updated
as
> ia32 is added. Another patch will solve this problem.

Yes, please do fix the .check targets. (It's fine to do that in another CL, but
don't wait too long please.)

> The ANDROID_MODES targets are not supported in Makefile. Do we really want to
> build ARM and IA32 together with make android.(debug|release)? 

I agree, I don't think it makes sense to have an "android.release" target that
builds android_ia32.release and android_arm.release". As I commented in the
Makefile, I'd suggest to remove ANDROID_MODES completely.

> Actually "make
> all" does not work for ARM as it needs a cross toolchain.

Oh yes, it works just fine, and we use it all the time. It uses V8's built-in
ARM simulator ;-)
(Unless it detects that CXX is a cross-compiler.)

https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile
File Makefile (right):

https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode123
Makefile:123: ANDROID_ARCHES = ia32.android arm.android
Can we swap these around (to "android.ia32")? Just for the sake of easier tab
completion in the out/ dir ;-)
(See also my comment below regarding "android_ia32" with an underscore.)

https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode133
Makefile:133: ANDROID_BUILDS = $(foreach mode,$(suffix $(ANDROID_MODES)), \
Let's s/$(suffix $(ANDROID_MODES))/$(MODES)/ here and below, and remove
ANDROID_MODES entirely.

https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode144
Makefile:144: $(ANDROID_ARCHES) $(ANDROID_BUILD) $(ANDROID_CHECKS) \
s/ANDROID_BUILD/ANDROID_BUILDS/

https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode179
Makefile:179: $(ANDROID_ARCHES): $(addprefix $$@,$(suffix $(ANDROID_MODES)))
Again, s/$(suffix $(ANDROID_MODES))/$(MODES)/.

https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode262
Makefile:262: -S$(suffix $(basename $@)).$(subst .,,$(suffix $@)) \
This line would become simpler if you defined:
ANDROID_ARCHES = android_ia32 android_arm
and I don't see a reason why underscores instead of dots would hurt (it might
make it more difficult to fix the test runner script, though).
If you prefer the dots, that's fine too, this is just a suggestion. (The reason
we use dots for "ia32.release" etc. is just so that $(basename ...) and $(suffix
...) are useful.)

https://chromiumcodereview.appspot.com/10778033/diff/1/tools/android-envsetup.sh
File tools/android-envsetup.sh (right):

https://chromiumcodereview.appspot.com/10778033/diff/1/tools/android-envsetup...
tools/android-envsetup.sh:78: export GYP_DEFINES="${DEFINES}"
I don't like that GYP_DEFINES are overridden here. People might use that
environment variable to store their default options.
On the other hand, if you append to the variable, 'source'ing this script twice
in the same shell will create a mess... Still, I think I'd prefer appending.
[Other potential solutions I can think of that are more work:
(1) Use this script as a wrapper around 'make', obviating the need to override
the calling shell's environment?
(2) Specifically filter conflicting definitions from the existing GYP_DEFINES,
then append the new DEFINES?
]

[haitao.feng, 2012-07-18 03:03:15]

Thanks for your review and comments. It is interesting that you simulate the
JITed arm instruction in X86. I will read the source codes to learn how it works
:)

For the codes:
1) The name is changed according to your suggestion. And .check works.  Here is
the current x86 results:
   [08:31|% 100|+ 8601|- 105]: Done
It is much faster than the 90 minutes running in ARM emulator. I will
investigate the failed cases.

2) "Use this script as a wrapper around 'make', obviating the need to override
the calling shell's environment" is done in tools/android-build.sh. Now no side
effect and the usage is:
   a)export ANDROID_NDK_ROOT=$(NDK location)
   b)make (ia32|arm).android.(debug|release)
Please update wiki when this CL is committed.

For the others, mipsel is not included as I do not have a MIPS android phone or
emulator.

On 2012/07/17 16:29:57, Jakob wrote:
> Great, I like this approach! But I do have a few comments.
> 
> On 2012/07/17 14:54:36, haitao.feng wrote:
> > The .check target does not work. The corresponding scripts should be updated
> as
> > ia32 is added. Another patch will solve this problem.
> 
> Yes, please do fix the .check targets. (It's fine to do that in another CL,
but
> don't wait too long please.)
> 
> > The ANDROID_MODES targets are not supported in Makefile. Do we really want
to
> > build ARM and IA32 together with make android.(debug|release)? 
> 
> I agree, I don't think it makes sense to have an "android.release" target that
> builds android_ia32.release and android_arm.release". As I commented in the
> Makefile, I'd suggest to remove ANDROID_MODES completely.
> 
> > Actually "make
> > all" does not work for ARM as it needs a cross toolchain.
> 
> Oh yes, it works just fine, and we use it all the time. It uses V8's built-in
> ARM simulator ;-)
> (Unless it detects that CXX is a cross-compiler.)
> 
> https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile
> File Makefile (right):
> 
> https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode123
> Makefile:123: ANDROID_ARCHES = ia32.android arm.android
> Can we swap these around (to "android.ia32")? Just for the sake of easier tab
> completion in the out/ dir ;-)
> (See also my comment below regarding "android_ia32" with an underscore.)
> 
> https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode133
> Makefile:133: ANDROID_BUILDS = $(foreach mode,$(suffix $(ANDROID_MODES)), \
> Let's s/$(suffix $(ANDROID_MODES))/$(MODES)/ here and below, and remove
> ANDROID_MODES entirely.
> 
> https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode144
> Makefile:144: $(ANDROID_ARCHES) $(ANDROID_BUILD) $(ANDROID_CHECKS) \
> s/ANDROID_BUILD/ANDROID_BUILDS/
> 
> https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode179
> Makefile:179: $(ANDROID_ARCHES): $(addprefix $$@,$(suffix $(ANDROID_MODES)))
> Again, s/$(suffix $(ANDROID_MODES))/$(MODES)/.
> 
> https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode262
> Makefile:262: -S$(suffix $(basename $@)).$(subst .,,$(suffix $@)) \
> This line would become simpler if you defined:
> ANDROID_ARCHES = android_ia32 android_arm
> and I don't see a reason why underscores instead of dots would hurt (it might
> make it more difficult to fix the test runner script, though).
> If you prefer the dots, that's fine too, this is just a suggestion. (The
reason
> we use dots for "ia32.release" etc. is just so that $(basename ...) and
$(suffix
> ...) are useful.)
> 
>
https://chromiumcodereview.appspot.com/10778033/diff/1/tools/android-envsetup.sh
> File tools/android-envsetup.sh (right):
> 
>
https://chromiumcodereview.appspot.com/10778033/diff/1/tools/android-envsetup...
> tools/android-envsetup.sh:78: export GYP_DEFINES="${DEFINES}"
> I don't like that GYP_DEFINES are overridden here. People might use that
> environment variable to store their default options.
> On the other hand, if you append to the variable, 'source'ing this script
twice
> in the same shell will create a mess... Still, I think I'd prefer appending.
> [Other potential solutions I can think of that are more work:
> (1) Use this script as a wrapper around 'make', obviating the need to override
> the calling shell's environment?
> (2) Specifically filter conflicting definitions from the existing GYP_DEFINES,
> then append the new DEFINES?
> ]

[haitao.feng, 2012-07-18 03:10:13]

"b) make (ia32|arm).android.(debug|release)" 
should be:
   b) make (android_ia32|android_arm).(debug|release)

On 2012/07/18 03:03:15, haitao.feng wrote:
> Thanks for your review and comments. It is interesting that you simulate the
> JITed arm instruction in X86. I will read the source codes to learn how it
works
> :)
> 
> For the codes:
> 1) The name is changed according to your suggestion. And .check works.  Here
is
> the current x86 results:
>    [08:31|% 100|+ 8601|- 105]: Done
> It is much faster than the 90 minutes running in ARM emulator. I will
> investigate the failed cases.
> 
> 2) "Use this script as a wrapper around 'make', obviating the need to override
> the calling shell's environment" is done in tools/android-build.sh. Now no
side
> effect and the usage is:
>    a)export ANDROID_NDK_ROOT=$(NDK location)
>    b)make (ia32|arm).android.(debug|release)
> Please update wiki when this CL is committed.
> 
> For the others, mipsel is not included as I do not have a MIPS android phone
or
> emulator.
> 
> On 2012/07/17 16:29:57, Jakob wrote:
> > Great, I like this approach! But I do have a few comments.
> > 
> > On 2012/07/17 14:54:36, haitao.feng wrote:
> > > The .check target does not work. The corresponding scripts should be
updated
> > as
> > > ia32 is added. Another patch will solve this problem.
> > 
> > Yes, please do fix the .check targets. (It's fine to do that in another CL,
> but
> > don't wait too long please.)
> > 
> > > The ANDROID_MODES targets are not supported in Makefile. Do we really want
> to
> > > build ARM and IA32 together with make android.(debug|release)? 
> > 
> > I agree, I don't think it makes sense to have an "android.release" target
that
> > builds android_ia32.release and android_arm.release". As I commented in the
> > Makefile, I'd suggest to remove ANDROID_MODES completely.
> > 
> > > Actually "make
> > > all" does not work for ARM as it needs a cross toolchain.
> > 
> > Oh yes, it works just fine, and we use it all the time. It uses V8's
built-in
> > ARM simulator ;-)
> > (Unless it detects that CXX is a cross-compiler.)
> > 
> > https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile
> > File Makefile (right):
> > 
> > https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode123
> > Makefile:123: ANDROID_ARCHES = ia32.android arm.android
> > Can we swap these around (to "android.ia32")? Just for the sake of easier
tab
> > completion in the out/ dir ;-)
> > (See also my comment below regarding "android_ia32" with an underscore.)
> > 
> > https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode133
> > Makefile:133: ANDROID_BUILDS = $(foreach mode,$(suffix $(ANDROID_MODES)), \
> > Let's s/$(suffix $(ANDROID_MODES))/$(MODES)/ here and below, and remove
> > ANDROID_MODES entirely.
> > 
> > https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode144
> > Makefile:144: $(ANDROID_ARCHES) $(ANDROID_BUILD) $(ANDROID_CHECKS) \
> > s/ANDROID_BUILD/ANDROID_BUILDS/
> > 
> > https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode179
> > Makefile:179: $(ANDROID_ARCHES): $(addprefix $$@,$(suffix $(ANDROID_MODES)))
> > Again, s/$(suffix $(ANDROID_MODES))/$(MODES)/.
> > 
> > https://chromiumcodereview.appspot.com/10778033/diff/1/Makefile#newcode262
> > Makefile:262: -S$(suffix $(basename $@)).$(subst .,,$(suffix $@)) \
> > This line would become simpler if you defined:
> > ANDROID_ARCHES = android_ia32 android_arm
> > and I don't see a reason why underscores instead of dots would hurt (it
might
> > make it more difficult to fix the test runner script, though).
> > If you prefer the dots, that's fine too, this is just a suggestion. (The
> reason
> > we use dots for "ia32.release" etc. is just so that $(basename ...) and
> $(suffix
> > ...) are useful.)
> > 
> >
>
https://chromiumcodereview.appspot.com/10778033/diff/1/tools/android-envsetup.sh
> > File tools/android-envsetup.sh (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10778033/diff/1/tools/android-envsetup...
> > tools/android-envsetup.sh:78: export GYP_DEFINES="${DEFINES}"
> > I don't like that GYP_DEFINES are overridden here. People might use that
> > environment variable to store their default options.
> > On the other hand, if you append to the variable, 'source'ing this script
> twice
> > in the same shell will create a mess... Still, I think I'd prefer appending.
> > [Other potential solutions I can think of that are more work:
> > (1) Use this script as a wrapper around 'make', obviating the need to
override
> > the calling shell's environment?
> > (2) Specifically filter conflicting definitions from the existing
GYP_DEFINES,
> > then append the new DEFINES?
> > ]

[Jakob Kummerow, 2012-07-18 11:28:20]

Great work, thanks! LGTM.

I'll fix the nits below and land this for you.

https://chromiumcodereview.appspot.com/10778033/diff/2002/Makefile
File Makefile (right):

https://chromiumcodereview.appspot.com/10778033/diff/2002/Makefile#newcode222
Makefile:222: rm -f $(OUTDIR)/Makefile.$(basename $@).release
We don't actually need to generate these, so we don't need to clean them up
either (see my comment in android-build.sh).

https://chromiumcodereview.appspot.com/10778033/diff/2002/tools/android-build.sh
File tools/android-build.sh (right):

https://chromiumcodereview.appspot.com/10778033/diff/2002/tools/android-build...
tools/android-build.sh:82: -S.${ARCH}.${MODE} ${GYPFLAGS}
The generated Makefile is not release/debug specific.

https://chromiumcodereview.appspot.com/10778033/diff/2002/tools/test-wrapper-...
File tools/test-wrapper-gypbuild.py (right):

https://chromiumcodereview.appspot.com/10778033/diff/2002/tools/test-wrapper-...
tools/test-wrapper-gypbuild.py:151: if not arch in ['ia32', 'x64', 'arm',
'mipsel', 'android_arm', 'android_ia32']:
nit: long line