Removing crypto/third_party/nss/ and removing crypto _win files

Removing crypto/third_party/nss/ and removing crypto _win files

BUG=604728
Committed: https://crrev.com/9c641463a0755cfbbe827a62580dd9859c41806e
Cr-Commit-Position: refs/heads/master@{#391048}

[svaldez, 2016-04-28 20:44:30]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chromium.org

[Ryan Sleevi, 2016-04-28 21:06:58]

https://codereview.chromium.org/1924093006/diff/1/crypto/hmac.cc
File crypto/hmac.cc (right):

https://codereview.chromium.org/1924093006/diff/1/crypto/hmac.cc#newcode24
crypto/hmac.cc:24: };
Can't we get rid of this entirely, since we're getting rid of the
platform-specific bits? That would avoid the extra heap allocation indirection.

just change the std::unique_ptr<HMACPlatformData> plat_ to be
std::vector<unsigned char> key_;

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
This seems like a way bigger size hit to NaCl; we should definitely have a NaCl
owner review this. From discussion with David, I thought the plan was to gut
'just enough' to do HMAC.

[svaldez, 2016-04-28 21:14:51]

https://codereview.chromium.org/1924093006/diff/1/crypto/hmac.cc
File crypto/hmac.cc (right):

https://codereview.chromium.org/1924093006/diff/1/crypto/hmac.cc#newcode24
crypto/hmac.cc:24: };
On 2016/04/28 21:06:58, Ryan Sleevi wrote:
> Can't we get rid of this entirely, since we're getting rid of the
> platform-specific bits? That would avoid the extra heap allocation
indirection.
> 
> just change the std::unique_ptr<HMACPlatformData> plat_ to be
> std::vector<unsigned char> key_;

Done.

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
On 2016/04/28 21:06:58, Ryan Sleevi wrote:
> This seems like a way bigger size hit to NaCl; we should definitely have a
NaCl
> owner review this. From discussion with David, I thought the plan was to gut
> 'just enough' to do HMAC.

I *think* that the static linker will just drop the right things.

[Ryan Sleevi, 2016-04-28 21:29:59]

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
On 2016/04/28 21:14:50, svaldez wrote:
> I *think* that the static linker will just drop the right things.

Well, you're building as <(component), so it can't drop exported symbols, even
if unused.

[davidben, 2016-04-28 21:35:44]

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
On 2016/04/28 21:29:59, Ryan Sleevi wrote:
> On 2016/04/28 21:14:50, svaldez wrote:
> > I *think* that the static linker will just drop the right things.
> 
> Well, you're building as <(component), so it can't drop exported symbols, even
> if unused.

Only in the shared library build, right? The static library build will still
drop things freely.

I don't have hugely strong opinions. The static library version makes the build
less fragile and makes updating BoringSSL less of a pain. At the same time, the
list of files that make up HMAC-SHA-256 probably wouldn't change much and
relying on the static linker for binary size means we need to be watching
nacl_win64's binary size? (Are we? We should...) I am generally a fan of using
the static linker though. Fewer build configs to fuss with and maintain.

Though note that we have rely on the static linker slightly anyway. The method
tables for all the digests in BoringSSL are, right now, kept in the same
file[*]. I believe Windows builds with the equivalent of -ffunction-sections (I
forget the flag, but I recall looking this up at one point), so it should work.
Worth verifying though.

[*] Of course, there's no reason that can't change.

[Ryan Sleevi, 2016-04-29 00:12:55]

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
On 2016/04/28 21:35:44, davidben wrote:
> Only in the shared library build, right? The static library build will still
> drop things freely.

I believe the nacl helper is built as a DLL; but definitely, we'll want to find
a "NaCl build guru" to review.

I'm all in favor of KISS if necessary, but every other NaCl change I saw go by
tried to include narrow inclusions, rather than looking at specific exclusions.

[davidben, 2016-04-29 00:43:15]

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/1/third_party/boringssl/borin...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
On 2016/04/29 00:12:54, Ryan Sleevi wrote:
> On 2016/04/28 21:35:44, davidben wrote:
> > Only in the shared library build, right? The static library build will still
> > drop things freely.
> 
> I believe the nacl helper is built as a DLL; but definitely, we'll want to
find
> a "NaCl build guru" to review.

Looks like it's built as an executable, unless there's magic elsewhere to make
that a DLL.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_exe....

If it were a DLL, the value of <(component) would still be the same. It's one
set of build variables and two targets. We'd be building boringssl_crypto_win64
as a static library (notably NOT exporting the symbols) and then linking that
into a dll, which is what we want. Whether, in that case, the linker would drop
all sections unreachable from *any* exported symbol, I'm not sure.

[svaldez, 2016-04-29 19:23:23]

