CFI: download LLVM Gold plugin, if requested by env var or GYP_DEFINES.

CFI: download LLVM Gold plugin, if requested by env var or GYP_DEFINES.

BUG=chromium:536159

[krasin, 2015-09-28 21:25:07]

krasin@google.com changed reviewers:
+ hans@chromium.org

[krasin, 2015-09-28 21:25:07]

Hans,

please, review this CL.

It has two issues, which I am aware of:

1. Gold plugin does not have a stamp file, so we don't know, if it's up to date.
With this CL, the script will download it anyway, even if LLVM toolchain is up
to date. This is correct, but a little bit wasteful.

2. The GYP_DEFINES check is too complicated, and it could be that I have missed
some. It's also inevitable that at some point GYP scripts will add a new
functionality, and this check will need to be updated.

It's possible to address #1 at the cost of a few extra lines of code. Let me
know if you think it's worth doing, and I will add this to the current CL.

#2 does not have a good solution, or, at least, I am not aware of it.

[scottmg, 2015-09-28 22:09:41]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-09-28 22:09:41]

What's the current size of the toolchain vs. the 12M that'd be added?

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:56: if [[ "$GYP_DEFINES" =~
.*(cfi_vptr|use_lto|use_lto_o2|official_build)=1.* ]]; then
Is "official_build=1" how CrOS does it? On Windows it's `buildtype=Official`.

I've also been burned by this crazy way that compile.py sets official build
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/co...

I don't know if you have to take care of that here or not (probably not since I
guess the bot will set one of the other things you check for above?)

[hans, 2015-09-28 22:11:03]

hans@chromium.org changed reviewers:
+ pcc@chromium.org

[hans, 2015-09-28 22:11:04]

+pcc who added the current gold plugin downloading code, in case he has any
additional comments.

On 2015/09/28 21:25:07, krasin wrote:
> 1. Gold plugin does not have a stamp file, so we don't know, if it's up to
date.

The gold plugin has the same version as clang. I think we can just check for the
presence of the plugin instead of having a separate stamp file.

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:56: if [[ "$GYP_DEFINES" =~
.*(cfi_vptr|use_lto|use_lto_o2|official_build)=1.* ]]; then
Seems like a lot of different options :-/ Can you rewrite this so it fits under
80 columns? Maybe a for-loop over them would be cleaner?

Also, should the if-block be Linux-only? I assume we don't want to do this when
official_build=1 on Mac.

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:57: # LLVM Gold plugin is required to build with
this configuration
ultra nit: period at the end of the comment.

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:242: if [[ -n "${LLVM_DOWNLOAD_GOLD_PLUGIN}" ]];
then
The code below (line 285) already downloads the gold plugin if this variable is
set.

It would be better if the code here just checked if the gold plugin is already
downloaded, and if not skips the exit 0 below. That would correspond nicely to
the comment above: "Check if there's anything to be done, exit early if not."

[hans, 2015-09-28 22:12:52]

On 2015/09/28 22:09:41, scottmg wrote:
> What's the current size of the toolchain vs. the 12M that'd be added?

clang-247874-1.tgz is 29M.

[krasin, 2015-09-28 22:22:41]

On 2015/09/28 22:12:52, hans wrote:
> On 2015/09/28 22:09:41, scottmg wrote:
> > What's the current size of the toolchain vs. the 12M that'd be added?
> 
> clang-247874-1.tgz is 29M.

I observe 13 MB:
krasin@krasin2:~/chr7/src$ ls -lh
./third_party/llvm-build/Release+Asserts/llvmgold-247874-1.tgz
-rw-r----- 1 krasin eng 13M Sep 28 15:21
./third_party/llvm-build/Release+Asserts/llvmgold-247874-1.tgz

And it will be 12 MB, if gzip -9 was used.

[krasin, 2015-09-28 22:24:52]

Correction: 12 MB with bzip2 -9, still 13 MB with gzip -9.

[krasin, 2015-09-28 22:27:45]

On 2015/09/28 22:24:52, krasin wrote:
> Correction: 12 MB with bzip2 -9, still 13 MB with gzip -9.

