Add VP8 and H264 depacketizer fuzzers.

Add VP8 and H264 depacketizer fuzzers.

Also removes listing of targets in webrtc_fuzzers which is very prone to
not being up to date. They're not required for ClusterFuzz integration
or building locally. This also means that adding fuzzers won't require
approval outside the fuzzers directory.

BUG=webrtc:4771
R=kjellander@webrtc.org, stefan@webrtc.org

[pbos-webrtc, 2015-12-11 15:36:34]

PTAL :)

[pbos-webrtc, 2015-12-11 15:37:54]

move h264 target

[pbos-webrtc, 2015-12-11 15:47:17]

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer build
targets under
Note that this is similar to what use_libfuzzer in src/BUILD.gn does under
Chromium. I don't think this group target is useful if we can't do wildcards
(test/fuzzers:*) because it'll get outdated and mismatch (also I don't think
anyone needs to build it).

[pbos-webrtc, 2015-12-11 15:54:30]

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer build
targets under
On 2015/12/11 15:47:17, pbos-webrtc wrote:
> Note that this is similar to what use_libfuzzer in src/BUILD.gn does under
> Chromium. I don't think this group target is useful if we can't do wildcards
> (test/fuzzers:*) because it'll get outdated and mismatch (also I don't think
> anyone needs to build it).

When getting this on the bots, we can run gn refs out/Release --all
--type=executable --as=output webrtc/test/fuzzers:webrtc_fuzzer_main to discover
them (that's what ClusterFuzz' upload bot does).

[kjellander_webrtc, 2015-12-14 07:11:37]

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer build
targets under
On 2015/12/11 15:47:17, pbos-webrtc wrote:
> Note that this is similar to what use_libfuzzer in src/BUILD.gn does under
> Chromium. I don't think this group target is useful if we can't do wildcards
> (test/fuzzers:*) because it'll get outdated and mismatch (also I don't think
> anyone needs to build it).

Is the goal here to have a minimum amount of objects to compile in order to get
the fuzz binaries for a bot building with use_libfuzzer=true?
Then I still suggest keeping the old name, so the CLI ends up being:
 out/Release --all
--type=executable --as=output webrtc:webrtc_fuzzers

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer build
targets under
On 2015/12/11 15:47:17, pbos-webrtc wrote:
> Note that this is similar to what use_libfuzzer in src/BUILD.gn does under
> Chromium. I don't think this group target is useful if we can't do wildcards
> (test/fuzzers:*) because it'll get outdated and mismatch (also I don't think
> anyone needs to build it).

If you create a target named 'fuzzers' (same as the directory name) inside
webrtc/test/fuzzers/BUILD.gn, it will be built by default if something depends
on 'webrtc/test/fuzzers'.
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

