Make ARM64 detection consistent.

Make ARM64 detection consistent.

Use only __aarch64__ and don't look for __arm64__ at all.
It turns out that clang defines both and GCC only the former.
Hence, looking only for __aarch64__ should be enough.

BUG=354405, 358092
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260671

[Primiano Tucci (use gerrit), 2014-03-31 11:58:35]

Mike,
we're trying to build chrome_shell_apk for arm64 (not iOS) and there are some
inconsistencies with the way arm64 is detected.
IIUC, clang (which is used for iOS-64bit) defines both __arm64__ and
__aarch64__, while GCC defines only __aarch64__.
Do you feel this change is OK or would cause problems on iOS?

[pink (ping after 24hrs), 2014-03-31 12:02:19]

I'm not the best person to ask, but Sylvain has been working on the ARM64 stuff,
and I also added Stuart since this touches Omaha.

[sdefresne, 2014-03-31 12:31:19]

lgtm

https://codereview.chromium.org/218663003/diff/1/chrome/browser/omaha_query_p...
File chrome/browser/omaha_query_params/omaha_query_params.cc (left):

https://codereview.chromium.org/218663003/diff/1/chrome/browser/omaha_query_p...
chrome/browser/omaha_query_params/omaha_query_params.cc:44: "arm64";
You can change this value to "aarch64" too, since the value "arm64" has not yet
been used.

[Primiano Tucci (use gerrit), 2014-03-31 12:46:58]

Done, Thanks.

+thakis: Nico, would you give an OWNERS approval? Thanks.

https://codereview.chromium.org/218663003/diff/1/chrome/browser/omaha_query_p...
File chrome/browser/omaha_query_params/omaha_query_params.cc (left):

https://codereview.chromium.org/218663003/diff/1/chrome/browser/omaha_query_p...
chrome/browser/omaha_query_params/omaha_query_params.cc:44: "arm64";
On 2014/03/31 12:31:19, sdefresne wrote:
> You can change this value to "aarch64" too, since the value "arm64" has not
yet
> been used.

Done.

[stuartmorgan, 2014-03-31 13:26:01]

lgtm

https://codereview.chromium.org/218663003/diff/20001/chrome/browser/omaha_que...
File chrome/browser/omaha_query_params/omaha_query_params.cc (right):

https://codereview.chromium.org/218663003/diff/20001/chrome/browser/omaha_que...
chrome/browser/omaha_query_params/omaha_query_params.cc:44: "aarch64";
Is there any reason *not* to call this arm64? Seems odd to make something that's
just for dashboard consumption consistent with the define rather than the "arm"
string; arm64 seems more self-explanatory.

I'm fine either way though if there's a good reason for this.

[Primiano Tucci (use gerrit), 2014-03-31 14:33:47]

+asargent: apparently you wrote some code which consumes the arch here in
chrome/browser/extensions/api/runtime/runtime_api.cc
Do you think the arch string should be renamed as well or not?

It seems we're stuck on the value of a string here :) Any thoughts?

[Nico, 2014-03-31 15:42:37]

__arm64__ is apple's arm64 abi, and __aarch64__ is the "official" 64 bit abi,
no? Are you sure clang defines __aarch64__ when doing 64 bit ios builds?

[Nico, 2014-03-31 16:13:42]

https://codereview.chromium.org/218663003/diff/20001/chrome/browser/omaha_que...
File chrome/browser/omaha_query_params/omaha_query_params.cc (right):

https://codereview.chromium.org/218663003/diff/20001/chrome/browser/omaha_que...
chrome/browser/omaha_query_params/omaha_query_params.cc:44: "aarch64";
On 2014/03/31 13:26:01, stuartmorgan wrote:
> Is there any reason *not* to call this arm64? Seems odd to make something
that's
> just for dashboard consumption consistent with the define rather than the
"arm"
> string; arm64 seems more self-explanatory.
> 
> I'm fine either way though if there's a good reason for this.

(I agree with this. The gyp target_arch value is also called arm64.)

[Primiano Tucci (use gerrit), 2014-03-31 16:19:58]

On 2014/03/31 15:42:37, Nico wrote:
> __arm64__ is apple's arm64 abi, and __aarch64__ is the "official" 64 bit abi,
> no? Are you sure clang defines __aarch64__ when doing 64 bit ios builds?

