Titouan Rigoudy (Gerrit)

unread,

Jun 4, 2024, 10:10:25 AMJun 4

to Adrian Taylor, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Adrian Taylor

Titouan Rigoudy voted and added 3 comments

Votes added by Titouan Rigoudy

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 29 (Latest):

Titouan Rigoudy . resolved

LGTM. It would have been nice to have a way to register initialization functions in a generic way for test_suite.cc to call later, but this works!

File base/test/test_suite.h

Line 121, Patchset 29 (Latest): // We need argv_as_pointers_.data() to have type char**, so we can't use

Titouan Rigoudy . unresolved

```suggestion
  // We need fuzztest_argv_raw_.data() to have type char**, so we can't use
```

File third_party/fuzztest/maybe_init_fuzztest.h

Line 8, Patchset 29 (Latest):extern void (*initialization_function)(int* argc, char*** argv);

Titouan Rigoudy . unresolved

Should we put this in a namespace? It seems ripe for collisions.

Open in Gerrit

Related details

Attention is currently required from:

Adrian Taylor

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Adrian Taylor (Gerrit)

unread,

Jun 4, 2024, 10:38:33 AMJun 4

to Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Adrian Taylor added 2 comments

File base/test/test_suite.h

Line 121, Patchset 29: // We need argv_as_pointers_.data() to have type char**, so we can't use

Titouan Rigoudy . resolved

```suggestion
  // We need fuzztest_argv_raw_.data() to have type char**, so we can't use
```

Adrian Taylor

Done

File third_party/fuzztest/maybe_init_fuzztest.h

Line 8, Patchset 29:extern void (*initialization_function)(int* argc, char*** argv);

Titouan Rigoudy . resolved

Should we put this in a namespace? It seems ripe for collisions.

Adrian Taylor

Done

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Titouan Rigoudy (Gerrit)

unread,

Jun 4, 2024, 11:08:25 AMJun 4

to Adrian Taylor, Daniel Cheng, Ben Pastene, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Adrian Taylor, Ben Pastene and Daniel Cheng

Titouan Rigoudy voted and added 2 comments

Votes added by Titouan Rigoudy

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 30 (Latest):

Titouan Rigoudy . resolved

Still LGTM, with one more optional comment.

File third_party/fuzztest/maybe_init_fuzztest.h