Sorry, I have misread the question. Yes, the toolchain size is 29M

[krasin, 2015-09-28 22:32:37]

On 2015/09/28 22:09:41, scottmg wrote:
> What's the current size of the toolchain vs. the 12M that'd be added?
> 
>
https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update.sh
> File tools/clang/scripts/update.sh (right):
> 
>
https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
> tools/clang/scripts/update.sh:56: if [[ "$GYP_DEFINES" =~
> .*(cfi_vptr|use_lto|use_lto_o2|official_build)=1.* ]]; then
> Is "official_build=1" how CrOS does it? On Windows it's `buildtype=Official`.
> 
> I've also been burned by this crazy way that compile.py sets official build
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/co...
> 
> I don't know if you have to take care of that here or not (probably not since
I
> guess the bot will set one of the other things you check for above?)

Ugh...
The problem is that not only we need to detect all official builds, we also need
to detect all builds, where LTO is requested.
And that's an open-ended list, which will guarantee troubles for people. :(

[krasin, 2015-09-28 22:38:52]

> On 2015/09/28 21:25:07, krasin wrote:
> > 1. Gold plugin does not have a stamp file, so we don't know, if it's up to
> date.
> 
> The gold plugin has the same version as clang. I think we can just check for
the
> presence of the plugin instead of having a separate stamp file.

Consider the following sequence for a local Chrome checkout:

1. I create a new client and run hooks with cfi_vptr=1. LLVM toolchain and Gold
plugin are downloaded and have the version 1.
2. I drop cfi_vptr=1, then sync to a new revision. LLVM Toolchain has version 2,
Gold plugin still has version 1.
3. I add cfi_vptr=1 back, run hooks. LLVM Toolchain has version 2 and is
considered up to date, Gold plugin is not downloaded.
4. Boom: weird build error due to Clang and Gold plugin version mismatch.

If we don't want to introduce another stamp (I don't), we could probably delete
Gold plugin whenever we update Clang, and not update the plugin.

[hans, 2015-09-28 22:41:34]

On 2015/09/28 22:38:52, krasin wrote:
> If we don't want to introduce another stamp (I don't), we could probably
delete
> Gold plugin whenever we update Clang, and not update the plugin.

We already do this. See the 'rm -rf "${LLVM_BUILD_DIR}"' before extracting the
clang tarball. The plugin also lives under that directory.

[krasin, 2015-09-28 22:43:45]

On 2015/09/28 22:41:34, hans wrote:
> On 2015/09/28 22:38:52, krasin wrote:
> > If we don't want to introduce another stamp (I don't), we could probably
> delete
> > Gold plugin whenever we update Clang, and not update the plugin.
> 
> We already do this. See the 'rm -rf "${LLVM_BUILD_DIR}"' before extracting the
> clang tarball. The plugin also lives under that directory.

Thank you, I didn't realize that. In this case, things are a little better.

[scottmg, 2015-09-28 22:43:49]

scottmg@chromium.org changed reviewers:
+ thakis@chromium.org

[scottmg, 2015-09-28 22:43:49]

I'm inclined to say it's not really worth doing it conditionally unless we can
simplify the conditions under which the plugin in required, preferably to
exactly one thing to be checked for.

But I don't really know how often it's updated and what sort of hassle adding
this creates when rolling, etc., so I defer to Hans and Nico.

[krasin, 2015-09-28 22:46:57]

On 2015/09/28 22:43:49, scottmg wrote:
> I'm inclined to say it's not really worth doing it conditionally unless we can
> simplify the conditions under which the plugin in required, preferably to
> exactly one thing to be checked for.
> 
> But I don't really know how often it's updated and what sort of hassle adding
> this creates when rolling, etc., so I defer to Hans and Nico.

I also think that we should not create a fragile logic, that has a shrinking
usefulness: if LTO survives the reality test, it's going to be used more often
(not just for CFI or the official build), as least, on Linux.

[hans, 2015-09-28 23:07:03]