Thus that gives you the possibility of maintaining a list of what should be
compiled in webrtc/fuzzers while keeping it simple for the above level including
it.

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode286
webrtc/BUILD.gn:286: group("webrtc_fuzzer_gn_link") {
I prefer the old name, I don't like the _gn_link suffix

https://codereview.webrtc.org/1518973003/diff/1/webrtc/test/fuzzers/BUILD.gn
File webrtc/test/fuzzers/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/test/fuzzers/BUILD.gn#...
webrtc/test/fuzzers/BUILD.gn:18: "../../system_wrappers:field_trial_default",
If you do the change in webrtc/test/BUILD.gn, shouldn't you include the fuzzers
as dependencies here to ensure they're built?

https://codereview.webrtc.org/1518973003/diff/1/webrtc/test/fuzzers/BUILD.gn#...
webrtc/test/fuzzers/BUILD.gn:33: if (is_clang) {
Please can you fix the warnings and remove this instead? Applies below as well.

[pbos-webrtc, 2015-12-14 13:06:49]

dummy

[pbos-webrtc, 2015-12-14 13:07:03]

ptal, renamed target -> dummy.

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer build
targets under
On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> On 2015/12/11 15:47:17, pbos-webrtc wrote:
> > Note that this is similar to what use_libfuzzer in src/BUILD.gn does under
> > Chromium. I don't think this group target is useful if we can't do wildcards
> > (test/fuzzers:*) because it'll get outdated and mismatch (also I don't think
> > anyone needs to build it).
> 
> If you create a target named 'fuzzers' (same as the directory name) inside
> webrtc/test/fuzzers/BUILD.gn, it will be built by default if something depends
> on 'webrtc/test/fuzzers'.
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...
> 
> Thus that gives you the possibility of maintaining a list of what should be
> compiled in webrtc/fuzzers while keeping it simple for the above level
including
> it.

The point is to not have such a list (because it requires duplication, and we've
already missed putting targets in this list). I need to have a link from webrtc
-> webrtc/test/fuzzers/BUILD.gn so that targets inside it can be built from gn
generated from root webrtc.

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode286
webrtc/BUILD.gn:286: group("webrtc_fuzzer_gn_link") {
On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> I prefer the old name, I don't like the _gn_link suffix

Point was to point out that it's not a "real" target, so that no one tries to
build it. I've changed it to "webrtc_fuzzers_dummy", because building this
target doesn't give you any fuzzers.

https://codereview.webrtc.org/1518973003/diff/1/webrtc/test/fuzzers/BUILD.gn
File webrtc/test/fuzzers/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/test/fuzzers/BUILD.gn#...
webrtc/test/fuzzers/BUILD.gn:18: "../../system_wrappers:field_trial_default",
On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> If you do the change in webrtc/test/BUILD.gn, shouldn't you include the
fuzzers
> as dependencies here to ensure they're built?

I assume you mean webrtc/BUILD.gn. No fuzzer target will be built by default,
because we don't want to maintain a list of targets in this file (but rather
compile them all). This list of targets is already outdated and easy to miss.

This is what Chromium's libfuzzer bot does today (discovers targets using gn
refs).

