Instrumented libraries: add a hook to download pre-built binaries.

Instrumented libraries: add a hook to download pre-built binaries.

BUG=462636
R=glider@chromium.org
TBR=thakis@chromium.org

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

[earthdok, 2015-03-18 21:47:49]

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

[earthdok, 2015-03-18 21:48:23]

+Nico for build/ ownership and general review (if desired)

[Nico, 2015-03-19 18:31:00]

lgtm

https://codereview.chromium.org/1019573003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1019573003/diff/1/build/common.gypi#newcode441
build/common.gypi:441: 'msan_track_origins%': 2,
unrelated?

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
File third_party/instrumented_libraries/scripts/download_binaries.py (right):

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
third_party/instrumented_libraries/scripts/download_binaries.py:22: if
'msan_track_origins=2' in gyp_defines:
do we need to support multiple modes for this? can we say we only support
chained origins and get rid of this toggle?

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
third_party/instrumented_libraries/scripts/download_binaries.py:39: 
error out here if not on linux?

[earthdok, 2015-03-23 13:02:57]

https://codereview.chromium.org/1019573003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1019573003/diff/1/build/common.gypi#newcode441
build/common.gypi:441: 'msan_track_origins%': 2,
On 2015/03/19 18:31:00, Nico wrote:
> unrelated?

There haven't been any builders using msan_track_origins=1 for a while, so I've
been planning to make 2 the default. The defaults here and in
download_binaries.py must match, so might as well make the change now.

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
File third_party/instrumented_libraries/scripts/download_binaries.py (right):

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
third_party/instrumented_libraries/scripts/download_binaries.py:22: if
'msan_track_origins=2' in gyp_defines:
On 2015/03/19 18:31:00, Nico wrote:
> do we need to support multiple modes for this? can we say we only support
> chained origins and get rid of this toggle?

We may want to support track_origins=0. It's used by a bot that ClusterFuzz
depends on.

Even if we only supported one mode, we'd still want to error out here rather
than download an abi-incompatible set of libraries. I don't see how this code
could be made simpler.

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
third_party/instrumented_libraries/scripts/download_binaries.py:39: 
On 2015/03/19 18:31:00, Nico wrote:
> error out here if not on linux?

Done.

[earthdok, 2015-03-23 13:06:56]

https://codereview.chromium.org/1019573003/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1019573003/diff/1/build/common.gypi#newcode441
build/common.gypi:441: 'msan_track_origins%': 2,
On 2015/03/23 13:02:56, earthdok wrote:
> On 2015/03/19 18:31:00, Nico wrote:
> > unrelated?
> 
> There haven't been any builders using msan_track_origins=1 for a while, so
I've
> been planning to make 2 the default. The defaults here and in
> download_binaries.py must match, so might as well make the change now.

I'll do it in a separate CL.

[Nico, 2015-03-23 16:35:09]

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
File third_party/instrumented_libraries/scripts/download_binaries.py (right):

https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
third_party/instrumented_libraries/scripts/download_binaries.py:22: if
'msan_track_origins=2' in gyp_defines:
On 2015/03/23 13:02:56, earthdok wrote:
> On 2015/03/19 18:31:00, Nico wrote:
> > do we need to support multiple modes for this? can we say we only support
> > chained origins and get rid of this toggle?
> 
> We may want to support track_origins=0. It's used by a bot that ClusterFuzz
> depends on.
> 
> Even if we only supported one mode, we'd still want to error out here rather
> than download an abi-incompatible set of libraries. I don't see how this code
> could be made simpler.

If there's only one mode, where would the abi incompatibility come from?

[earthdok, 2015-03-23 16:48:04]

On 2015/03/23 16:35:09, Nico wrote:
>
https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
> File third_party/instrumented_libraries/scripts/download_binaries.py (right):
> 
>
https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
> third_party/instrumented_libraries/scripts/download_binaries.py:22: if
> 'msan_track_origins=2' in gyp_defines:
> On 2015/03/23 13:02:56, earthdok wrote:
> > On 2015/03/19 18:31:00, Nico wrote:
> > > do we need to support multiple modes for this? can we say we only support
> > > chained origins and get rid of this toggle?
> > 
> > We may want to support track_origins=0. It's used by a bot that ClusterFuzz
> > depends on.
> > 
> > Even if we only supported one mode, we'd still want to error out here rather
> > than download an abi-incompatible set of libraries. I don't see how this
code
> > could be made simpler.
> 
> If there's only one mode, where would the abi incompatibility come from?

I interpreted your words as "pre-built instrumented libraries should support
only one mode". Then we still need to check that the rest of Chromium is being
built in a compatible mode.

If you meant "do not support MSan in Chromium in modes other than
track_origins=2", then my comment about the LKGR bot still stands.

[Alexander Potapenko, 2015-03-23 17:18:44]

LGTM w/nit

https://codereview.chromium.org/1019573003/diff/40001/third_party/instrumente...
File third_party/instrumented_libraries/scripts/download_binaries.py (right):

