Make NaCl IPC-based PPAPI proxy build on ARM.

Make NaCl IPC-based PPAPI proxy build on ARM.

This change modifies the build to fix compile and link failures. It introduces a build/common_untrusted.gypi file, to add things that are needed by all untrusted targets that build Chrome code.

BUG=116317
TEST=compiles native_client.gyp:nacl_ipc_irt target on ARM.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=153074

[bbudge, 2012-08-23 01:58:07]

This doesn't yet work for all of the untrusted targets. Publishing to avoid
duplicate work.

[bbudge, 2012-08-23 03:25:16]

This now is enough to build all of our untrusted targets. PTAL

[brettw, 2012-08-23 03:55:15]

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
build/common_untrusted.gypi:20: # Disable C++ 11 extensions. Chrome's OVERRIDE
macro will generate
I'm surprised about this. Are there a bunch of OVERRIDES that are broken? I
would not expect there to be non-real errors with OVERRIDE. If there are a few
cases because of our ifdefs, can we fix them instead of doing this?

[bbudge, 2012-08-23 12:29:30]

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
build/common_untrusted.gypi:20: # Disable C++ 11 extensions. Chrome's OVERRIDE
macro will generate
On 2012/08/23 03:55:16, brettw wrote:
> I'm surprised about this. Are there a bunch of OVERRIDES that are broken? I
> would not expect there to be non-real errors with OVERRIDE. If there are a few
> cases because of our ifdefs, can we fix them instead of doing this?

There are a bunch (> 1100) of them, coming from base and gpu/command_buffer in
the builds I've seen. I will talk to the PNaCl guys, as this should be their
compiler.

 In file included from at_exit.cc:5:
 In file included from ../base/at_exit.h:12:
 In file included from ../base/callback.h:14:
 In file included from ../base/callback_internal.h:14:
 In file included from ../base/memory/ref_counted.h:12:
 ../base/threading/thread_collision_warner.h:141:23: warning:
'override' keyword is a C++11 extension [-Wc++11-extensions]
   virtual void warn() OVERRIDE;
                       ^
 ../base/compiler_specific.h:135:18: note: expanded
from macro 'OVERRIDE'
 #define OVERRIDE override

[bbudge, 2012-08-23 17:18:31]

On 2012/08/23 12:29:30, bbudge1 wrote:
> http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi
> File build/common_untrusted.gypi (right):
> 
>
http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
> build/common_untrusted.gypi:20: # Disable C++ 11 extensions. Chrome's OVERRIDE
> macro will generate
> On 2012/08/23 03:55:16, brettw wrote:
> > I'm surprised about this. Are there a bunch of OVERRIDES that are broken? I
> > would not expect there to be non-real errors with OVERRIDE. If there are a
few
> > cases because of our ifdefs, can we fix them instead of doing this?
> 
> There are a bunch (> 1100) of them, coming from base and gpu/command_buffer in
> the builds I've seen. I will talk to the PNaCl guys, as this should be their
> compiler.
> 
>  In file included from at_exit.cc:5:
>  In file included from ../base/at_exit.h:12:
>  In file included from ../base/callback.h:14:
>  In file included from ../base/callback_internal.h:14:
>  In file included from ../base/memory/ref_counted.h:12:
>  ../base/threading/thread_collision_warner.h:141:23: warning:
> 'override' keyword is a C++11 extension [-Wc++11-extensions]
>    virtual void warn() OVERRIDE;
>                        ^
>  ../base/compiler_specific.h:135:18: note: expanded
> from macro 'OVERRIDE'
>  #define OVERRIDE override

Looks like this is a known issue:
http://code.google.com/p/chromium-os/issues/detail?id=32016