On 2015/09/28 22:46:57, krasin wrote:
> On 2015/09/28 22:43:49, scottmg wrote:
> > I'm inclined to say it's not really worth doing it conditionally unless we
can
> > simplify the conditions under which the plugin in required, preferably to
> > exactly one thing to be checked for.
> > 
> > But I don't really know how often it's updated and what sort of hassle
adding
> > this creates when rolling, etc., so I defer to Hans and Nico.
> 
> I also think that we should not create a fragile logic, that has a shrinking
> usefulness: if LTO survives the reality test, it's going to be used more often
> (not just for CFI or the official build), as least, on Linux.

The clang package is shipped to a large number of developers and bots. We've
always tried to keep the package small in order to minimize the annoyance of
downloading yet another thing in order to build.

It seems merging the gold plugin into the clang package would grow it by 40%,
for something that's only used on in a small fraction of our Linux builds. If
this starts getting used more broadly, we can consider merging it into one
package, but it seems too early for that.

About the logic for this being fragile, I'm not actually too worried. It's
annoying that we apparently have a lot of different flags for enabling LTO
(maybe that's the real problem?), but if we miss one, the error will be hard and
obvious.

[krasin, 2015-10-01 21:07:46]

Since we care about the Chromium LLVM toolchain distribution size and want to
keep it as small as we can, I have investigated various compression options. It
turns out that if we use xz, we'll get 20% reduce of the size, even if LLVM Gold
plugin is included (it will be 24 MB vs 29 MB now).

My proposal is to include LLVM Gold plugin into the toolchain (on Linux) and
adopt xz. It's widely used in the industry for exactly this purpose:
https://en.wikipedia.org/wiki/Xz

"xz has gained notability for compressing packages in the GNU coreutils
project,[7] Debian family of systems deb (file format), openSUSE,[8] Fedora,[9]
Arch Linux,[10] Slackware,[11] FreeBSD,[12] Gentoo,[13] GNOME,[14] and TeX
Live,[15] as well as being an option to compress a compiled Linux kernel.[16] In
December 2013, kernel.org announced the addition of xz compressed files and
ending bzip2 compressed files for distributing kernel archive files.[17]"

Any objections? If none, I will go ahead and make another CL, that implements
that.

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:57: # LLVM Gold plugin is required to build with
this configuration
On 2015/09/28 22:11:04, hans wrote:
> ultra nit: period at the end of the comment.

Done.

[krasin, 2015-10-01 21:08:15]

Sorry, forgot to include the link to the spreadsheet with all the sizes:
https://docs.google.com/spreadsheets/d/138ClPDBbtnPd_kNurBSyQEX5MnKjcBLeIqE56...

[krasin, 2015-10-01 21:23:29]

On 2015/10/01 21:08:15, krasin wrote:
> Sorry, forgot to include the link to the spreadsheet with all the sizes:
>
https://docs.google.com/spreadsheets/d/138ClPDBbtnPd_kNurBSyQEX5MnKjcBLeIqE56...

Hans suggested that I should also measure the size with xz, but without the
plugin. I have updated the spreadsheet,

No plugin, xz: 19 MB
With plugin, xz: 24 MB

This is just 5 MB, and we actually don't lose anything:

1) We save 5 MB since it will be 24 MB vs 29 MB now
2) By including LLVM Gold plugin into the archive, we simplify the logic. Not
only we avoid this CL,
I will also be able to remove lots of cruft under build/, tools/clang/scripts/
and in the buildbot configs.

Nico,
can you please ack, if you're fine with the proposal?

[krasin, 2015-10-05 19:17:34]

I have a brand new CL: https://codereview.chromium.org/1375213007/
In fact, the size of the archive (Gold plugin included) is just 21 MB, not 24
MB, as it was previously reported. Possibly, I had some local artifacts.
Anyway, the new number ought to be correct, as it's the size of the archive
produced by package.py (with my CL applied).

Closing this CL, as I highly hesitate to commit this ugly lto-detection logic.

[Nico, 2015-10-08 02:27:25]

