Add fuzzer for PRTime() from NSPR.

Add fuzzer for PRTime() from NSPR.

BUG=none

Committed: https://crrev.com/0531c60c1a299c98fe6685f613c6b213687d5fd9
Cr-Commit-Position: refs/heads/master@{#419255}

[Thiemo Nagel, 2016-09-15 01:21:49]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[Thiemo Nagel, 2016-09-15 01:22:16]

tnagel@chromium.org changed reviewers:
+ mmoroz@chromium.org

[Thiemo Nagel, 2016-09-15 01:22:17]

Hi Max,

could you please take a look?

Thank you,
Thiemo

[commit-bot: I haz the power, 2016-09-15 01:22:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kcc2, 2016-09-15 01:26:47]

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

[kcc2, 2016-09-15 01:26:48]

testing/libfuzzer/fuzzers is the location for some examples,
but I'd prefer to have new fuzz targets closer to the code they are fuzzing.

[commit-bot: I haz the power, 2016-09-15 03:07:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 03:07:26]

Dry run: This issue passed the CQ dry run.

[mmoroz, 2016-09-15 15:02:37]

LGTM for fuzzer

btw, the target looks pretty interesting, many calculations with pointers :)

and +1 to kcc@'s comment: would you mind moving that to
src/base/third_party/nspr/?

we usually create a subdirectory named "fuzz" and put source code/dictionary
there. Given that the nearest BUILD.gn file is src/base/BUILD.gn, I don't mind
to leave build configuration here.

[Thiemo Nagel, 2016-09-15 22:42:47]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 22:43:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-09-15 22:46:54]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 22:47:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-09-15 22:47:12]

Patchset #2 (id:20001) has been deleted

[Thiemo Nagel, 2016-09-15 22:48:41]

tnagel@chromium.org changed reviewers:
+ danakj@chromium.org

[Thiemo Nagel, 2016-09-15 22:48:41]

Hi Dana,

could you please do an OWNER review for moving the files to //base?  The code
itself is already approved by the fuzzer folks.

Thank you,
Thiemo

[danakj, 2016-09-15 23:51:01]

https://codereview.chromium.org/2346723002/diff/40001/testing/libfuzzer/fuzze...
File testing/libfuzzer/fuzzers/BUILD.gn (right):

https://codereview.chromium.org/2346723002/diff/40001/testing/libfuzzer/fuzze...
testing/libfuzzer/fuzzers/BUILD.gn:342:
"//base/third_party/nspr/fuzz/prtime_fuzzer.cc",
It looks like the sources for every other target are in this current directory,
why is this one different?

[commit-bot: I haz the power, 2016-09-15 23:51:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 23:51:31]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-09-15 23:56:53]

> It looks like the sources for every other target are in this current
directory,
> why is this one different?

BUILD.gn in testing/libfuzzer/fuzzers says at the top: "Individual libfuzzer
tests that didn't find their home yet."  This seems to be true, a quick search
turns up ~100 fuzzers living outside that directory:

https://cs.chromium.org/search/?q=file:.*_fuzzer.cc+-file:src/testing/libfuzz...

[danakj, 2016-09-16 00:24:02]

On Thu, Sep 15, 2016 at 4:56 PM, <tnagel@chromium.org> wrote:

> > It looks like the sources for every other target are in this current
> directory,
> > why is this one different?
>
> BUILD.gn in testing/libfuzzer/fuzzers says at the top: "Individual
> libfuzzer
> tests that didn't find their home yet." This seems to be true, a quick
> search
> turns up ~100 fuzzers living outside that directory:
>
> https://cs.chromium.org/search/?q=file:.*_fuzzer.cc+-
> file:src/testing/libfuzzer/fuzzers&sq=package:chromium&type=cs
>

Hm, then why are you using this BUILD.gn file? It seems a bit weird to have
a BUILD.gn referencing sources in a completely diff directory, that doesn't
seem to be how we've structured our use of gn so far. I'd expect a BUILD.gn
where the sources are.