[dmichael (off chromium), 2012-08-23 17:46:32]

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
build/common_untrusted.gypi:20: # Disable C++ 11 extensions. Chrome's OVERRIDE
macro will generate
On 2012/08/23 12:29:30, bbudge1 wrote:
> On 2012/08/23 03:55:16, brettw wrote:
> > I'm surprised about this. Are there a bunch of OVERRIDES that are broken? I
> > would not expect there to be non-real errors with OVERRIDE. If there are a
few
> > cases because of our ifdefs, can we fix them instead of doing this?
> 
> There are a bunch (> 1100) of them, coming from base and gpu/command_buffer in
> the builds I've seen. I will talk to the PNaCl guys, as this should be their
> compiler.
> 
>  In file included from at_exit.cc:5:
>  In file included from ../base/at_exit.h:12:
>  In file included from ../base/callback.h:14:
>  In file included from ../base/callback_internal.h:14:
>  In file included from ../base/memory/ref_counted.h:12:
>  ../base/threading/thread_collision_warner.h:141:23: warning:
> 'override' keyword is a C++11 extension [-Wc++11-extensions]
>    virtual void warn() OVERRIDE;
>                        ^
>  ../base/compiler_specific.h:135:18: note: expanded
> from macro 'OVERRIDE'
>  #define OVERRIDE override
Is this relevant?
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/EUXSi0QvXnc/dis...
It sounds like maybe you need tu set the "clang" gyp variable to 1 for your arm
targets.

[dmichael (off chromium), 2012-08-23 17:49:45]

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
build/common_untrusted.gypi:5: # This GYP file should be included in every NaCl
build of Chrome code.
NaCl is maybe a little ambiguous here, and "target" might be a better word than
"build".

Maybe more like...  "This GYP file should be included for every target in
Chromium that is built using the NaCl toolchain."? Or is that too wordy :)

[DaleCurtis, 2012-08-23 19:16:40]

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp
File base/base_untrusted.gyp (right):

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp#new...
base/base_untrusted.gyp:36: '__arm__',
Since my patch to switch this out for __ARMEL__ failed for the same reason as
without this, I suspect __ARMEL__ isn't defined either.  So you might consider
adding __ARMEL__ too for everything in base that depends on build/build_config.h
(lots).

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=cros_tegra...

http://codereview.chromium.org/10871009/diff/4005/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/4005/build/common_untrusted.gypi...
build/common_untrusted.gypi:15: 'clang': 1,
Slightly off topic: but I'd heard the clang arm cross compiler was behind the
x86 version, particularly around optimizations.

[bbudge, 2012-08-23 20:32:29]

Waiting for cros_tegra2 try run. All comments addressed.

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
build/common_untrusted.gypi:5: # This GYP file should be included in every NaCl
build of Chrome code.
On 2012/08/23 17:49:45, dmichael wrote:
> NaCl is maybe a little ambiguous here, and "target" might be a better word
than
> "build".
> 
> Maybe more like...  "This GYP file should be included for every target in
> Chromium that is built using the NaCl toolchain."? Or is that too wordy :)

Done. Thanks for the excellent comment.

http://codereview.chromium.org/10871009/diff/9001/build/common_untrusted.gypi...
build/common_untrusted.gypi:20: # Disable C++ 11 extensions. Chrome's OVERRIDE
macro will generate
On 2012/08/23 17:46:32, dmichael wrote:
> On 2012/08/23 12:29:30, bbudge1 wrote:
> > On 2012/08/23 03:55:16, brettw wrote:
> > > I'm surprised about this. Are there a bunch of OVERRIDES that are broken?
I
> > > would not expect there to be non-real errors with OVERRIDE. If there are a
> few
> > > cases because of our ifdefs, can we fix them instead of doing this?
> > 
> > There are a bunch (> 1100) of them, coming from base and gpu/command_buffer
in
> > the builds I've seen. I will talk to the PNaCl guys, as this should be their
> > compiler.
> > 
> >  In file included from at_exit.cc:5:
> >  In file included from ../base/at_exit.h:12:
> >  In file included from ../base/callback.h:14:
> >  In file included from ../base/callback_internal.h:14:
> >  In file included from ../base/memory/ref_counted.h:12:
> >  ../base/threading/thread_collision_warner.h:141:23:
warning:
> > 'override' keyword is a C++11 extension [-Wc++11-extensions]
> >    virtual void warn() OVERRIDE;
> >                        ^
> >  ../base/compiler_specific.h:135:18: note: expanded
> > from macro 'OVERRIDE'
> >  #define OVERRIDE override
> Is this relevant?
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/EUXSi0QvXnc/dis...
> It sounds like maybe you need tu set the "clang" gyp variable to 1 for your
arm
> targets.