svaldez@chromium.org changed reviewers:
+ bradnelson@chromium.org

[svaldez, 2016-04-29 19:23:24]

Hi Bradley,

We're removing some of the windows-specific code from crypto, which means that
the Windows NaCl build will be pulling in the HMAC code from BoringSSL instead.
We think that this is fine in terms of binary size regardless of the build,
since the linker should drop all the bits of BoringSSL that aren't used by NaCl,
however we don't know if NaCl's build process does anything to cause this to
pull in more of BoringSSL than it should. 

Do you know whether there are going to be issues by doing this, or who would be
a good "NaCl build guru" to review this?

Thanks,
Steven

[svaldez, 2016-04-29 19:47:24]

On 2016/04/29 19:23:24, svaldez wrote:
> Hi Bradley,
> 
> We're removing some of the windows-specific code from crypto, which means that
> the Windows NaCl build will be pulling in the HMAC code from BoringSSL
instead.
> We think that this is fine in terms of binary size regardless of the build,
> since the linker should drop all the bits of BoringSSL that aren't used by
NaCl,
> however we don't know if NaCl's build process does anything to cause this to
> pull in more of BoringSSL than it should. 
> 
> Do you know whether there are going to be issues by doing this, or who would
be
> a good "NaCl build guru" to review this?
> 
> Thanks,
> Steven

For context, it looks like the static build has a increase of 0.6% (2383 Kb ->
2399 Kb).

[davidben, 2016-04-29 19:55:26]

On 2016/04/29 19:47:24, svaldez wrote:
> On 2016/04/29 19:23:24, svaldez wrote:
> > Hi Bradley,
> > 
> > We're removing some of the windows-specific code from crypto, which means
that
> > the Windows NaCl build will be pulling in the HMAC code from BoringSSL
> instead.
> > We think that this is fine in terms of binary size regardless of the build,
> > since the linker should drop all the bits of BoringSSL that aren't used by
> NaCl,
> > however we don't know if NaCl's build process does anything to cause this to
> > pull in more of BoringSSL than it should. 
> > 
> > Do you know whether there are going to be issues by doing this, or who would
> be
> > a good "NaCl build guru" to review this?
> > 
> > Thanks,
> > Steven
> 
> For context, it looks like the static build has a increase of 0.6% (2383 Kb ->
> 2399 Kb).

Of that, it seems 11kb is because the crypto::HMAC API isn't very binary-size
friendly and depends on SHA-1, so we're shipping BoringSSL's SHA-1 and SHA-256
implementations. We happened to not be shipping a SHA-1 impl last time and just
called out to Windows. (Which wouldn't have worked because of the sandbox, but
NaCl happened not to call it.)

If we cared, we could probably fix this by having the relevant NaCl code call
into BoringSSL directly (which is static-linker friendly; EVP_sha256() and
EVP_sha1() are separate symbols rather than an enum you switch-case on).

[bradnelson, 2016-05-01 00:28:19]

So there's two nacl portions, the win64 part (used in nacl64.exe) and the
in-nacl part (used in chromoting).

Neither is super size sensitive as far as I know.

I think the linker will do the right thing here.

[davidben, 2016-05-02 19:49:43]

lgtm. I'm a little annoyed that boringssl.gyp has a duplicated hunk like that,
but gyp doesn't make it really very easy to avoid that. (Short of having some
boringssl_common.gypi that we include with variables to configure, but that
sounds like a mess.)

https://codereview.chromium.org/1924093006/diff/80001/third_party/boringssl/b...
File third_party/boringssl/boringssl.gyp (right):

https://codereview.chromium.org/1924093006/diff/80001/third_party/boringssl/b...
third_party/boringssl/boringssl.gyp:15: '<@(boringssl_ssl_sources)',
Optional: you can remove this. (Or not. Meh. I suppose it's slightly faster
compile.)

[svaldez, 2016-05-02 19:58:58]

The CQ bit was checked by svaldez@chromium.org

[commit-bot: I haz the power, 2016-05-02 19:59:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1924093006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1924093006/100001

[commit-bot: I haz the power, 2016-05-02 20:49:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-02 20:50:02]

Description was changed from

==========
Removing crypto/third_party/nss/ and removing crypto _win files

BUG=604728
==========

to

==========
Removing crypto/third_party/nss/ and removing crypto _win files

BUG=604728

Committed: https://crrev.com/9c641463a0755cfbbe827a62580dd9859c41806e
Cr-Commit-Position: refs/heads/master@{#391048}
==========

[commit-bot: I haz the power, 2016-05-02 20:50:03]

Patchset 6 (id:??) landed as
https://crrev.com/9c641463a0755cfbbe827a62580dd9859c41806e
Cr-Commit-Position: refs/heads/master@{#391048}