i still think this is the way to go. needs the same change in the .py (sorry)

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:56: if [[ "$GYP_DEFINES" =~
.*(cfi_vptr|use_lto|use_lto_o2|official_build)=1.* ]]; then
On 2015/09/28 22:11:04, hans wrote:
> Seems like a lot of different options :-/ Can you rewrite this so it fits
under
> 80 columns? Maybe a for-loop over them would be cleaner?
> 
> Also, should the if-block be Linux-only? I assume we don't want to do this
when
> official_build=1 on Mac.

Yeah, I think you need buildtype=Official (and maybe branding=Chrome)
(https://code.google.com/p/chromium/codesearch#chromium/src/build/linux/sysroo...).

I'd expect that almost nobody uses these other toggles, and those who do know
what to do.

[krasin, 2015-10-08 02:44:34]

What's about gn? In case of gn, there could be no environment variables like
that set, since it's all in args.gn.

[Nico, 2015-10-08 02:46:27]

we can burn that bridge when we get to it. (how does the sysroot get downloaded
in that case?)

[krasin, 2015-10-08 02:47:11]

On 2015/10/08 02:46:27, Nico (offline until Fri Oct 9) wrote:
> we can burn that bridge when we get to it. (how does the sysroot get
downloaded
> in that case?)

sorry for non-educated question: what is sysroot in the case of Chrome?

[krasin, 2015-10-08 02:48:19]

On 2015/10/08 02:47:11, krasin wrote:
> On 2015/10/08 02:46:27, Nico (offline until Fri Oct 9) wrote:
> > we can burn that bridge when we get to it. (how does the sysroot get
> downloaded
> > in that case?)
> 
> sorry for non-educated question: what is sysroot in the case of Chrome?

btw, it was suggested that Gold plugin could be downloaded similarly to NaCl
toolchain. It's still a part of gclient runhooks, but at that stage it knows if
nacl is enabled.

[krasin, 2015-10-08 02:50:29]

On 2015/10/08 02:48:19, krasin wrote:
> On 2015/10/08 02:47:11, krasin wrote:
> > On 2015/10/08 02:46:27, Nico (offline until Fri Oct 9) wrote:
> > > we can burn that bridge when we get to it. (how does the sysroot get
> > downloaded
> > > in that case?)
> > 
> > sorry for non-educated question: what is sysroot in the case of Chrome?
> 
> btw, it was suggested that Gold plugin could be downloaded similarly to NaCl
> toolchain. It's still a part of gclient runhooks, but at that stage it knows
if
> nacl is enabled.

Hm... may be that wrong, as the script for downloading nacl toolchain is called
from DEPS:
https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&q=download_na....

Not sure, probably we should ignore the advice given to me.

[krasin, 2015-10-09 21:33:52]

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:56: if [[ "$GYP_DEFINES" =~
.*(cfi_vptr|use_lto|use_lto_o2|official_build)=1.* ]]; then
On 2015/10/08 02:27:24, Nico (offline until Fri Oct 9) wrote:
> On 2015/09/28 22:11:04, hans wrote:
> > Seems like a lot of different options :-/ Can you rewrite this so it fits
> under
> > 80 columns? Maybe a for-loop over them would be cleaner?
> > 
> > Also, should the if-block be Linux-only? I assume we don't want to do this
> when
> > official_build=1 on Mac.
> 
> Yeah, I think you need buildtype=Official (and maybe branding=Chrome)
>
(https://code.google.com/p/chromium/codesearch#chromium/src/build/linux/sysroo...).
> 
> I'd expect that almost nobody uses these other toggles, and those who do know
> what to do.

Done in https://codereview.chromium.org/1385403006

https://codereview.chromium.org/1373043003/diff/1/tools/clang/scripts/update....
tools/clang/scripts/update.sh:242: if [[ -n "${LLVM_DOWNLOAD_GOLD_PLUGIN}" ]];
then
On 2015/09/28 22:11:04, hans wrote:
> The code below (line 285) already downloads the gold plugin if this variable
is
> set.
> 
> It would be better if the code here just checked if the gold plugin is already
> downloaded, and if not skips the exit 0 below. That would correspond nicely to
> the comment above: "Check if there's anything to be done, exit early if not."

Good idea. Done in https://codereview.chromium.org/1385403006