[MIPS] Support for MIPS PNaCl in PPAPI

[MIPS] Support for MIPS PNaCl in PPAPI

Add necessary definitions and build blocks for MIPS.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= Build Chromium for MIPS

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

[petarj, 2014-02-05 23:13:42]

PTAL.

[dmichael (off chromium), 2014-02-05 23:55:22]

+jvoung, who is far more familiar with the PNaCl parts.

We can probably take a patch for this, but I'm sure you're already aware that if
we don't have bots building and running tests for MIPS, it's likely to break.

Also, you need to follow our process for external contributions, if you haven't
already:
http://dev.chromium.org/developers/contributing-code/external-contributor-che...

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:227: kNaClMacArm,
I believe these enum values have to be stable, so I think Mips has to go at the
end, just before kNaClOSArchMax.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:248: os_arch =
static_cast<NaClOSArch>(os_arch + 3);
If mips only makes sense on linux, maybe you should just set it explicitly to
kNaClLinuxMips here.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.gypi (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.gypi:35: ['target_arch=="mipsel"',
{
So am I correct in understanding that you're only adding support for
little-endian MIPS? That seems appropriate; I think a lot of things would be
broken in a big-endian architecture.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py:194:
return J(PnaclDirs.TranslatorRoot(), 'lib-mips32')
I'm not very familiar with this code, but...  why can't the name of the
directory be made to match so you don't need this special case?

[petarj, 2014-02-06 00:38:48]

> Also, you need to follow our process for external contributions, if you
haven't
> already:

Done that a few years ago. Check the AUTHORS file.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:227: kNaClMacArm,
On 2014/02/05 23:55:22, dmichael wrote:
> I believe these enum values have to be stable, so I think Mips has to go at
the
> end, just before kNaClOSArchMax.

Done.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:248: os_arch =
static_cast<NaClOSArch>(os_arch + 3);
On 2014/02/05 23:55:22, dmichael wrote:
> If mips only makes sense on linux, maybe you should just set it explicitly to
> kNaClLinuxMips here.

Done.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.gypi (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.gypi:35: ['target_arch=="mipsel"',
{
On 2014/02/05 23:55:22, dmichael wrote:
> So am I correct in understanding that you're only adding support for
> little-endian MIPS? That seems appropriate; I think a lot of things would be
> broken in a big-endian architecture.

Correct.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py:194:
return J(PnaclDirs.TranslatorRoot(), 'lib-mips32')
On 2014/02/05 23:55:22, dmichael wrote:
> I'm not very familiar with this code, but...  why can't the name of the
> directory be made to match so you don't need this special case?

PNaCl uses mips32 to refer to 32bit MIPS LE. This is how the libraries end up in
lib-mips32.
We need a special case somewhere, having it here seemed least intrusive to the
current setup.

[jvoung (off chromium), 2014-02-06 00:39:55]

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:227: kNaClMacArm,
On 2014/02/05 23:55:22, dmichael wrote:
> I believe these enum values have to be stable, so I think Mips has to go at
the
> end, just before kNaClOSArchMax.

I think we need to update server to know of the new UMA value too

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:18:
'out_newlib_mips': '>(tc_lib_dir_pnacl_translate)/lib-mips/libpnacl_irt_shim.a',
lib-mips32 to be consistent w/ the other part? Doesn't matter much I guess...

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:43:
'--pnacl-allow-translate',
Currently have a CL to change this a bit. CC'ed you on part one of it.

What is the native ABI for the IRT? Is it based on clang's le32-none-nacl? Are
you also using the pnacl ABI stabilization passes like ExpandByVal when you
build the IRT?

If so, this shim might be a nop and you can use a dummy shim or change the
linker rules to skip the shim.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py:42:
return 'mips'
can you just make the canonical arch "mips32" put that in ARCHES=[...], then
will that fix the bit in the LibDir()?

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_support_extension.gyp
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_support_extension.gyp:96:
'<(PRODUCT_DIR)/pnacl/pnacl_public_mips_crtbeginS_o',
We no longer build a crtbeginS

https://codereview.chromium.org/152133007/diff/1/ppapi/ppapi_untrusted.gyp
File ppapi/ppapi_untrusted.gyp (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/ppapi_untrusted.gyp#ne...
ppapi/ppapi_untrusted.gyp:179: # or ARM on ARM platforms, not all versions
always.
TODO() to be able to build mips tests?

[petarj, 2014-02-20 00:23:17]

New patch has been uploaded. PTAL.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:18:
'out_newlib_mips': '>(tc_lib_dir_pnacl_translate)/lib-mips/libpnacl_irt_shim.a',
On 2014/02/06 00:39:56, jvoung (cr) wrote:
> lib-mips32 to be consistent w/ the other part? Doesn't matter much I guess...

This line is removed now.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:43:
'--pnacl-allow-translate',
On 2014/02/06 00:39:56, jvoung (cr) wrote:
> Currently have a CL to change this a bit. CC'ed you on part one of it.
> 
> What is the native ABI for the IRT? Is it based on clang's le32-none-nacl?

It's le32-unknown-nacl.

> Are
> you also using the pnacl ABI stabilization passes like ExpandByVal when you
> build the IRT?
>

Yes, '-expand-byval' is used.
 
> If so, this shim might be a nop and you can use a dummy shim or change the
> linker rules to skip the shim.
> 

You are right. We have just modified the code to use dummy shim.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_component_crx_gen.py:42:
return 'mips'
On 2014/02/06 00:39:56, jvoung (cr) wrote:
> can you just make the canonical arch "mips32" put that in ARCHES=[...], then
> will that fix the bit in the LibDir()?

Done.

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
File
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_support_extension.gyp
(right):

https://codereview.chromium.org/152133007/diff/1/ppapi/native_client/src/untr...
ppapi/native_client/src/untrusted/pnacl_support_extension/pnacl_support_extension.gyp:96:
'<(PRODUCT_DIR)/pnacl/pnacl_public_mips_crtbeginS_o',
On 2014/02/06 00:39:56, jvoung (cr) wrote:
> We no longer build a crtbeginS

Done.

https://codereview.chromium.org/152133007/diff/1/ppapi/ppapi_untrusted.gyp
File ppapi/ppapi_untrusted.gyp (right):

https://codereview.chromium.org/152133007/diff/1/ppapi/ppapi_untrusted.gyp#ne...
ppapi/ppapi_untrusted.gyp:179: # or ARM on ARM platforms, not all versions
always.
On 2014/02/06 00:39:56, jvoung (cr) wrote:
> TODO() to be able to build mips tests?

Done.

[jvoung (off chromium), 2014-02-20 02:22:12]

LGTM -- I'll add the UMA histogram entry.

[petarj, 2014-02-20 09:35:09]

The CQ bit was checked by petarj@mips.com

[commit-bot: I haz the power, 2014-02-20 12:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petarj@mips.com/152133007/350001

[petarj, 2014-02-20 13:15:03]

On 2014/02/20 12:44:39, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/petarj%40mips.com/152133007/350001

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ppapi/ppapi_untrusted.gyp

David, are you fine with this change set?

[commit-bot: I haz the power, 2014-02-20 13:21:05]

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

[commit-bot: I haz the power, 2014-02-20 13:21:05]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

Try jobs failed on following builders: linux_chromium_rel

[dmichael (off chromium), 2014-02-20 15:46:59]

ppapi lgtm

[dmichael (off chromium), 2014-02-20 15:47:04]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-02-20 15:47:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petarj@mips.com/152133007/350001

[commit-bot: I haz the power, 2014-02-20 15:47:58]

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

[commit-bot: I haz the power, 2014-02-20 15:48:00]

Failed to apply patch for ppapi/native_client/native_client.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/native_client/native_client.gyp
  Hunk #3 FAILED at 108.
  Hunk #6 FAILED at 312.
  2 out of 6 hunks FAILED -- saving rejects to file
ppapi/native_client/native_client.gyp.rej

Patch:       ppapi/native_client/native_client.gyp
Index: ppapi/native_client/native_client.gyp
diff --git a/ppapi/native_client/native_client.gyp
b/ppapi/native_client/native_client.gyp
index
ee83999a444fcca64e226c903458f6009f4ba188..41fb41b332ef999b870cf0351efbb531e12c0435
100644
--- a/ppapi/native_client/native_client.gyp
+++ b/ppapi/native_client/native_client.gyp
@@ -36,7 +36,7 @@
             },
           ],
           'conditions': [
-            ['target_arch!="arm"', {
+            ['target_arch=="ia32" or target_arch=="x64"', {
               'copies': [
                 {
                   'destination': '>(tc_include_dir_glibc)/include/nacl',
@@ -84,6 +84,16 @@
                   ],
                 },
               ]
+            }],
+            ['target_arch=="mipsel"', {
+              'copies': [
+                {
+                  'destination': '>(tc_lib_dir_newlib_mips)',
+                  'files': [
+                    'src/untrusted/irt_stub/libppapi.a',
+                  ],
+                },
+              ]
             }]
           ],
         },
@@ -98,6 +108,7 @@
             'out_newlib64':
'<(SHARED_INTERMEDIATE_DIR)/nacl_irt_x86_64_raw.nexe',
             'out_newlib32':
'<(SHARED_INTERMEDIATE_DIR)/nacl_irt_x86_32_raw.nexe',
             'out_newlib_arm':
'<(SHARED_INTERMEDIATE_DIR)/nacl_irt_arm_raw.nexe',
+            'out_newlib_mips':
'<(SHARED_INTERMEDIATE_DIR)/nacl_irt_mips32_raw.nexe',
             'build_glibc': 0,
             'build_newlib': 0,
             'build_irt': 1,
@@ -146,7 +157,7 @@
                   'enable_x86_32': 0
                 }
               ],
-              ['target_arch!="arm"', {
+              ['target_arch=="ia32" or target_arch=="x64"', {
                 'extra_deps_newlib64': [
                   '>(tc_lib_dir_irt64)/libppapi_proxy_untrusted.a',
                   '>(tc_lib_dir_irt64)/libppapi_ipc_untrusted.a',
@@ -210,6 +221,28 @@
                   '>(tc_lib_dir_irt_arm)/libgio.a',
                 ],
               }],
+              ['target_arch=="mipsel"', {
+                'extra_deps_mips': [
+                  '>(tc_lib_dir_irt_mips)/libppapi_proxy_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libppapi_ipc_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libppapi_shared_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libgles2_implementation_untrusted.a',
+                 
'>(tc_lib_dir_irt_mips)/libcommand_buffer_client_untrusted.a',
+                 
'>(tc_lib_dir_irt_mips)/libcommand_buffer_common_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libgpu_ipc_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libtracing_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libgles2_cmd_helper_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libgles2_utils_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libipc_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libbase_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libirt_browser.a',
+                 
'>(tc_lib_dir_irt_mips)/libshared_memory_support_untrusted.a',
+                  '>(tc_lib_dir_irt_mips)/libsrpc.a',
+                  '>(tc_lib_dir_irt_mips)/libplatform.a',
+                  '>(tc_lib_dir_irt_mips)/libimc_syscalls.a',
+                  '>(tc_lib_dir_irt_mips)/libgio.a',
+                ],
+              }],
             ],
           },
           'dependencies': [
@@ -280,6 +313,44 @@
                 },
               ],
             }],
+            ['target_arch=="mipsel"', {
+              'actions': [
+                {
+                  'action_name': 'tls_edit_nacl_irt_mips',
+                  'message': 'Patching TLS for nacl_irt (mips)',
+                  'inputs': [
+                    '<(PRODUCT_DIR)/tls_edit',
+                    '<(SHARED_INTERMEDIATE_DIR)/nacl_irt_mips32_raw.nexe',
+                  ],
+                  'outputs': [
+                    '<(PRODUCT_DIR)/nacl_irt_mips32.nexe',
+                  ],
+                  'action': ['<@(_inputs)', '<@(_outputs)'],
+                },
+
+                # The non-stripped nacl_irt debug file must also go through
+                # tls_edit, however gyp does not know anything about the
+                # debug file since it is built as a side effect. We
+                # must depend on the nacl_irt_raw.nexe and use the
+                # nacl_irt_raw.nexe.debug file as the input to tls_edit.
+                {
+                  'action_name': 'tls_edit_nacl_irt_debug_mips',
+                  'message': 'Patching TLS for nacl_irt.debug (mips)',
+                  'inputs': [
+                    '<(PRODUCT_DIR)/tls_edit',
+                    '<(SHARED_INTERMEDIATE_DIR)/nacl_irt_mips32_raw.nexe',
+                  ],
+                  'outputs': [
+                    '<(PRODUCT_DIR)/nacl_irt_mips32.nexe.debug',
+                  ],
+                  'action': [
+                    '<(PRODUCT_DIR)/tls_edit',
+                   
'<(SHARED_INTERMEDIATE_DIR)/nacl_irt_mips32_raw.nexe.debug',
+                    '<@(_outputs)',
+                  ],
+                },
+              ],
+            }],
             ['target_arch=="x64" or OS=="win"', {
               'actions': [
                 {

[petarj, 2014-02-20 15:57:15]

The CQ bit was checked by petarj@mips.com

[commit-bot: I haz the power, 2014-02-20 15:58:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petarj@mips.com/152133007/670003

[commit-bot: I haz the power, 2014-02-20 23:34:12]

Change committed as 252416

[Mark Seaborn, 2014-02-20 23:59:03]

https://codereview.chromium.org/152133007/diff/670003/ppapi/native_client/src...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp
(right):

https://codereview.chromium.org/152133007/diff/670003/ppapi/native_client/src...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:7:
['target_arch!="mipsel"', {
Why are you excluding the IRT shim on MIPS?  This could use a comment.

Currently the shim whitelists some IRT interfaces under PNaCl, and this
whitelisting should really be done for MIPS too.

[jvoung (off chromium), 2014-02-21 00:32:56]

On 2014/02/20 23:59:03, Mark Seaborn wrote:
>
https://codereview.chromium.org/152133007/diff/670003/ppapi/native_client/src...
> File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp
> (right):
> 
>
https://codereview.chromium.org/152133007/diff/670003/ppapi/native_client/src...
> ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:7:
> ['target_arch!="mipsel"', {
> Why are you excluding the IRT shim on MIPS?  This could use a comment.
> 
> Currently the shim whitelists some IRT interfaces under PNaCl, and this
> whitelisting should really be done for MIPS too.

Ah right, forgot about the whitelisting... hmm.

[petarj, 2014-02-21 19:17:38]

On 2014/02/21 00:32:56, jvoung (cr) wrote:
> On 2014/02/20 23:59:03, Mark Seaborn wrote:
> >
>
https://codereview.chromium.org/152133007/diff/670003/ppapi/native_client/src...
> > File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp
> > (right):
> > 
> >
>
https://codereview.chromium.org/152133007/diff/670003/ppapi/native_client/src...
> > ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_irt_shim.gyp:7:
> > ['target_arch!="mipsel"', {
> > Why are you excluding the IRT shim on MIPS?  This could use a comment.
> > 
> > Currently the shim whitelists some IRT interfaces under PNaCl, and this
> > whitelisting should really be done for MIPS too.
> 
> Ah right, forgot about the whitelisting... hmm.

I have modified the change to use IRT shim again.
Take a look at: https://codereview.chromium.org/175533002/.

Regards,
Petar