To be honest I don't have any iOS-related knowledge.
My understanding was that __aarch64__ was always defined and reflects the target
HW arch (opposite to __arm64__ which, as you suggested, reflects the abi).
I got confirmation of this in a former CL (https://crrev.com/216833004/) and
from my compiler spiritual guide (hans@) :)
I'd expect the trybots to fail miserably in the opposite case, shouldn't they?

BTW, I've just tested with XCode 5 and yes, __aarch64__ seems to be defined.

[Primiano Tucci (use gerrit), 2014-03-31 16:25:29]

https://codereview.chromium.org/218663003/diff/20001/chrome/browser/omaha_que...
File chrome/browser/omaha_query_params/omaha_query_params.cc (right):

https://codereview.chromium.org/218663003/diff/20001/chrome/browser/omaha_que...
chrome/browser/omaha_query_params/omaha_query_params.cc:44: "aarch64";
On 2014/03/31 16:13:43, Nico wrote:
> On 2014/03/31 13:26:01, stuartmorgan wrote:
> > Is there any reason *not* to call this arm64? Seems odd to make something
> that's
> > just for dashboard consumption consistent with the define rather than the
> "arm"
> > string; arm64 seems more self-explanatory.
> > 
> > I'm fine either way though if there's a good reason for this.
> 
> (I agree with this. The gyp target_arch value is also called arm64.)

Right, me and Stuart had a discussion offline and that (GYP) was  the same point
I had about keeping arm64.
However, it looks it's worth double-checking this since this string is
apparently sent to Omaha, and I don't know whether it would be fine having Linux
64 fall into the same category of iOS 64. (And for sake of clarity, changing
this string as it is in patchset 2, looks wrong in any case to me)
I'll hold this CL until I get a confirmation on this point.

[Nico, 2014-03-31 17:08:41]

On 2014/03/31 16:19:58, Primiano Tucci wrote:
> On 2014/03/31 15:42:37, Nico wrote:
> > __arm64__ is apple's arm64 abi, and __aarch64__ is the "official" 64 bit
abi,
> > no? Are you sure clang defines __aarch64__ when doing 64 bit ios builds?
> 
> To be honest I don't have any iOS-related knowledge.
> My understanding was that __aarch64__ was always defined and reflects the
target
> HW arch (opposite to __arm64__ which, as you suggested, reflects the abi).
> I got confirmation of this in a former CL (https://crrev.com/216833004/) and
> from my compiler spiritual guide (hans@) :)
> I'd expect the trybots to fail miserably in the opposite case, shouldn't they?

I don't think we have 64 bit iOS trybots (or any bots), but I'm not sure.

> BTW, I've just tested with XCode 5 and yes, __aarch64__ seems to be defined.

Ok, that's good enough :-)

lgtm once the string question is resolved. Thanks for checking!

[sdefresne, 2014-03-31 17:13:05]

On 2014/03/31 17:08:41, Nico wrote:
> On 2014/03/31 16:19:58, Primiano Tucci wrote:
> > On 2014/03/31 15:42:37, Nico wrote:
> > > __arm64__ is apple's arm64 abi, and __aarch64__ is the "official" 64 bit
> abi,
> > > no? Are you sure clang defines __aarch64__ when doing 64 bit ios builds?
> > 
> > To be honest I don't have any iOS-related knowledge.
> > My understanding was that __aarch64__ was always defined and reflects the
> target
> > HW arch (opposite to __arm64__ which, as you suggested, reflects the abi).
> > I got confirmation of this in a former CL (https://crrev.com/216833004/) and
> > from my compiler spiritual guide (hans@) :)
> > I'd expect the trybots to fail miserably in the opposite case, shouldn't
they?
> 
> I don't think we have 64 bit iOS trybots (or any bots), but I'm not sure.

We don't have 64 bit iOS trybots, but we do have fyi 64 bit iOS bots:
http://build.chromium.org/p/chromium.fyi/builders/Chromium%20iOS%20Device%20(...

I tried to get some 64 bit iOS bots, but apparently my CL didn't work, since
they are still only building for 32 bit :-/