https://codereview.webrtc.org/1518973003/diff/1/webrtc/test/fuzzers/BUILD.gn#...
webrtc/test/fuzzers/BUILD.gn:33: if (is_clang) {
On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> Please can you fix the warnings and remove this instead? Applies below as
well.

Nope, it's not in these files, but video_coding isn't find_bad_constructs clean.
All depending targets have to suppress this warning. :(

[kjellander_webrtc, 2015-12-14 18:49:06]

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer build
targets under
On 2015/12/14 13:07:02, pbos-webrtc wrote:
> On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> > On 2015/12/11 15:47:17, pbos-webrtc wrote:
> > > Note that this is similar to what use_libfuzzer in src/BUILD.gn does under
> > > Chromium. I don't think this group target is useful if we can't do
wildcards
> > > (test/fuzzers:*) because it'll get outdated and mismatch (also I don't
think
> > > anyone needs to build it).
> > 
> > If you create a target named 'fuzzers' (same as the directory name) inside
> > webrtc/test/fuzzers/BUILD.gn, it will be built by default if something
depends
> > on 'webrtc/test/fuzzers'.
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...
> > 
> > Thus that gives you the possibility of maintaining a list of what should be
> > compiled in webrtc/fuzzers while keeping it simple for the above level
> including
> > it.
> 
> The point is to not have such a list (because it requires duplication, and
we've
> already missed putting targets in this list). I need to have a link from
webrtc
> -> webrtc/test/fuzzers/BUILD.gn so that targets inside it can be built from gn
> generated from root webrtc.

I read up a bit on the Chromium implementation and you're right, it uses gn refs
as a way to figure out which fuzzer executables exists, which is new to me (but
seems to work well).

However, I still think we should use my approach, with webrtc/BUILD.gn having a
target that builds all our fuzzer code. That needs to be a separate target as
'webrtc_tests' is already taken and is not built by Chromium (as we normally
don't want to build the test code in Chromium).

I think it should be 'webrtc_fuzzers', and it should build all our fuzzers. That
way, we can also set use_libfuzzer=true in our
https://chromium.googlesource.com/external/webrtc/+/master/build_overrides/we...
so they're built by default in our standalone build. Then we'll get protection
from compilation errors that otherwise would only be discovered during rolling
into WebRTC, since the code isn't compiled.
Even if we build all our fuzzers in 'webrtc_fuzzers', the 'gn refs' lookup will
still work for the Chromium bot.

https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode286
webrtc/BUILD.gn:286: group("webrtc_fuzzer_gn_link") {
On 2015/12/14 13:07:02, pbos-webrtc wrote:
> On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> > I prefer the old name, I don't like the _gn_link suffix
> 
> Point was to point out that it's not a "real" target, so that no one tries to
> build it. I've changed it to "webrtc_fuzzers_dummy", because building this
> target doesn't give you any fuzzers.

I think it would be useful to have a target we can build, otherwise the code
will stop compiling as soon someone commits a mistake, or some new Clang
compilation warning is rolled in without the code being actually compiled for
every build.

[pbos-webrtc, 2015-12-14 22:09:26]

On 2015/12/14 18:49:06, kjellander (webrtc) wrote:
> https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
> File webrtc/BUILD.gn (right):
> 
> https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
> webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer
build
> targets under
> On 2015/12/14 13:07:02, pbos-webrtc wrote:
> > On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> > > On 2015/12/11 15:47:17, pbos-webrtc wrote:
> > > > Note that this is similar to what use_libfuzzer in src/BUILD.gn does
under
> > > > Chromium. I don't think this group target is useful if we can't do
> wildcards
> > > > (test/fuzzers:*) because it'll get outdated and mismatch (also I don't
> think
> > > > anyone needs to build it).
> > > 
> > > If you create a target named 'fuzzers' (same as the directory name) inside
> > > webrtc/test/fuzzers/BUILD.gn, it will be built by default if something
> depends
> > > on 'webrtc/test/fuzzers'.
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...
> > > 
> > > Thus that gives you the possibility of maintaining a list of what should
be
> > > compiled in webrtc/fuzzers while keeping it simple for the above level
> > including
> > > it.
> > 
> > The point is to not have such a list (because it requires duplication, and
> we've
> > already missed putting targets in this list). I need to have a link from
> webrtc
> > -> webrtc/test/fuzzers/BUILD.gn so that targets inside it can be built from
gn
> > generated from root webrtc.
> 
> I read up a bit on the Chromium implementation and you're right, it uses gn
refs
> as a way to figure out which fuzzer executables exists, which is new to me
(but
> seems to work well).
> 
> However, I still think we should use my approach, with webrtc/BUILD.gn having
a
> target that builds all our fuzzer code. That needs to be a separate target as
> 'webrtc_tests' is already taken and is not built by Chromium (as we normally
> don't want to build the test code in Chromium).
> 
> I think it should be 'webrtc_fuzzers', and it should build all our fuzzers.
That
> way, we can also set use_libfuzzer=true in our
>
https://chromium.googlesource.com/external/webrtc/+/master/build_overrides/we...
> so they're built by default in our standalone build. Then we'll get protection
> from compilation errors that otherwise would only be discovered during rolling
> into WebRTC, since the code isn't compiled.
> Even if we build all our fuzzers in 'webrtc_fuzzers', the 'gn refs' lookup
will
> still work for the Chromium bot.
> 
> https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode286
> webrtc/BUILD.gn:286: group("webrtc_fuzzer_gn_link") {
> On 2015/12/14 13:07:02, pbos-webrtc wrote:
> > On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> > > I prefer the old name, I don't like the _gn_link suffix
> > 
> > Point was to point out that it's not a "real" target, so that no one tries
to
> > build it. I've changed it to "webrtc_fuzzers_dummy", because building this
> > target doesn't give you any fuzzers.
> 
> I think it would be useful to have a target we can build, otherwise the code
> will stop compiling as soon someone commits a mistake, or some new Clang
> compilation warning is rolled in without the code being actually compiled for
> every build.

Can we consider the gn refs approach for our bots as well? Then we don't risk
breaking chromium libfuzzers due to committing something that didn't go into the
fuzzer list (because we missed adding it). Otherwise, do you think we can
enforce the list being up to date through the PRESUBMIT.py script (without false
negatives)?

We've already missed adding targets, which is why I don't want to have this list
here. It's different from general test targets, since every new fuzzer adds a
new binary, whereas every test change usually only modifies existing targets.

[kjellander_webrtc, 2015-12-16 10:11:33]

On 2015/12/14 22:09:26, pbos-webrtc wrote:
> On 2015/12/14 18:49:06, kjellander (webrtc) wrote:
> > https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn
> > File webrtc/BUILD.gn (right):
> > 
> > https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode284
> > webrtc/BUILD.gn:284: # This target is only here for gn to discover fuzzer
> build
> > targets under
> > On 2015/12/14 13:07:02, pbos-webrtc wrote:
> > > On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> > > > On 2015/12/11 15:47:17, pbos-webrtc wrote:
> > > > > Note that this is similar to what use_libfuzzer in src/BUILD.gn does
> under
> > > > > Chromium. I don't think this group target is useful if we can't do
> > wildcards
> > > > > (test/fuzzers:*) because it'll get outdated and mismatch (also I don't
> > think
> > > > > anyone needs to build it).
> > > > 
> > > > If you create a target named 'fuzzers' (same as the directory name)
inside
> > > > webrtc/test/fuzzers/BUILD.gn, it will be built by default if something
> > depends
> > > > on 'webrtc/test/fuzzers'.
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...
> > > > 
> > > > Thus that gives you the possibility of maintaining a list of what should
> be
> > > > compiled in webrtc/fuzzers while keeping it simple for the above level
> > > including
> > > > it.
> > > 
> > > The point is to not have such a list (because it requires duplication, and
> > we've
> > > already missed putting targets in this list). I need to have a link from
> > webrtc
> > > -> webrtc/test/fuzzers/BUILD.gn so that targets inside it can be built
from
> gn
> > > generated from root webrtc.
> > 
> > I read up a bit on the Chromium implementation and you're right, it uses gn
> refs
> > as a way to figure out which fuzzer executables exists, which is new to me
> (but
> > seems to work well).
> > 
> > However, I still think we should use my approach, with webrtc/BUILD.gn
having
> a
> > target that builds all our fuzzer code. That needs to be a separate target
as
> > 'webrtc_tests' is already taken and is not built by Chromium (as we normally
> > don't want to build the test code in Chromium).
> > 
> > I think it should be 'webrtc_fuzzers', and it should build all our fuzzers.
> That
> > way, we can also set use_libfuzzer=true in our
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/build_overrides/we...
> > so they're built by default in our standalone build. Then we'll get
protection
> > from compilation errors that otherwise would only be discovered during
rolling
> > into WebRTC, since the code isn't compiled.
> > Even if we build all our fuzzers in 'webrtc_fuzzers', the 'gn refs' lookup
> will
> > still work for the Chromium bot.
> > 
> > https://codereview.webrtc.org/1518973003/diff/1/webrtc/BUILD.gn#newcode286
> > webrtc/BUILD.gn:286: group("webrtc_fuzzer_gn_link") {
> > On 2015/12/14 13:07:02, pbos-webrtc wrote:
> > > On 2015/12/14 07:11:37, kjellander (webrtc) wrote:
> > > > I prefer the old name, I don't like the _gn_link suffix
> > > 
> > > Point was to point out that it's not a "real" target, so that no one tries
> to
> > > build it. I've changed it to "webrtc_fuzzers_dummy", because building this
> > > target doesn't give you any fuzzers.
> > 
> > I think it would be useful to have a target we can build, otherwise the code
> > will stop compiling as soon someone commits a mistake, or some new Clang
> > compilation warning is rolled in without the code being actually compiled
for
> > every build.
> 
> Can we consider the gn refs approach for our bots as well? 

Let's chat offline about what to implement on our bots to cover this.

> Then we don't risk breaking chromium libfuzzers due to committing something
that didn't go into the
> fuzzer list (because we missed adding it). 

If you'd implement my suggested approach we would both get a compile target we
can compile in WebRTC, so we avoid breaking compilation, but 'gn refs' would
still work in Chromium.
I'm not sure what you mean could break because something was forgotten being
added. If you add a new fuzzer but forget to add it to BUILD.gn it just won't
compile and won't show up in the libfuzzer bot build (i.e. won't break the
build).

> Otherwise, do you think we can
> enforce the list being up to date through the PRESUBMIT.py script (without
false
> negatives)?

Please explain what list you're referring to here.


> 
> We've already missed adding targets, which is why I don't want to have this
list
> here. It's different from general test targets, since every new fuzzer adds a
> new binary, whereas every test change usually only modifies existing targets.

[kjellander_webrtc, 2015-12-17 10:59:23]

We chatted offline and agreed that the current state is the best solution as it
seems like the 'gn refs' approach is the only way to find and build only the
relevant binaries for the libfuzzer bot.

I'm setting up a similar libfuzzer bot for WebRTC in
https://codereview.chromium.org/1530293002/

So lgtm!

[pbos-webrtc, 2015-12-17 12:27:00]

rebase

[pbos-webrtc, 2015-12-17 12:39:46]

rebase done, stefan ptal

[stefan-webrtc, 2015-12-17 13:20:13]

lgtm

[pbos-webrtc, 2015-12-17 13:28:34]

Description was changed from

==========
Add VP8 and H264 depacketizer fuzzers.

Also removes listing of targets in webrtc_fuzzers which is very prone to
not being up to date. They're not required for ClusterFuzz integration
or building locally. This also means that adding fuzzers won't require
approval outside the fuzzers directory.

BUG=webrtc:4771
R=kjellander@webrtc.org, stefan@webrtc.org
==========

to

==========
Add VP8 and H264 depacketizer fuzzers.

Also removes listing of targets in webrtc_fuzzers which is very prone to
not being up to date. They're not required for ClusterFuzz integration
or building locally. This also means that adding fuzzers won't require
approval outside the fuzzers directory.

BUG=webrtc:4771
R=kjellander@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1e0cfd9a462185ba5c434114c...
==========

[commit-bot: I haz the power, 2015-12-17 13:28:35]

Description was changed from

==========
Add VP8 and H264 depacketizer fuzzers.

Also removes listing of targets in webrtc_fuzzers which is very prone to
not being up to date. They're not required for ClusterFuzz integration
or building locally. This also means that adding fuzzers won't require
approval outside the fuzzers directory.

BUG=webrtc:4771
R=kjellander@webrtc.org, stefan@webrtc.org
==========

to

==========
Add VP8 and H264 depacketizer fuzzers.

Also removes listing of targets in webrtc_fuzzers which is very prone to
not being up to date. They're not required for ClusterFuzz integration
or building locally. This also means that adding fuzzers won't require
approval outside the fuzzers directory.

BUG=webrtc:4771
R=kjellander@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/1e0cfd9a462185ba5c434114cf9dcaf160cd68e3
Cr-Commit-Position: refs/heads/master@{#11067}
==========

[pbos-webrtc, 2015-12-17 13:28:35]

Committed patchset #4 (id:60001) manually as
1e0cfd9a462185ba5c434114cf9dcaf160cd68e3 (presubmit successful).

[commit-bot: I haz the power, 2015-12-17 13:28:37]

Patchset 4 (id:??) landed as
https://crrev.com/1e0cfd9a462185ba5c434114cf9dcaf160cd68e3
Cr-Commit-Position: refs/heads/master@{#11067}