Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1064693002:
Compile BoringSSL for arm64 iOS (Closed)
DescriptionCompile BoringSSL for arm64 iOS
BUG=338886
Committed: https://crrev.com/de446058f8bb18cdae0ef9395b76ac0d1c1b8f26
Cr-Commit-Position: refs/heads/master@{#324132}
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR comments #Patch Set 3 : Fix typo #
Messages
Total messages: 16 (5 generated)
tkchin@chromium.org changed reviewers: + davidben@chromium.org
+agl. Could you file a bug in crbug for this? I assume this is to get BoringSSL/iOS working. We'll need to later get the assembly working which will require probing for some CPU capabilities, so all that should want bugs filed. Also, do you have the standalone build working with the tests running? If we're going to be shipping this code, I'd be a lot more comfortable with the standalone tests working in some form (at least the ones that don't need Go; the Go ones I expect will need more effort, though it's worth seeing how //net's test server is run on iOS). It might be worth taking a few minutes to talk face-to-face about what it'd take to do this port with tests and assembly and all (not to say this isn't a sensible start). I wasn't even aware you were working on this. https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... File third_party/boringssl/boringssl.gyp (right): https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... third_party/boringssl/boringssl.gyp:29: ['target_arch == "arm"', { Why only arm64? Is iOS only 64-bit? (I'm not familiar with iOS.)
I'll put something on your calendar. I tried the change with the demo app and it behaved normally. Need to do some work to get the whole suite of webrtc tests working on iOS though. Re-used the old bug I filed. Do we need something separate for Chromium iOS? WebRTC shouldn't need certificate root store, which is why this change should work for us. https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... File third_party/boringssl/boringssl.gyp (right): https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... third_party/boringssl/boringssl.gyp:29: ['target_arch == "arm"', { On 2015/04/06 21:15:02, David Benjamin wrote: > Why only arm64? Is iOS only 64-bit? (I'm not familiar with iOS.) The reason is actually because the various gyp files in third party dependencies aren't consistent in their use of target_arch. The correct way is likely to use target_arch="arm" and target_subarch="armv7" or target_subarch="arm64". However, most projects just use target_arch="armv7" and target_arch="arm64" rules. In this case when specifying armv7 we'll short circuit to the OPENSSL_NO_ASM block below.
https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/BUILD.gn File third_party/boringssl/BUILD.gn (right): https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/BUILD... third_party/boringssl/BUILD.gn:93: } else if (current_cpu == "arm") { The arm/armv7 thing doesn't apply to GN too, does it? I don't see an current_cpu == "armv7" lines. This probably wants an is_linux || is_android check here too? https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... File third_party/boringssl/boringssl.gyp (right): https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... third_party/boringssl/boringssl.gyp:29: ['target_arch == "arm"', { On 2015/04/06 21:35:01, tkchin wrote: > On 2015/04/06 21:15:02, David Benjamin wrote: > > Why only arm64? Is iOS only 64-bit? (I'm not familiar with iOS.) > > The reason is actually because the various gyp files in third party dependencies > aren't consistent in their use of target_arch. The correct way is likely to use > target_arch="arm" and target_subarch="armv7" or target_subarch="arm64". However, > most projects just use target_arch="armv7" and target_arch="arm64" rules. > > In this case when specifying armv7 we'll short circuit to the OPENSSL_NO_ASM > block below. Huh, interesting. Yeah, doing codesearch I see a lot of (target_arch == "arm" or target_arch == "armv7") rules. Never armv7 on its own though. Since GN doesn't seem to have that separation, how about switching the target_arch == "arm" line to check both and then add the same OS=linux/android condition in the arm block? Is that a correct use of target_arch? (Mostly I want to avoid it being confusing when glancing at it why only one of the two blocks checks the OS. And since presumably we'll want to eventually ship the assembly on iOS too, this seems a reasonable thing to do now. Also nice to keep GN and GYP in sync.)
On 2015/04/06 21:54:39, David Benjamin wrote: > https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/BUILD.gn > File third_party/boringssl/BUILD.gn (right): > > https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/BUILD... > third_party/boringssl/BUILD.gn:93: } else if (current_cpu == "arm") { > The arm/armv7 thing doesn't apply to GN too, does it? I don't see an current_cpu > == "armv7" lines. This probably wants an is_linux || is_android check here too? > > https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... > File third_party/boringssl/boringssl.gyp (right): > > https://codereview.chromium.org/1064693002/diff/1/third_party/boringssl/borin... > third_party/boringssl/boringssl.gyp:29: ['target_arch == "arm"', { > On 2015/04/06 21:35:01, tkchin wrote: > > On 2015/04/06 21:15:02, David Benjamin wrote: > > > Why only arm64? Is iOS only 64-bit? (I'm not familiar with iOS.) > > > > The reason is actually because the various gyp files in third party > dependencies > > aren't consistent in their use of target_arch. The correct way is likely to > use > > target_arch="arm" and target_subarch="armv7" or target_subarch="arm64". > However, > > most projects just use target_arch="armv7" and target_arch="arm64" rules. > > > > In this case when specifying armv7 we'll short circuit to the OPENSSL_NO_ASM > > block below. > > Huh, interesting. Yeah, doing codesearch I see a lot of (target_arch == "arm" or > target_arch == "armv7") rules. Never armv7 on its own though. > > Since GN doesn't seem to have that separation, how about switching the > target_arch == "arm" line to check both and then add the same OS=linux/android > condition in the arm block? Is that a correct use of target_arch? (Mostly I want > to avoid it being confusing when glancing at it why only one of the two blocks > checks the OS. And since presumably we'll want to eventually ship the assembly > on iOS too, this seems a reasonable thing to do now. Also nice to keep GN and > GYP in sync.) Done.
lgtm
The CQ bit was checked by tkchin@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1064693002/#ps40001 (title: "Fix typo")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1064693002/40001
The CQ bit was unchecked by commit-bot@chromium.org
This issue passed the CQ dry run.
The CQ bit was checked by tkchin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1064693002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/de446058f8bb18cdae0ef9395b76ac0d1c1b8f26 Cr-Commit-Position: refs/heads/master@{#324132} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