> > BTW, I've just tested with XCode 5 and yes, __aarch64__ seems to be defined.
> 
> Ok, that's good enough :-)

Yes, Xcode define both __arm64__ and __aarch64__ when using "-arch arm64".

> 
> lgtm once the string question is resolved. Thanks for checking!

[asargent_no_longer_on_chrome, 2014-03-31 18:36:55]

On 2014/03/31 14:33:47, Primiano Tucci wrote:
> +asargent: apparently you wrote some code which consumes the arch here in
> chrome/browser/extensions/api/runtime/runtime_api.cc
> Do you think the arch string should be renamed as well or not?
> 
> It seems we're stuck on the value of a string here :) Any thoughts?

It probably should not be changed here. 

While we don't yet support extensions on iOS or Android, it is very important
not to break backwards compatibility for apps and extensions by changing the
value of the architecture value returned by the chrome.runtime.getPlatformInfo
method for ARM chromebooks (see
https://developer.chrome.com/extensions/runtime#method-getPlatformInfo). 

The architecture flag is also used in extension autoupdate requests via
OmahaQueryParams::Get, and while we could update the chrome webstore/omaha to
deal with a different value, we wouldn't want to break backwards compatibility
with 3rd party update servers (eg those used by enterprises). 

Additionally, you would want to check with one of the folks in
chrome/browser/component_updater/OWNERS since that code consumes this return
value too. sorin@chromium.org would probably be a good candidate.

[Nico, 2014-03-31 18:42:32]

On Mon, Mar 31, 2014 at 11:36 AM, <asargent@chromium.org> wrote:

> On 2014/03/31 14:33:47, Primiano Tucci wrote:
>
>> +asargent: apparently you wrote some code which consumes the arch here in
>> chrome/browser/extensions/api/runtime/runtime_api.cc
>> Do you think the arch string should be renamed as well or not?
>>
>
>  It seems we're stuck on the value of a string here :) Any thoughts?
>>
>
> It probably should not be changed here.
>
> While we don't yet support extensions on iOS or Android, it is very
> important
> not to break backwards compatibility for apps and extensions by changing
> the
> value of the architecture value returned by the
> chrome.runtime.getPlatformInfo
> method for ARM chromebooks (see
> https://developer.chrome.com/extensions/runtime#method-getPlatformInfo).
>

I don't think we ship any 64 bit arm chromebooks yet.


>
> The architecture flag is also used in extension autoupdate requests via
> OmahaQueryParams::Get, and while we could update the chrome webstore/omaha
> to
> deal with a different value, we wouldn't want to break backwards
> compatibility
> with 3rd party update servers (eg those used by enterprises).
>
> Additionally, you would want to check with one of the folks in
> chrome/browser/component_updater/OWNERS since that code consumes this
> return
> value too. sorin@chromium.org would probably be a good candidate.
>
>
>
>
> https://codereview.chromium.org/218663003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stuartmorgan, 2014-03-31 18:45:20]

"Should not be changed" means leaving it as arm64. It seems like most people are
on board with arm64; Sylvain, do you feel strongly otherwise?

[sdefresne, 2014-03-31 19:15:57]

On 2014/03/31 18:45:20, stuartmorgan wrote:
> "Should not be changed" means leaving it as arm64. It seems like most people
are
> on board with arm64; Sylvain, do you feel strongly otherwise?

No, I have no hard feeling for any particular value. AFAIK, this value is not
used on iOS, and since I'm the one that added it, I don't think it is used
anywhere. "arm64" suits me fine if this is better for the rest of the project.

[Primiano Tucci (use gerrit), 2014-03-31 19:16:32]

On 2014/03/31 18:45:20, stuartmorgan wrote:
> "Should not be changed" means leaving it as arm64. It seems like most people
are
> on board with arm64; Sylvain, do you feel strongly otherwise?

Sylvain and I had a chat offline and it seems that the safest choice is leaving
the string to be "arm64".

Thanks all for your comments.

[Primiano Tucci (use gerrit), 2014-03-31 19:16:48]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2014-03-31 19:17:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/218663003/40001

[sdefresne, 2014-03-31 19:19:16]

Still LGTM

[commit-bot: I haz the power, 2014-03-31 22:10:34]

Change committed as 260671