Compile ICU for [P]NaCl

Compile ICU for [P]NaCl

Added new targets icuuc_untrusted, icui18n_untrusted and icudata_untrusted
that compile ICU for NaCl and PNaCl.

BUG=276739
R=mark@chromium.org

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

[jungshik at Google, 2013-12-19 01:22:12]

On 2013/12/18 02:13:44, Sergey Ulanov wrote:

Can you add the procedure you used to generate pnacl.h to README.chromium?  

Also, can (P)NaCl use icu data file instead of static linking?  I'm making a
series of CLs to use icudata file instead of statically linking icu data. Would
it be better for (P)NaCl?

[jungshik at Google, 2013-12-19 06:24:55]

On 2013/12/19 01:22:12, Jungshik Shin wrote:
> On 2013/12/18 02:13:44, Sergey Ulanov wrote:
> 
> Can you add the procedure you used to generate pnacl.h to README.chromium?  
> 
> Also, can (P)NaCl use icu data file instead of static linking?  I'm making a
> series of CLs to use icudata file instead of statically linking icu data.
Would
> it be better for (P)NaCl?

See http://crbug.com/72633 and CLs referred to there. If (P)NaCl can use the
icudata file as opposed to statically linking it on Linux/Mac, I guess it can
save the Chrome download size by sharing the same icudata file. I'm just
guessing here without knowing much about the way (P)NaCl is built and packaged
with Chrome.

[jungshik at Google, 2013-12-19 06:25:05]

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp
File icu_untrusted.gyp (right):

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode58
icu_untrusted.gyp:58: 'source/data/in/icudt46l.dat'
This was renamed to icudtl.dat to make it version-agnostic. No need to change
the filename when ICU is updated.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode65
icu_untrusted.gyp:65: 'source/data/in/icudt46l.dat', '<(data_c_file)',
ditto