Line 8, Patchset 30 (Latest):namespace maybe_fuzztest {

Titouan Rigoudy . unresolved

I think I would put everything in the `fuzztest` namespace, except maybe this initialization function in `fuzztest_internal`, to make it clear that users of this header should not mess with this. This works, though.

Open in Gerrit

Related details

Attention is currently required from:

Adrian Taylor
Ben Pastene
Daniel Cheng

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Adrian Taylor (Gerrit)

unread,

Jun 4, 2024, 11:11:38 AMJun 4

to Daniel Cheng, Ben Pastene, Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Ben Pastene and Daniel Cheng

Adrian Taylor added 1 comment

File third_party/fuzztest/maybe_init_fuzztest.h

Line 8, Patchset 30 (Latest):namespace maybe_fuzztest {

Titouan Rigoudy . resolved

I think I would put everything in the `fuzztest` namespace, except maybe this initialization function in `fuzztest_internal`, to make it clear that users of this header should not mess with this. This works, though.

Adrian Taylor

Thanks, but I'm going to decline to do that because the `fuzztest` namespace is owned by the fuzztest team not us, and the `fuzztest_internal` namespace feels like it would be even more so (though it doesn't exist right now). This is kind of meta-fuzztest code owned by Chromium. So this seemed like the least bad choice.

Open in Gerrit

Related details

Attention is currently required from:

Ben Pastene
Daniel Cheng

Submit Requirements:

Code-Coverage
Code-Review

satisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

Jun 4, 2024, 3:52:49 PMJun 4

to Adrian Taylor, Daniel Cheng, Ben Pastene, Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Adrian Taylor and Ben Pastene

Daniel Cheng added 6 comments

Commit Message

Line 13, Patchset 30 (Latest):unit test targets (though this is still not encouaged by Chromium's

Daniel Cheng . unresolved

encouraged

Line 17, Patchset 30 (Latest):This extra call in base::TestSuite sets up FuzzTest ready.

Daniel Cheng . unresolved

This sentence doesn't quite parse; wonder if this is missing a word?

File base/test/test_suite.cc

Line 670, Patchset 30 (Latest): // changed it.

Daniel Cheng . resolved

So random Chromium code is changing the original argc/argv??

That's... fun.

File third_party/fuzztest/BUILD.gn

Line 362, Patchset 30 (Latest): "really_init_fuzztest.cc",

Daniel Cheng . unresolved

I hate to suggest this, because renaming files will remove CR+1s but I don't love the current names.

init_helper.{cc,h} for the "maybe" stuff
"something else" for really_init_fuzztest (though I have no idea what)

File third_party/fuzztest/maybe_init_fuzztest.h

Line 8, Patchset 30 (Latest):namespace maybe_fuzztest {

Titouan Rigoudy . resolved

I think I would put everything in the `fuzztest` namespace, except maybe this initialization function in `fuzztest_internal`, to make it clear that users of this header should not mess with this. This works, though.

Adrian Taylor

Thanks, but I'm going to decline to do that because the `fuzztest` namespace is owned by the fuzztest team not us, and the `fuzztest_internal` namespace feels like it would be even more so (though it doesn't exist right now). This is kind of meta-fuzztest code owned by Chromium. So this seemed like the least bad choice.

Daniel Cheng

How about something like `fuzztest_support`?

File third_party/fuzztest/really_init_fuzztest.cc

Line 25, Patchset 30 (Latest):FuzztestInitializer _static_initializer;

Daniel Cheng . unresolved

Is this a special name that fuzztest requires?

Otherwise, we should probably avoid names starting with a leading `_`; I believe these are reserved for the implementation.

Open in Gerrit

Related details

Attention is currently required from:

Adrian Taylor
Ben Pastene

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

satisfied_requirement

unsatisfied_requirement

open

diffy

Ben Pastene (Gerrit)

unread,

Jun 4, 2024, 6:52:50 PMJun 4

to Adrian Taylor, Daniel Cheng, Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Adrian Taylor

Ben Pastene voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Adrian Taylor

satisfied_requirement

unsatisfied_requirement

open

diffy

Adrian Taylor (Gerrit)

unread,

Jun 5, 2024, 9:20:55 AMJun 5

to Ben Pastene, Daniel Cheng, Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Ben Pastene, Daniel Cheng and Titouan Rigoudy

Adrian Taylor added 5 comments

Commit Message

Line 13, Patchset 30:unit test targets (though this is still not encouaged by Chromium's

Daniel Cheng . resolved

encouraged

Adrian Taylor

Done

Line 17, Patchset 30:This extra call in base::TestSuite sets up FuzzTest ready.

Daniel Cheng . resolved

This sentence doesn't quite parse; wonder if this is missing a word?

Adrian Taylor

Done

File third_party/fuzztest/BUILD.gn

Line 362, Patchset 30: "really_init_fuzztest.cc",

Daniel Cheng . resolved

I hate to suggest this, because renaming files will remove CR+1s but I don't love the current names.
init_helper.{cc,h} for the "maybe" stuff
"something else" for really_init_fuzztest (though I have no idea what)

Adrian Taylor

Done. I went with `confirm_init.cc` for `really_init_fuzztest.cc`

File third_party/fuzztest/maybe_init_fuzztest.h

Line 8, Patchset 30:namespace maybe_fuzztest {

Titouan Rigoudy . resolved

I think I would put everything in the `fuzztest` namespace, except maybe this initialization function in `fuzztest_internal`, to make it clear that users of this header should not mess with this. This works, though.

Adrian Taylor

Thanks, but I'm going to decline to do that because the `fuzztest` namespace is owned by the fuzztest team not us, and the `fuzztest_internal` namespace feels like it would be even more so (though it doesn't exist right now). This is kind of meta-fuzztest code owned by Chromium. So this seemed like the least bad choice.

Daniel Cheng

How about something like `fuzztest_support`?

Adrian Taylor

I chose `fuzztest_init_helper`

File third_party/fuzztest/really_init_fuzztest.cc

Line 25, Patchset 30:FuzztestInitializer _static_initializer;

Daniel Cheng . resolved

Is this a special name that fuzztest requires?
Otherwise, we should probably avoid names starting with a leading `_`; I believe these are reserved for the implementation.

Adrian Taylor

Done

Open in Gerrit

Related details

Attention is currently required from:

Ben Pastene
Daniel Cheng
Titouan Rigoudy

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

Jun 5, 2024, 1:42:52 PMJun 5

to Adrian Taylor, Daniel Cheng, Ben Pastene, Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Adrian Taylor, Ben Pastene and Titouan Rigoudy

Daniel Cheng voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Adrian Taylor
Ben Pastene
Titouan Rigoudy

Submit Requirements:

Code-Coverage
Code-Review

satisfied_requirement

open

diffy

Adrian Taylor (Gerrit)

unread,

Jun 6, 2024, 2:15:19 AMJun 6

to Daniel Cheng, Ben Pastene, Titouan Rigoudy, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, Chromium LUCI CQ, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Attention needed from Ben Pastene and Titouan Rigoudy

Adrian Taylor voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Ben Pastene
Titouan Rigoudy

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 6, 2024, 2:18:53 AMJun 6

to Adrian Taylor, Daniel Cheng, Ben Pastene, Titouan Rigoudy, Akihiro Ota, chromotin...@chromium.org, Hans Wennborg, Kentaro Hara, Nate Chapin, (Julie)Jeongeun Kim, Kevin Babbitt, Peter Beverloo, Rijubrata Bhaumik, Sadrul Chowdhury, Ian Vollick, chromium...@chromium.org, abigailbk...@google.com, antoniosartori+wa...@chromium.org, blink-re...@chromium.org, cblume...@chromium.org, cblum...@chromium.org, cc-...@chromium.org, chfreme...@chromium.org, chromeos-gfx-...@google.com, dtseng...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, francisjp...@google.com, gavinp...@chromium.org, hirokisa...@chromium.org, huangs...@chromium.org, jbauma...@chromium.org, jophba...@chromium.org, josiah...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, kyungjunle...@google.com, loading...@chromium.org, media-cro...@chromium.org, mfoltz...@chromium.org, mpdento...@chromium.org, nektar...@chromium.org, net-r...@chromium.org, oilpan-rev...@chromium.org, oshima...@chromium.org, ozone-...@chromium.org, pdf-r...@chromium.org, penghuan...@chromium.org, poscia...@chromium.org, roblia...@chromium.org, rsesek...@chromium.org, sky+...@chromium.org, vaapi-...@chromium.org, wfh+...@chromium.org, yuzo+...@chromium.org, dominicc+...@chromium.org, blink-...@chromium.org, blundell+...@chromium.org, fuzzin...@chromium.org, droger+w...@chromium.org, fuz...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Initialize FuzzTest for all Chromium tests.

FUZZ_TEST is a new type of test which is half way between a unit test
and a fuzzer (the same code can run in two modes).

Eventually, we hope to allow their free inclusion in existing Chromium
unit test targets (though this is still not encouraged by Chromium's
fuzzing documentation as there are a number of things we need to resolve
first).

This extra call in base::TestSuite sets up FuzzTest ready for this.

base/test:test_support is included in all sorts of test targets, not
just test suites. And, of the test suites where it ends up being
included, some of them will have fuzztests and some of them won't. This
CL is heavily structured such that the new code impacts only test suites
which have fuzztests.

Specifically,

* base/test:test_support ends up being included in various pre-existing
  fuzzers. This is a problem because those fuzzers might
  have implementations of core fuzzing functions such as
  LLVMFuzzerCustomMutator, which would conflict with the fuzztest
  implementation. We therefore take pains not to include
  src/fuzztest/internal/compatibility_mode.cc except if the test suite
  really does have fuzztests.
* base/test:test_support is sometimes linked from targets that include
  protobuf_full (notably some GPU test targets). fuzztest depends on
  protobuf_lite. On component builds this causes a conflict - a build
  failure on Windows and ODR violations on other platforms. We therefore
  need to avoid including fuzztests' dependencies into
  base/test:test_support unless there really are fuzztests in the test
  suite.

Fortunately, testing/test.gni already knows whether there are fuzztests
in a given test suite, as it needs to generate wrapper executables for
each fuzztest.

We therefore use dependency injection, where base/test:test_support calls
a MaybeInitFuzztest function. In test suites containing real fuzztests,
a function pointer will have been populated with a real function that
can initialize fuzztests.

A couple of other details:
* Fuzztest takes the liberty of looking at its command line arguments
  "later". Chromium turns out to modify its command line, and thus the
  fuzztest command line would become invalid. We store a copy to avoid
  this problem.
* When test suites are running in fuzzing mode, we avoid going into
  multiple process test mode.

Cq-Include-Trybots: luci.chromium.try:linux-libfuzzer-asan-rel,win-libfuzzer-asan-rel,linux-centipede-asan-rel

Bug: 40286211

Change-Id: I9456ffd6b273c0e32fd88abd8547bf7f9cad96a3

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4981917

Reviewed-by: Daniel Cheng <dch...@chromium.org>

Commit-Queue: Adrian Taylor <adet...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1311096}

Files:

M base/test/BUILD.gn
M base/test/launcher/unit_test_launcher.cc
M base/test/test_suite.cc
M base/test/test_suite.h
M base/test/test_switches.cc
M base/test/test_switches.h
M testing/test.gni
M third_party/fuzztest/BUILD.gn
A third_party/fuzztest/confirm_init.cc
A third_party/fuzztest/init_helper.cc
A third_party/fuzztest/init_helper.h

Change size: M

Delta: 11 files changed, 140 insertions(+), 8 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Daniel Cheng

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Initialize FuzzTest for all Chromium tests. [chromium/src : main]

Titouan Rigoudy (Gerrit)

Titouan Rigoudy voted and added 3 comments

Votes added by Titouan Rigoudy

3 comments

Related details

Adrian Taylor (Gerrit)

Adrian Taylor added 2 comments

Related details

Titouan Rigoudy (Gerrit)

Titouan Rigoudy voted and added 2 comments

Votes added by Titouan Rigoudy

2 comments

Related details

Adrian Taylor (Gerrit)

Adrian Taylor added 1 comment

Related details

Daniel Cheng (Gerrit)

Daniel Cheng added 6 comments

Related details

Ben Pastene (Gerrit)

Ben Pastene voted Code-Review+1

Related details

Adrian Taylor (Gerrit)

Adrian Taylor added 5 comments

Related details

Daniel Cheng (Gerrit)

Daniel Cheng voted Code-Review+1

Related details

Adrian Taylor (Gerrit)

Adrian Taylor voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information