Introduce skia_pic gyp variable.

Introduce skia_pic gyp variable.

skia_pic tells ninja to use -fPIC when building for position
independent code.

Set skia_pic to true when building our existing targets that
require position independent code.

Also use skia_pic when building for Android.

Committed: https://skia.googlesource.com/skia/+/c0bc9134514a4f138621203a6f7d4553ebec238a

[djsollen, 2014-07-18 19:02:25]

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:184: 'or skia_sanitizer == "undefined" or skia_os ==
"android"', {
does the sanitizer stuff need to be guarded by...
'skia_os in ["linux", "freebsd", "openbsd", "solaris", "nacl", "chromeos"

[scroggo, 2014-07-18 19:06:44]

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:184: 'or skia_sanitizer == "undefined" or skia_os ==
"android"', {
On 2014/07/18 19:02:25, djsollen wrote:
> does the sanitizer stuff need to be guarded by...
> 'skia_os in ["linux", "freebsd", "openbsd", "solaris", "nacl", "chromeos"

Good question. I'm guessing we don't ever build the sanitizer unless skia_os is
one of those.

If we add all those checks, might it be cleaner to leave them alone (e.g.
continue to set -fPIC for them inside their own conditions blocks and ignore
them here)?

[mtklein, 2014-07-19 14:49:20]

lgtm as is, with some options

