Refactor incompatible pointer type to a config

Refactor incompatible pointer type to a config

It makes it easier to fix the logic in a single place,
for example when porting a new cross compiler.

BUG=None
TEST=gn gen generates the same ninja files.
TBR=brettw@chromium.org

Committed: https://crrev.com/d68ffb8b4bf017ec49d2a6fbb46b9e46299c4f16
Cr-Commit-Position: refs/heads/master@{#339257}

[Peter Mayo, 2015-07-13 18:10:30]

petermayo@chromium.org changed reviewers:
+ dpranke@chromium.org

[Peter Mayo, 2015-07-13 18:10:30]

git cl upload suggested brettw, not sure which of you wants to grab this.

[Dirk Pranke, 2015-07-13 23:08:04]

https://codereview.chromium.org/1233913002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1233913002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:1068: # Some code presumes that pointers to
structures/objects are compatable
typo: "compatible".

https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.gn
File third_party/usrsctp/BUILD.gn (left):

https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.g...
third_party/usrsctp/BUILD.gn:130: cflags = [ "-w" ]
Do we need this block somewhere in the generic config?

[Peter Mayo, 2015-07-14 14:10:31]

https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.gn
File third_party/usrsctp/BUILD.gn (left):

https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.g...
third_party/usrsctp/BUILD.gn:130: cflags = [ "-w" ]
On 2015/07/13 23:08:04, Dirk Pranke wrote:
> Do we need this block somewhere in the generic config?

I hope not.

It seems pretty terrible to turn all warnings off in general, and the comment
leads me to believe it is only necessary because of the incompatible pointers. 
For the time being this change doesn't increase the chance of rot, since it will
still get the flag from the config.  Any changes that tighten up that change
will expose any rot in this config similarly to fixing it here.

[Peter Mayo, 2015-07-14 14:10:56]

Fix typo

[Peter Mayo, 2015-07-14 14:14:28]

https://codereview.chromium.org/1233913002/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1233913002/diff/1/build/config/compiler/BUILD...
build/config/compiler/BUILD.gn:1068: # Some code presumes that pointers to
structures/objects are compatable
On 2015/07/13 23:08:04, Dirk Pranke wrote:
> typo: "compatible".

Done.

[Dirk Pranke, 2015-07-14 15:54:14]

https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.gn
File third_party/usrsctp/BUILD.gn (left):

https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.g...
third_party/usrsctp/BUILD.gn:130: cflags = [ "-w" ]
On 2015/07/14 14:10:30, Peter Mayo wrote:
> On 2015/07/13 23:08:04, Dirk Pranke wrote:
> > Do we need this block somewhere in the generic config?
> 
> I hope not. 

I think I phrased this badly; my concern was actually that  I didn't see
anything that would offset this deletion, so won't we this cause a problem on a
non-CrOS mipsel build after this is deleted?

[Peter Mayo, 2015-07-14 16:03:14]

On 2015/07/14 15:54:14, Dirk Pranke wrote:
> https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.gn
> File third_party/usrsctp/BUILD.gn (left):
> 
>
https://codereview.chromium.org/1233913002/diff/1/third_party/usrsctp/BUILD.g...
> third_party/usrsctp/BUILD.gn:130: cflags = [ "-w" ]
> On 2015/07/14 14:10:30, Peter Mayo wrote:
> > On 2015/07/13 23:08:04, Dirk Pranke wrote:
> > > Do we need this block somewhere in the generic config?
> > 
> > I hope not. 
> 
> I think I phrased this badly; my concern was actually that  I didn't see
> anything that would offset this deletion, so won't we this cause a problem on
a
> non-CrOS mipsel build after this is deleted?

Ah - since we are no longer adding the flag elsewhere except when is_clang my
expectation is that we don't need to override it any more.  It will open this
module to other warnings on that platform.  That could be a problem, but it
would be an accurate one.

How do I test mipsel?  I don't see a (gn|chromium) mipsel trybot.

I presume I have to get a toolchain somewhere.

[Dirk Pranke, 2015-07-14 16:05:24]

> How do I test mipsel?  I don't see a (gn|chromium) mipsel trybot.

I'm not sure, the main person I've seen testing mipsel is sbc@;

[Dirk Pranke, 2015-07-14 16:05:38]

dpranke@chromium.org changed reviewers:
+ sbc@chromium.org

[Dirk Pranke, 2015-07-14 16:06:01]

+sbc ... Sam, what's the right change here for the mips port?

[Sam Clegg, 2015-07-15 17:40:10]

On 2015/07/14 16:06:01, Dirk Pranke wrote:
> +sbc ... Sam, what's the right change here for the mips port?

I think there is a mipsel chromeos trybot these days isn't there?  I would run
that and see if it passes.

Also, does this change imply that other gcc (non-clang) builds are using gcc
5.0?  (or are we using clang pretty much across the board these days?)

Wouldn't and explicit gcc version check be better here?  Or is that not
something that is possible in gn right now?

[Sam Clegg, 2015-07-15 17:40:20]

https://codereview.chromium.org/1233913002/diff/20001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1233913002/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1076: #cflags -= [ "-Werror", "-Wall" ]
Remove this comment?

https://codereview.chromium.org/1233913002/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1078: } else if (is_clang) {
Wouldn't it make sense for check for is_clang condition first, in case chromeos
switch to using clang for their arm build?

[Peter Mayo, 2015-07-15 18:04:36]

On 2015/07/15 17:40:10, Sam Clegg wrote:
> On 2015/07/14 16:06:01, Dirk Pranke wrote:
> I think there is a mipsel chromeos trybot these days isn't there?  I would run
> that and see if it passes.

ChromeOS uses chromeos-base/chromeos-chrome ebuild which uses gyp, so this
should affect anything there yet.

I will try to get one of the mipsel boards building in the future workflow to
test this.

> 
> Also, does this change imply that other gcc (non-clang) builds are using gcc
> 5.0?  (or are we using clang pretty much across the board these days?)

No, the intent of this is a refactor to unify the spread out logic.  So that we
can get it right for all of the chromeos situations, so we can convert the
ebuild mentioned above (and the dev workflows) to use the gn path.

> 
> Wouldn't an explicit gcc version check be better here?  Or is that not
> something that is possible in gn right now?

I'll leave that for someone else to think about.  I'm after consistent logic
between modules for this change.

[Peter Mayo, 2015-07-15 18:08:55]

Address feedback

[Peter Mayo, 2015-07-15 18:09:38]

https://codereview.chromium.org/1233913002/diff/20001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1233913002/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1076: #cflags -= [ "-Werror", "-Wall" ]
On 2015/07/15 17:40:20, Sam Clegg wrote:
> Remove this comment?

Done.

https://codereview.chromium.org/1233913002/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1078: } else if (is_clang) {
On 2015/07/15 17:40:20, Sam Clegg wrote:
> Wouldn't it make sense for check for is_clang condition first, in case
chromeos
> switch to using clang for their arm build?

Done.

[Sam Clegg, 2015-07-15 18:20:13]

On 2015/07/15 18:04:36, Peter Mayo wrote:
> On 2015/07/15 17:40:10, Sam Clegg wrote:
> > On 2015/07/14 16:06:01, Dirk Pranke wrote:
> > I think there is a mipsel chromeos trybot these days isn't there?  I would
run
> > that and see if it passes.
> 
> ChromeOS uses chromeos-base/chromeos-chrome ebuild which uses gyp, so this
> should affect anything there yet.
> 
> I will try to get one of the mipsel boards building in the future workflow to
> test this.
> 
> > 
> > Also, does this change imply that other gcc (non-clang) builds are using gcc
> > 5.0?  (or are we using clang pretty much across the board these days?)
> 
> No, the intent of this is a refactor to unify the spread out logic.  So that
we
> can get it right for all of the chromeos situations, so we can convert the
> ebuild mentioned above (and the dev workflows) to use the gn path.
> 
> > 
> > Wouldn't an explicit gcc version check be better here?  Or is that not
> > something that is possible in gn right now?
> 
> I'll leave that for someone else to think about.  I'm after consistent logic
> between modules for this change.

Ok then.  Since I've not done any cross compiling with the gn build I can't be
of much use here.

Why are you checking current_cpu == "arm" in your new code but the old code was
checking target_cpu == "mipsel"?   It seems quite different if you are only
going for a refactor?

[Peter Mayo, 2015-07-15 18:29:05]

Added mipsel clause back

[Peter Mayo, 2015-07-15 18:39:42]

> 
> Why are you checking current_cpu == "arm" in your new code but the old code
was
> checking target_cpu == "mipsel"?   It seems quite different if you are only
> going for a refactor?

Because I want that one to start working too, it doesn't today. As for the old
code, the confusing thing is that nspr, internal_gles2_conform_tests, and yasm
all use the problematic code, but don't currently have -w on mipsel.

I was hoping to have a way to validate that we are using gcc 5.0+ for our mipsel
forks.

[Sam Clegg, 2015-07-15 18:45:23]

On 2015/07/15 18:39:42, Peter Mayo wrote:
> > 
> > Why are you checking current_cpu == "arm" in your new code but the old code
> was
> > checking target_cpu == "mipsel"?   It seems quite different if you are only
> > going for a refactor?
> 
> Because I want that one to start working too, it doesn't today. As for the old
> code, the confusing thing is that nspr, internal_gles2_conform_tests, and yasm
> all use the problematic code, but don't currently have -w on mipsel.
> 
> I was hoping to have a way to validate that we are using gcc 5.0+ for our
mipsel
> forks.

I see, I was missing that piece of information.  You guys are using gcc 5.0 for
mipsel (and I guess x86?) but not arm?  That makes more sense to me now.  Is it
safe to assume that all non-chromeos non-clang builds are all using gcc < 5.0?

[Peter Mayo, 2015-07-15 19:01:45]

On 2015/07/15 18:45:23, Sam Clegg wrote:
> On 2015/07/15 18:39:42, Peter Mayo wrote:
> > > 
> > > Why are you checking current_cpu == "arm" in your new code but the old
code
> > was
> > > checking target_cpu == "mipsel"?   It seems quite different if you are
only
> > > going for a refactor?
> > 
> > Because I want that one to start working too, it doesn't today. As for the
old
> > code, the confusing thing is that nspr, internal_gles2_conform_tests, and
yasm
> > all use the problematic code, but don't currently have -w on mipsel.
> > 
> > I was hoping to have a way to validate that we are using gcc 5.0+ for our
> mipsel
> > forks.
> 
> I see, I was missing that piece of information.  You guys are using gcc 5.0
for
> mipsel (and I guess x86?) but not arm?  That makes more sense to me now.  Is
it
> safe to assume that all non-chromeos non-clang builds are all using gcc < 5.0?

Actually when I try to use any of the mipsel- boards for chromeos I get errors
finding the SDK.

I see the mipsel bots still running on chromiumos, but they are building libs
for gcc 4.9

In fact it looks like chromeos is gcc 4.9 for 'all' of them.

Remember though, none of them are using gn yet.  I picked an arm board to start
with, since that quickly lets you know when you got a host/target tool wrong.

[Sam Clegg, 2015-07-15 19:12:53]

On 2015/07/15 19:01:45, Peter Mayo wrote:
> On 2015/07/15 18:45:23, Sam Clegg wrote:
> > On 2015/07/15 18:39:42, Peter Mayo wrote:
> > > > 
> > > > Why are you checking current_cpu == "arm" in your new code but the old
> code
> > > was
> > > > checking target_cpu == "mipsel"?   It seems quite different if you are
> only
> > > > going for a refactor?
> > > 
> > > Because I want that one to start working too, it doesn't today. As for the
> old
> > > code, the confusing thing is that nspr, internal_gles2_conform_tests, and
> yasm
> > > all use the problematic code, but don't currently have -w on mipsel.
> > > 
> > > I was hoping to have a way to validate that we are using gcc 5.0+ for our
> > mipsel
> > > forks.
> > 
> > I see, I was missing that piece of information.  You guys are using gcc 5.0
> for
> > mipsel (and I guess x86?) but not arm?  That makes more sense to me now.  Is
> it
> > safe to assume that all non-chromeos non-clang builds are all using gcc <
5.0?
> 
> Actually when I try to use any of the mipsel- boards for chromeos I get errors
> finding the SDK.
> 
> I see the mipsel bots still running on chromiumos, but they are building libs
> for gcc 4.9
> 
> In fact it looks like chromeos is gcc 4.9 for 'all' of them.
> 
> Remember though, none of them are using gn yet.  I picked an arm board to
start
> with, since that quickly lets you know when you got a host/target tool wrong.

OK, perhaps its best to just assume < 5.0 for now?   I guess is fairly academic
as you say since none of this is being used by any active bots at this point.

I'm fine with this change but someone else should LG because I really don't know
much gn.

[Dirk Pranke, 2015-07-15 19:37:42]

lgtm

[Peter Mayo, 2015-07-15 20:32:29]

Patchset #5 (id:80001) has been deleted

[Peter Mayo, 2015-07-15 21:46:54]

The CQ bit was checked by petermayo@chromium.org

[commit-bot: I haz the power, 2015-07-15 21:47:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233913002/60001

[commit-bot: I haz the power, 2015-07-15 21:59:29]

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

[commit-bot: I haz the power, 2015-07-15 21:59:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Mayo, 2015-07-16 19:59:49]

petermayo@chromium.org changed reviewers:
+ brettw@chromium.org

[Peter Mayo, 2015-07-16 20:00:32]

Brett,

Are you comfortable if I TBR BUILD.gn changes reviewed by Dirk?

[Peter Mayo, 2015-07-17 15:03:32]

The CQ bit was checked by petermayo@chromium.org

[commit-bot: I haz the power, 2015-07-17 15:03:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233913002/60001

[commit-bot: I haz the power, 2015-07-17 15:35:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-17 15:36:26]

Patchset 4 (id:??) landed as
https://crrev.com/d68ffb8b4bf017ec49d2a6fbb46b9e46299c4f16
Cr-Commit-Position: refs/heads/master@{#339257}

[brettw, 2015-07-20 17:03:57]

This was fine. LGTM