Done. Yep, that works.

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp
File base/base_untrusted.gyp (right):

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp#new...
base/base_untrusted.gyp:36: '__arm__',
On 2012/08/23 19:16:40, DaleCurtis wrote:
> Since my patch to switch this out for __ARMEL__ failed for the same reason as
> without this, I suspect __ARMEL__ isn't defined either.  So you might consider
> adding __ARMEL__ too for everything in base that depends on
build/build_config.h
> (lots).
> 
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=cros_tegra...

Done. I'm adding it to common_untrusted.gypi so it's defined for every target.

http://codereview.chromium.org/10871009/diff/4005/build/common_untrusted.gypi
File build/common_untrusted.gypi (right):

http://codereview.chromium.org/10871009/diff/4005/build/common_untrusted.gypi...
build/common_untrusted.gypi:15: 'clang': 1,
On 2012/08/23 19:16:40, DaleCurtis wrote:
> Slightly off topic: but I'd heard the clang arm cross compiler was behind the
> x86 version, particularly around optimizations.

My understanding is that Clang is the only ARM compiler for NaCl.

[DaleCurtis, 2012-08-23 20:38:58]

lgtm % q.

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp
File base/base_untrusted.gyp (right):

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp#new...
base/base_untrusted.gyp:36: '__arm__',
On 2012/08/23 20:32:30, bbudge1 wrote:
> On 2012/08/23 19:16:40, DaleCurtis wrote:
> > Since my patch to switch this out for __ARMEL__ failed for the same reason
as
> > without this, I suspect __ARMEL__ isn't defined either.  So you might
consider
> > adding __ARMEL__ too for everything in base that depends on
> build/build_config.h
> > (lots).
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=cros_tegra...
> 
> Done. I'm adding it to common_untrusted.gypi so it's defined for every target.

It looks like there are #if checks inside the NaCl code base that use __arm__
too, should this also be in common?

https://cs.corp.google.com/#search/&q=package:%5Echrome$%20__arm__%20native_c...

[dmichael (off chromium), 2012-08-23 20:40:55]

lgtm

[brettw, 2012-08-23 20:55:07]

LGTM.

Glad you found a solution to the override problem. Although the reason I was
concerned is that I thought it was saying that we were getting override
warnings, rather than warnings about using override (which is less concerning to
me).

[bbudge, 2012-08-23 21:16:37]

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp
File base/base_untrusted.gyp (right):

http://codereview.chromium.org/10871009/diff/4005/base/base_untrusted.gyp#new...
base/base_untrusted.gyp:36: '__arm__',
On 2012/08/23 20:38:58, DaleCurtis wrote:
> On 2012/08/23 20:32:30, bbudge1 wrote:
> > On 2012/08/23 19:16:40, DaleCurtis wrote:
> > > Since my patch to switch this out for __ARMEL__ failed for the same reason
> as
> > > without this, I suspect __ARMEL__ isn't defined either.  So you might
> consider
> > > adding __ARMEL__ too for everything in base that depends on
> > build/build_config.h
> > > (lots).
> > > 
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=cros_tegra...
> > 
> > Done. I'm adding it to common_untrusted.gypi so it's defined for every
target.
> 
> It looks like there are #if checks inside the NaCl code base that use __arm__
> too, should this also be in common?
> 
>
https://cs.corp.google.com/#search/&q=package:%255Echrome%24%2520__arm__%2520...

I do see one untrusted (header) file that uses it, so I suppose we should. Done.