https://codereview.chromium.org/406613003/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_conditions.gypi#n...
gyp/common_conditions.gypi:455: [ '_type == "executable"', {
Can we make skia_pic imply PIE executables too, or is that somehow now required
for Android?

I guess it's moot if we're always deploying as shared_libraries on our Android
bots?

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:184: 'or skia_sanitizer == "undefined" or skia_os ==
"android"', {
On 2014/07/18 19:06:43, scroggo wrote:
> On 2014/07/18 19:02:25, djsollen wrote:
> > does the sanitizer stuff need to be guarded by...
> > 'skia_os in ["linux", "freebsd", "openbsd", "solaris", "nacl", "chromeos"
> 
> Good question. I'm guessing we don't ever build the sanitizer unless skia_os
is
> one of those.

Right.  No need to guard.  It won't ever be set on OS/toolchains that don't
support it.  For what it's worth, some xSANs work on Linux and Mac now, and will
work on Windows once we get clang there.

> If we add all those checks, might it be cleaner to leave them alone (e.g.
> continue to set -fPIC for them inside their own conditions blocks and ignore
> them here)?

If it makes things simpler to think about, I think it's safe to simplify this
condition to 'skia_shared_lib or skia_sanitizer or skia_os == "android"'.  That
is, I think position-independent code should be fine for any sanitizer mode;
it's just _required_ for TSAN and UBSAN.  Right now we only additionally use
ASAN, and I don't think it'll mind being PIC.

As far as testing on our bots goes, I'd be willing to go all the way to making
-fPIC the default in all Skia gyps.  The only place we really want to turn it
off is for things like linking into Chrome, to save space and run a smidge
faster.

[djsollen, 2014-07-22 14:20:18]

lgtm

https://codereview.chromium.org/406613003/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_conditions.gypi#n...
gyp/common_conditions.gypi:455: [ '_type == "executable"', {
On 2014/07/19 14:49:19, mtklein wrote:
> Can we make skia_pic imply PIE executables too, or is that somehow now
required
> for Android?
> 
> I guess it's moot if we're always deploying as shared_libraries on our Android
> bots?

We need PIE for android executables as well.  The only android executable is
currently skia_launcher and it explicitly sets that in the gyp.

[scroggo, 2014-07-22 14:53:36]

https://codereview.chromium.org/406613003/diff/1/gyp/common_conditions.gypi
File gyp/common_conditions.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_conditions.gypi#n...
gyp/common_conditions.gypi:455: [ '_type == "executable"', {
On 2014/07/22 14:20:17, djsollen wrote:
> On 2014/07/19 14:49:19, mtklein wrote:
> > Can we make skia_pic imply PIE executables too, or is that somehow now
> required
> > for Android?
> > 
> > I guess it's moot if we're always deploying as shared_libraries on our
Android
> > bots?
> 
> We need PIE for android executables as well.  The only android executable is
> currently skia_launcher and it explicitly sets that in the gyp.

(Derek and I discussed in person, but for posterity:)
Moving this into skia_pic breaks Android. Our Android build currently does
something sneaky: our tools (e.g. SampleApp) use the type 'executable', but then
we change its type to 'shared_library' after the fact (see
platform_tools/android/gyp/dependencies.gyp). Then skia_launcher includes that
shared library. When I run android_ninja, SampleApp is considered an executable
(I'm guessing gyp reaches here before reaching the gyp file to change the type),
so it complains that executable is lacking a function main.

I can think of a few solutions:
- Don't define skia_pic for Android (downside here is that if someone *does*
  define it locally, they won't build)
- Don't include this as part of skia_pic (reduces code sharing)
- Change the way the executables get turned into shared libraries on Android
  (the current approach has a comment that it is designed to increase code
  sharing, but perhaps we can come up with another solution that does not
  reduce it).
- Skip the skia_launcher, and actually build the executables as executables
  (I'm sure there is a good reason for using skia_launcher though)
- Skip this part on Android (Derek actually proposed this, and it's probably
  the simplest approach.)

Thoughts?

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.chromium.org/406613003/diff/1/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:184: 'or skia_sanitizer == "undefined" or skia_os ==
"android"', {
On 2014/07/19 14:49:19, mtklein wrote:
> On 2014/07/18 19:06:43, scroggo wrote:
> > On 2014/07/18 19:02:25, djsollen wrote:
> > > does the sanitizer stuff need to be guarded by...
> > > 'skia_os in ["linux", "freebsd", "openbsd", "solaris", "nacl", "chromeos"
> > 
> > Good question. I'm guessing we don't ever build the sanitizer unless skia_os
> is
> > one of those.
> 
> Right.  No need to guard.  It won't ever be set on OS/toolchains that don't
> support it.  For what it's worth, some xSANs work on Linux and Mac now, and
will
> work on Windows once we get clang there.
> 
> > If we add all those checks, might it be cleaner to leave them alone (e.g.
> > continue to set -fPIC for them inside their own conditions blocks and ignore
> > them here)?
> 
> If it makes things simpler to think about, I think it's safe to simplify this
> condition to 'skia_shared_lib or skia_sanitizer or skia_os == "android"'. 
That
> is, I think position-independent code should be fine for any sanitizer mode;
> it's just _required_ for TSAN and UBSAN.  Right now we only additionally use
> ASAN, and I don't think it'll mind being PIC.

Done.

> 
> As far as testing on our bots goes, I'd be willing to go all the way to making
> -fPIC the default in all Skia gyps.  The only place we really want to turn it
> off is for things like linking into Chrome, to save space and run a smidge
> faster.

It seems like there are two ways to do this:
- default to skia_pic, but turn it off in chrome (or does chrome ignore
  our common variables? then it just wouldn't be set)
- add it for all of our targets

[mtklein, 2014-07-22 15:54:54]

> - Skip the skia_launcher, and actually build the executables as executables
>   (I'm sure there is a good reason for using skia_launcher though)

This is what I'd try first.  I think we used to have to do this roundabout
skia_launcher thing before we learned you can just run things directly out of
/data/local/tmp.  I believe it's now technically unnecessary.  I think we keep
it around now just to reduce time pushing over the USB cable?

> It seems like there are two ways to do this:
> - default to skia_pic, but turn it off in chrome (or does chrome ignore
>   our common variables? then it just wouldn't be set)
> - add it for all of our targets

Don't think any of our common_*.gyp is used by Chrome.

[mtklein, 2014-07-22 15:55:28]

(Still LGTM)

[scroggo, 2014-07-22 18:51:17]

On 2014/07/22 15:54:54, mtklein wrote:
> > - Skip the skia_launcher, and actually build the executables as executables
> >   (I'm sure there is a good reason for using skia_launcher though)
> 
> This is what I'd try first.  I think we used to have to do this roundabout
> skia_launcher thing before we learned you can just run things directly out of
> /data/local/tmp.  I believe it's now technically unnecessary.  I think we keep
> it around now just to reduce time pushing over the USB cable?

Derek, what do you think about this? I think that'll be a bigger change, so I'll
just not add those flags on Android with a FIXME.

> 
> > It seems like there are two ways to do this:
> > - default to skia_pic, but turn it off in chrome (or does chrome ignore
> >   our common variables? then it just wouldn't be set)
> > - add it for all of our targets
> 
> Don't think any of our common_*.gyp is used by Chrome.

Just to be safe, I'll leave that to a separate change.

[djsollen, 2014-07-22 18:53:50]

On 2014/07/22 18:51:17, scroggo wrote:
> On 2014/07/22 15:54:54, mtklein wrote:
> > > - Skip the skia_launcher, and actually build the executables as
executables
> > >   (I'm sure there is a good reason for using skia_launcher though)
> > 
> > This is what I'd try first.  I think we used to have to do this roundabout
> > skia_launcher thing before we learned you can just run things directly out
of
> > /data/local/tmp.  I believe it's now technically unnecessary.  I think we
keep
> > it around now just to reduce time pushing over the USB cable?
> 
> Derek, what do you think about this? I think that'll be a bigger change, so
I'll
> just not add those flags on Android with a FIXME.
> 
> > 
> > > It seems like there are two ways to do this:
> > > - default to skia_pic, but turn it off in chrome (or does chrome ignore
> > >   our common variables? then it just wouldn't be set)
> > > - add it for all of our targets
> > 
> > Don't think any of our common_*.gyp is used by Chrome.
> 
> Just to be safe, I'll leave that to a separate change.

I'm not opposed to moving away from skia_launcher, but we'll need to update all
of our tools.

[scroggo, 2014-07-22 18:58:11]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-07-22 18:58:19]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/scroggo@google.com/406613003/40001

[commit-bot: I haz the power, 2014-07-22 19:09:41]

Change committed as c0bc9134514a4f138621203a6f7d4553ebec238a