Also I thought base/third_party/ was for things that are used by base,
which this doesn't fit, it depends on base instead. Idk maybe you can
explain the reasoning behind why that is the right home?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Thiemo Nagel, 2016-09-16 00:43:33]

> Hm, then why are you using this BUILD.gn file? It seems a bit weird to have
> a BUILD.gn referencing sources in a completely diff directory, that doesn't
> seem to be how we've structured our use of gn so far. I'd expect a BUILD.gn
> where the sources are.

I guess I can't use base/BUILD.gn because that would introduce a dependency on
testing/libfuzzer, right?

> Also I thought base/third_party/ was for things that are used by base,
> which this doesn't fit, it depends on base instead. Idk maybe you can
> explain the reasoning behind why that is the right home?

kcc2 has requested it.

I guess we have to pick one of two evils:
[1] keep the fuzzer separate from the code that it's fuzzing
[2] have BUILD.gn use files from a different top-level directory

I'd probably go for [2] to keep the code and it's fuzzers together, but if Dana
doesn't approve, Kostya, Max, would you be willing to reconsider?

[danakj, 2016-09-16 01:01:50]

On Thu, Sep 15, 2016 at 5:43 PM, <tnagel@chromium.org> wrote:

> > Hm, then why are you using this BUILD.gn file? It seems a bit weird to
> have
> > a BUILD.gn referencing sources in a completely diff directory, that
> doesn't
> > seem to be how we've structured our use of gn so far. I'd expect a
> BUILD.gn
> > where the sources are.
>
> I guess I can't use base/BUILD.gn because that would introduce a
> dependency on
> testing/libfuzzer, right?
>

Only for new the target. I think if you want this to be by the code it
tests, I'd prefer that it really did so the way unit tests do. And if it's
going to be at a distance (in a fuzz subdir), I'd prefer it live with the
other fuzzers to be findable that way at least.

Concretely that means
1) move them to testing/libfuzzer/fuzzers/ where the BUILD target is
defined, or
2) put the BUILD target in the same file as the sources are, just like
that's where base_unittests is.
2b) What do you think of not putting them in a fuzz/ subdir and just name
them prtime_fuzztest.cc prtime_fuzztest.dict? That should be clearly in
line with the would-be prtime_unittest.cc pattern?


>
>
> > Also I thought base/third_party/ was for things that are used by base,
> > which this doesn't fit, it depends on base instead. Idk maybe you can
> > explain the reasoning behind why that is the right home?
>
> kcc2 has requested it.
>
> I guess we have to pick one of two evils:
> [1] keep the fuzzer separate from the code that it's fuzzing
> [2] have BUILD.gn use files from a different top-level directory
>
> I'd probably go for [2] to keep the code and it's fuzzers together, but if
> Dana
> doesn't approve, Kostya, Max, would you be willing to reconsider?
>
> https://codereview.chromium.org/2346723002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmoroz, 2016-09-16 07:57:16]