https://codereview.chromium.org/1019573003/diff/40001/third_party/instrumente...
third_party/instrumented_libraries/scripts/download_binaries.py:21: if 'msan=1'
in gyp_defines:
This may break if there's another GYP flag ending in "msan".
Maybe split gyp_defines by spaces beforehand?

[Nico, 2015-03-23 17:23:58]

On 2015/03/23 16:48:04, earthdok wrote:
> On 2015/03/23 16:35:09, Nico wrote:
> >
>
https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
> > File third_party/instrumented_libraries/scripts/download_binaries.py
(right):
> > 
> >
>
https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
> > third_party/instrumented_libraries/scripts/download_binaries.py:22: if
> > 'msan_track_origins=2' in gyp_defines:
> > On 2015/03/23 13:02:56, earthdok wrote:
> > > On 2015/03/19 18:31:00, Nico wrote:
> > > > do we need to support multiple modes for this? can we say we only
support
> > > > chained origins and get rid of this toggle?
> > > 
> > > We may want to support track_origins=0. It's used by a bot that
ClusterFuzz
> > > depends on.
> > > 
> > > Even if we only supported one mode, we'd still want to error out here
rather
> > > than download an abi-incompatible set of libraries. I don't see how this
> code
> > > could be made simpler.
> > 
> > If there's only one mode, where would the abi incompatibility come from?
> 
> I interpreted your words as "pre-built instrumented libraries should support
> only one mode". Then we still need to check that the rest of Chromium is being
> built in a compatible mode.
> 
> If you meant "do not support MSan in Chromium in modes other than
> track_origins=2", then my comment about the LKGR bot still stands.

You said "we may want to support", not "we have to support". Why does LKGR use
the 0 mode? Could it use 2 too?

[earthdok, 2015-03-23 17:28:08]

On 2015/03/23 17:23:58, Nico wrote:
> On 2015/03/23 16:48:04, earthdok wrote:
> > On 2015/03/23 16:35:09, Nico wrote:
> > >
> >
>
https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
> > > File third_party/instrumented_libraries/scripts/download_binaries.py
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1019573003/diff/1/third_party/instrumented_li...
> > > third_party/instrumented_libraries/scripts/download_binaries.py:22: if
> > > 'msan_track_origins=2' in gyp_defines:
> > > On 2015/03/23 13:02:56, earthdok wrote:
> > > > On 2015/03/19 18:31:00, Nico wrote:
> > > > > do we need to support multiple modes for this? can we say we only
> support
> > > > > chained origins and get rid of this toggle?
> > > > 
> > > > We may want to support track_origins=0. It's used by a bot that
> ClusterFuzz
> > > > depends on.
> > > > 
> > > > Even if we only supported one mode, we'd still want to error out here
> rather
> > > > than download an abi-incompatible set of libraries. I don't see how this
> > code
> > > > could be made simpler.
> > > 
> > > If there's only one mode, where would the abi incompatibility come from?
> > 
> > I interpreted your words as "pre-built instrumented libraries should support
> > only one mode". Then we still need to check that the rest of Chromium is
being
> > built in a compatible mode.
> > 
> > If you meant "do not support MSan in Chromium in modes other than
> > track_origins=2", then my comment about the LKGR bot still stands.
> 
> You said "we may want to support", not "we have to support".
We have to support track_origins=0 in Chromium. We may want to support pre-built
libraries for that configuration. Alternatively we may want to keep using the
old method (in-place builds).

> Why does LKGR use
> the 0 mode? Could it use 2 too?
CF uses both. For performance reasons, fuzzing is done with track_origins=0.
When a crash report is generated, it's reproduced with track_origins=2.

[earthdok, 2015-03-23 17:31:47]

https://codereview.chromium.org/1019573003/diff/40001/third_party/instrumente...
File third_party/instrumented_libraries/scripts/download_binaries.py (right):

https://codereview.chromium.org/1019573003/diff/40001/third_party/instrumente...
third_party/instrumented_libraries/scripts/download_binaries.py:21: if 'msan=1'
in gyp_defines:
On 2015/03/23 17:18:44, Alexander Potapenko wrote:
> This may break if there's another GYP flag ending in "msan".
> Maybe split gyp_defines by spaces beforehand?

fixed

[earthdok, 2015-03-23 19:02:00]

The CQ bit was checked by earthdok@chromium.org

[earthdok, 2015-03-23 19:02:01]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
glider@chromium.org
Link to the patchset: https://codereview.chromium.org/1019573003/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-23 19:02:26]

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

[earthdok, 2015-03-23 19:03:45]

The CQ bit was unchecked by earthdok@chromium.org

[earthdok, 2015-03-23 19:13:21]

added .sha1 files and a .gitignore file

[earthdok, 2015-03-23 19:13:37]

The CQ bit was checked by earthdok@chromium.org

[earthdok, 2015-03-23 19:13:38]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1019573003/#ps80001
(title: "add hashes and .gitignore")

[commit-bot: I haz the power, 2015-03-23 19:14:06]

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

[commit-bot: I haz the power, 2015-03-23 21:07:28]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-23 21:08:20]

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