[Mark Mentovai, 2013-12-19 18:26:47]

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py
File icu_data_to_c.py (right):

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py#newcode25
icu_data_to_c.py:25: #endif
#else __attribute__((visibility("hidden")))

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py#newcode26
icu_data_to_c.py:26: uint32_t icudt46_dat[] = {
This should absolutely 100% be const.

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py#newcode34
icu_data_to_c.py:34: bytes = data[pos:pos+4]
Uh-oh! If len(data) % 4 != 0, the last one of these will silently have fewer
than four bytes.

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py#newcode37
icu_data_to_c.py:37: if line_pos >= 8:
Make this |if line_pos % 8 == 0:|. Then you can get rid of the |line_pos = 0|
reassignment on line 39.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp
File icu_untrusted.gyp (right):

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode1
icu_untrusted.gyp:1: # Copyright 2013 The Chromium Authors. All rights reserved.
icu doesn’t really know anything special about NaCl, so the word “untrusted”
won’t necessarily make any sense to someone looking here. This should be named
icu_nacl_untrusted.gyp.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode7
icu_untrusted.gyp:7: 'werror': '',
Why?

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode30
icu_untrusted.gyp:30: '-w',
Why?

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode43
icu_untrusted.gyp:43:
'<(SHARED_INTERMEDIATE_DIR)/third_party/icu/nacl/iucdt46l.c',
This file is only dealt with from one target. It can be in INTERMEDIATE_DIR. It
doesn’t need to be in SHARED_INTERMEDIATE_DIR.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode48
icu_untrusted.gyp:48: '<(DEPTH)/native_client/tools.gyp:prep_toolchain',
You know where native_client is relative to icu in the source tree. Give the
right number of dots. <(DEPTH) is deprecated and should only be used in
instances where you can’t say what you mean to say with dots.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode64
icu_untrusted.gyp:64: 'python', 'icu_data_to_c.py',
Don’t write the same names more than once when you can avoid it: this can be
['python', '<@(_inputs)', '<@(_outputs)']

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode86
icu_untrusted.gyp:86: '<(DEPTH)/native_client/tools.gyp:prep_toolchain',
As on line 47. And the same on line 111.

https://codereview.chromium.org/109553005/diff/30001/source/common/unicode/pn...
File source/common/unicode/pnacl.h (right):

https://codereview.chromium.org/109553005/diff/30001/source/common/unicode/pn...
source/common/unicode/pnacl.h:28: * \brief Basic types for the platform
Jungshik can answer for sure, but we typically generate a patch file for ICU
code changes like your changes to pnacl.h and ptypes.h and stick them in the
patches directory. We also update README.chromium with information about the
patch. But please follow Jungshik’s advice here.

[Sergey Ulanov, 2013-12-20 00:23:34]

On 2013/12/19 06:24:55, Jungshik Shin wrote:
> On 2013/12/19 01:22:12, Jungshik Shin wrote:
> > On 2013/12/18 02:13:44, Sergey Ulanov wrote:
> > 
> > Can you add the procedure you used to generate pnacl.h to README.chromium?  
> > 
> > Also, can (P)NaCl use icu data file instead of static linking?  I'm making a
> > series of CLs to use icudata file instead of statically linking icu data.
> Would
> > it be better for (P)NaCl?
> 
> See http://crbug.com/72633 and CLs referred to there. If (P)NaCl can use the
> icudata file as opposed to statically linking it on Linux/Mac, I guess it can
> save the Chrome download size by sharing the same icudata file. I'm just
> guessing here without knowing much about the way (P)NaCl is built and packaged
> with Chrome.

For NaCl we can't load data directly form disk, but I think it can be packaged
with extension/app and loaded from there. For drive-by web (with PNaCl) it can
be loaded from web.
In any case, Chromoting client plugin doesn't really use ICU. We only need it
because src/base and src/net depend on it (but we don't use parts of base and
net that actually use ICU). The plugin will work even without ICU data (and not
having it makes the plugin smaller). I didn't know that loading ICU data
dynamically is an option. I've removed the python script and compile stubdata.c
to the icudata_untrusted target.

[Sergey Ulanov, 2013-12-20 00:55:21]

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py
File icu_data_to_c.py (right):

https://codereview.chromium.org/109553005/diff/30001/icu_data_to_c.py#newcode25
icu_data_to_c.py:25: #endif
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> #else __attribute__((visibility("hidden")))

Removed this script.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp
File icu_untrusted.gyp (right):

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode1
icu_untrusted.gyp:1: # Copyright 2013 The Chromium Authors. All rights reserved.
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> icu doesn’t really know anything special about NaCl, so the word “untrusted”
> won’t necessarily make any sense to someone looking here. This should be named
> icu_nacl_untrusted.gyp.

untrusted is the suffix used for all other NaCl targets (e.g. base_untrusted). I
agree that calling them base_nacl and ice_nacl would be better, but
ice_untrusted is more consistent with what we already have.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode7
icu_untrusted.gyp:7: 'werror': '',
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> Why?

Removed.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode30
icu_untrusted.gyp:30: '-w',
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> Why?

Replaced with suppressions for specific warnings that we get in ICU.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode43
icu_untrusted.gyp:43:
'<(SHARED_INTERMEDIATE_DIR)/third_party/icu/nacl/iucdt46l.c',
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> This file is only dealt with from one target. It can be in INTERMEDIATE_DIR.
It
> doesn’t need to be in SHARED_INTERMEDIATE_DIR.

Remove this line.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode48
icu_untrusted.gyp:48: '<(DEPTH)/native_client/tools.gyp:prep_toolchain',
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> You know where native_client is relative to icu in the source tree. Give the
> right number of dots. <(DEPTH) is deprecated and should only be used in
> instances where you can’t say what you mean to say with dots.

Done.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode58
icu_untrusted.gyp:58: 'source/data/in/icudt46l.dat'
On 2013/12/19 06:25:05, Jungshik Shin wrote:
> This was renamed to icudtl.dat to make it version-agnostic. No need to change
> the filename when ICU is updated. 

Done.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode64
icu_untrusted.gyp:64: 'python', 'icu_data_to_c.py',
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> Don’t write the same names more than once when you can avoid it: this can be
> ['python', '<@(_inputs)', '<@(_outputs)']

Done.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode65
icu_untrusted.gyp:65: 'source/data/in/icudt46l.dat', '<(data_c_file)',
On 2013/12/19 06:25:05, Jungshik Shin wrote:
> ditto

Done.

https://codereview.chromium.org/109553005/diff/30001/icu_untrusted.gyp#newcode86
icu_untrusted.gyp:86: '<(DEPTH)/native_client/tools.gyp:prep_toolchain',
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> As on line 47. And the same on line 111.

Done.

https://codereview.chromium.org/109553005/diff/30001/source/common/unicode/pn...
File source/common/unicode/pnacl.h (right):

https://codereview.chromium.org/109553005/diff/30001/source/common/unicode/pn...
source/common/unicode/pnacl.h:28: * \brief Basic types for the platform
On 2013/12/19 18:26:47, Mark Mentovai wrote:
> Jungshik can answer for sure, but we typically generate a patch file for ICU
> code changes like your changes to pnacl.h and ptypes.h and stick them in the
> patches directory. We also update README.chromium with information about the
> patch. But please follow Jungshik’s advice here.

Done.

[Mark Mentovai, 2013-12-20 19:03:00]

LGTM. It’s a shame about the proliferation of the “untrusted” names. Maybe we
should file a bug to change them to nacl or nacl_untrusted. If it wasn’t for the
contributions I made to NaCl, I would have had no idea what “untrusted” meant
here.

https://codereview.chromium.org/109553005/diff/90001/README.chromium
File README.chromium (right):

https://codereview.chromium.org/109553005/diff/90001/README.chromium#newcode38
README.chromium:38: pnacl.h and applying the patches/pnacl.h.patch.
So this happens to the original plinux.h, not the patched plinux.h that the next
line (https://codereview.chromium.org/15973007/) references?

[Sergey Ulanov, 2013-12-20 19:36:21]

https://codereview.chromium.org/109553005/diff/90001/README.chromium
File README.chromium (right):

https://codereview.chromium.org/109553005/diff/90001/README.chromium#newcode38
README.chromium:38: pnacl.h and applying the patches/pnacl.h.patch.
On 2013/12/20 19:03:00, Mark Mentovai wrote:
> So this happens to the original plinux.h, not the patched plinux.h that the
next
> line (https://codereview.chromium.org/15973007/) references?

Good point - I didn't notice there is another patch applied to plinux.h .
Updated pnacl.h.patch so now it applies to the original plinux.h, before
https://codereview.chromium.org/15973007/

[Mark Mentovai, 2013-12-20 19:42:26]

https://codereview.chromium.org/109553005/diff/130001/patches/pnacl.h.patch
File patches/pnacl.h.patch (right):

https://codereview.chromium.org/109553005/diff/130001/patches/pnacl.h.patch#n...
patches/pnacl.h.patch:8: +#if 1
Something’s weird. The actual .h has #if defined(__GLIBC__) here. If this really
was supposed to be #if 1, the #else would be kind of pointless.

[Sergey Ulanov, 2013-12-20 19:48:03]

https://codereview.chromium.org/109553005/diff/130001/patches/pnacl.h.patch
File patches/pnacl.h.patch (right):

https://codereview.chromium.org/109553005/diff/130001/patches/pnacl.h.patch#n...
patches/pnacl.h.patch:8: +#if 1
On 2013/12/20 19:42:27, Mark Mentovai wrote:
> Something’s weird. The actual .h has #if defined(__GLIBC__) here. If this
really
> was supposed to be #if 1, the #else would be kind of pointless.

sorry, my bad, I updated this line instead of the previous one.

[Mark Mentovai, 2013-12-20 20:01:45]

LGTM

[Sergey Ulanov, 2013-12-20 21:05:41]

Committed patchset #7 manually as r242173 (presubmit successful).