On 2016/09/16 01:01:50, danakj wrote:
> On Thu, Sep 15, 2016 at 5:43 PM, <mailto:tnagel@chromium.org> wrote:
> 
> > > Hm, then why are you using this BUILD.gn file? It seems a bit weird to
> > have
> > > a BUILD.gn referencing sources in a completely diff directory, that
> > doesn't
> > > seem to be how we've structured our use of gn so far. I'd expect a
> > BUILD.gn
> > > where the sources are.
> >
> > I guess I can't use base/BUILD.gn because that would introduce a
> > dependency on
> > testing/libfuzzer, right?
> >
> 
> Only for new the target. I think if you want this to be by the code it
> tests, I'd prefer that it really did so the way unit tests do. And if it's
> going to be at a distance (in a fuzz subdir), I'd prefer it live with the
> other fuzzers to be findable that way at least.
> 
> Concretely that means
> 1) move them to testing/libfuzzer/fuzzers/ where the BUILD target is
> defined, or
> 2) put the BUILD target in the same file as the sources are, just like
> that's where base_unittests is.
> 2b) What do you think of not putting them in a fuzz/ subdir and just name
> them prtime_fuzztest.cc prtime_fuzztest.dict? That should be clearly in
> line with the would-be prtime_unittest.cc pattern?
> 
> 
> >
> >
> > > Also I thought base/third_party/ was for things that are used by base,
> > > which this doesn't fit, it depends on base instead. Idk maybe you can
> > > explain the reasoning behind why that is the right home?
> >
> > kcc2 has requested it.
> >
> > I guess we have to pick one of two evils:
> > [1] keep the fuzzer separate from the code that it's fuzzing
> > [2] have BUILD.gn use files from a different top-level directory
> >
> > I'd probably go for [2] to keep the code and it's fuzzers together, but if
> > Dana
> > doesn't approve, Kostya, Max, would you be willing to reconsider?
> >
> > https://codereview.chromium.org/2346723002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thiemo, please feel free to land Patch Set #1. I didn't know that moving the
source could take so much time, we definitely should spend our time as efficient
as possible (i.e. rather write another fuzzer, than negotiate file locations and
names).

FWIW:
- We have 40 targets in testing/libfuzzer/fuzzers/BUILD.gn, though only 30
source code files. There are literally "tests that didn't find their home yet".
- Adding base/third_party/nspr/fuzz/BUILD.gn won't work, we have to add a
dependency in some of BUILD.gn in its parent directories as well, otherwise GN
won't discover our target -> too much noise and hard to support if we will
move/delete that.
- Adding that target to anyone of existing BUILD.gn files under //base is equal
to PS#2, because the build target will be stored separately from the sources in
both cases.
- I'm very surprised that src/base/third_party/nspr/prtime.cc doesn't have a
BUILD.gn right there, but it is expected from our fuzzer. I haven't seen any
special requirements regarding fuzzers.
- "_fuzzer.cc" prefix is much more popular than "_fuzzertest.cc" (127 vs 9), and
I prefer to follow that way. A test is something strictly defined and finite,
while fuzzer works randomly and infinitely. A test has an expected result that
is also strictly defined, while fuzzer has not (for example, one may say that it
is expected for fuzzer to do not crash, I would say that fuzzer is expected to
crash :), and so on). But that's just a terminology dispute, I don't mind if
people name files as they want inside their projects.

[Thiemo Nagel, 2016-09-16 18:50:17]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-09-16 18:50:17]

The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org
Link to the patchset: https://codereview.chromium.org/2346723002/#ps60001
(title: "Revert PS #2.")

[commit-bot: I haz the power, 2016-09-16 18:50:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-09-16 18:51:25]

> Thiemo, please feel free to land Patch Set #1.

Thanks, doing that.

[danakj, 2016-09-16 18:56:57]

On Fri, Sep 16, 2016 at 12:57 AM, <mmoroz@chromium.org> wrote:

> On 2016/09/16 01:01:50, danakj wrote:
>
> > On Thu, Sep 15, 2016 at 5:43 PM, <mailto:tnagel@chromium.org> wrote:
> >
> > > > Hm, then why are you using this BUILD.gn file? It seems a bit weird
> to
> > > have
> > > > a BUILD.gn referencing sources in a completely diff directory, that
> > > doesn't
> > > > seem to be how we've structured our use of gn so far. I'd expect a
> > > BUILD.gn
> > > > where the sources are.
> > >
> > > I guess I can't use base/BUILD.gn because that would introduce a
> > > dependency on
> > > testing/libfuzzer, right?
> > >
> >
> > Only for new the target. I think if you want this to be by the code it
> > tests, I'd prefer that it really did so the way unit tests do. And if
> it's
> > going to be at a distance (in a fuzz subdir), I'd prefer it live with the
> > other fuzzers to be findable that way at least.
> >
> > Concretely that means
> > 1) move them to testing/libfuzzer/fuzzers/ where the BUILD target is
> > defined, or
> > 2) put the BUILD target in the same file as the sources are, just like
> > that's where base_unittests is.
> > 2b) What do you think of not putting them in a fuzz/ subdir and just name
> > them prtime_fuzztest.cc prtime_fuzztest.dict? That should be clearly in
> > line with the would-be prtime_unittest.cc pattern?
> >
> >
> > >
> > >
> > > > Also I thought base/third_party/ was for things that are used by
> base,
> > > > which this doesn't fit, it depends on base instead. Idk maybe you can
> > > > explain the reasoning behind why that is the right home?
> > >
> > > kcc2 has requested it.
> > >
> > > I guess we have to pick one of two evils:
> > > [1] keep the fuzzer separate from the code that it's fuzzing
> > > [2] have BUILD.gn use files from a different top-level directory
> > >
> > > I'd probably go for [2] to keep the code and it's fuzzers together,
> but if
> > > Dana
> > > doesn't approve, Kostya, Max, would you be willing to reconsider?
> > >
> > > https://codereview.chromium.org/2346723002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
> Thiemo, please feel free to land Patch Set #1. I didn't know that moving
> the
> source could take so much time, we definitely should spend our time as
> efficient
> as possible (i.e. rather write another fuzzer, than negotiate file
> locations and
> names).
>
> FWIW:
> - We have 40 targets in testing/libfuzzer/fuzzers/BUILD.gn, though only 30
> source code files. There are literally "tests that didn't find their home
> yet".
> - Adding base/third_party/nspr/fuzz/BUILD.gn won't work, we have to add a
> dependency in some of BUILD.gn in its parent directories as well,
> otherwise GN
> won't discover our target -> too much noise and hard to support if we will
> move/delete that.
>

Yeah, I didn't mean for that since that's not where the build rule is for
the prtime.cc is.


> - Adding that target to anyone of existing BUILD.gn files under //base is
> equal
> to PS#2, because the build target will be stored separately from the
> sources in
> both cases.
> - I'm very surprised that src/base/third_party/nspr/prtime.cc doesn't
> have a
> BUILD.gn right there, but it is expected from our fuzzer. I haven't seen
> any
> special requirements regarding fuzzers.
>

Ya I was surprised too, but I'm happy with having the build target in the
same place as the build target for the .cc file, which is in base/BUILD.gn.
Keeping them together seems good to me.


> - "_fuzzer.cc" prefix is much more popular than "_fuzzertest.cc" (127 vs
> 9), and
> I prefer to follow that way. A test is something strictly defined and
> finite,
> while fuzzer works randomly and infinitely. A test has an expected result
> that
> is also strictly defined, while fuzzer has not (for example, one may say
> that it
> is expected for fuzzer to do not crash, I would say that fuzzer is
> expected to
> crash :), and so on). But that's just a terminology dispute, I don't mind
> if
> people name files as they want inside their projects.
>

That makes sense. I guess I'd just like the .dict file to match the same
name so it's clear that it's part of the fuzzer from looking at the
directory.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-09-16 19:47:32]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-16 19:49:11]

Description was changed from

==========
Add fuzzer for PRTime() from NSPR.

BUG=none
==========

to

==========
Add fuzzer for PRTime() from NSPR.

BUG=none

Committed: https://crrev.com/0531c60c1a299c98fe6685f613c6b213687d5fd9
Cr-Commit-Position: refs/heads/master@{#419255}
==========

[commit-bot: I haz the power, 2016-09-16 19:49:12]

Patchset 3 (id:??) landed as
https://crrev.com/0531c60c1a299c98fe6685f613c6b213687d5fd9
Cr-Commit-Position: refs/heads/master@{#419255}