This is an archive of the discontinued LLVM Phabricator instance.

Refactored clang-fuzzer and added new (copy) files
ClosedPublic

Authored by emmettneyman on Jun 1 2018, 4:42 PM.

Diff Detail

Event Timeline

emmettneyman created this revision.Jun 1 2018, 4:42 PM
  • Took out a debug print statement

Good practice is to avoid merging changes into a single one.
Here one patch should be "refactoring" and the second for "loop-proto-fuzzer."

tools/clang-fuzzer/ExampleClangProtoFuzzer.cpp
29

Please delete commented code.

tools/clang-fuzzer/FuzzerInitialize.h
12

static here means that each module which includes this header is going to have own instance of the variable.
I guess you need only one instance in FuzzerInitialize.cpp which can be achieved with

extern std::vector<const char *> CLArgs;

However I'd recommend getter:

const std::vector<const char *>& GetCLArgs();

with implementation and

static std::vector<const char *> CLArgs;

in FuzzerInitialize.cpp

13

you need only CLArgs here.
code which includes this header is not going to call LLVMFuzzerInitialize

tools/clang-fuzzer/experimental/ExampleClangLoopProtoFuzzer.cpp
30

Please remove deleted code

Good practice is to avoid merging changes into a single one.
Here one patch should be "refactoring" and the second for "loop-proto-fuzzer."

We are doing this for several reasons:

  1. smaller patches, faster review
  2. easier to investigate regressions caused by smaller patches
emmettneyman updated this revision to Diff 149807.EditedJun 4 2018, 11:01 AM

Changed CLArgs into getter and deleted commented code

Made changes in response to comments
Removed commented out code
Changed CLArgs to be a getter method
Removed LLVMFuzzerInitialize decl from header file

vitalybuka requested changes to this revision.Jun 4 2018, 1:10 PM
vitalybuka added inline comments.
tools/clang-fuzzer/FuzzerInitialize.cpp
11

Could you please update this description?

tools/clang-fuzzer/experimental/ExampleClangLoopProtoFuzzer.cpp
22

Please move ExampleClangLoopProtoFuzzer into a separate patch

This revision now requires changes to proceed.Jun 4 2018, 1:10 PM
  • Updated and added header comments to two new files. Deleted loop fuzzer files.
  • Fixed file header comment
  • Another edit to the file header comments.
  • Updated and added header comments to two new files. Deleted loop fuzzer files.

I will commit the loop fuzzer files in a future patch.

morehouse added inline comments.Jun 4 2018, 3:11 PM
tools/clang-fuzzer/CMakeLists.txt
48

Rather than compiling FuzzerInitialize.cpp into the binary, can we make it a library like handle-cxx or proto-to-cxx?

tools/clang-fuzzer/FuzzerInitialize.cpp
17–18

Do we need this include?

tools/clang-fuzzer/FuzzerInitialize.h
21

Why do we need these includes in the header? Doesn't look like they're used here.

  • Refactored FuzzerInitialize into library
vitalybuka accepted this revision.Jun 4 2018, 4:10 PM
This revision is now accepted and ready to land.Jun 4 2018, 4:10 PM
morehouse added inline comments.Jun 4 2018, 4:13 PM
tools/clang-fuzzer/ExampleClangProtoFuzzer.cpp
24

I think cstring is no longer used after this change. So we can probably remove this include.

tools/clang-fuzzer/fuzzer-initialize/CMakeLists.txt
3 ↗(On Diff #149867)

Nit: clangFuzzerInitialize would better follow the naming convention of the other libraries.

tools/clang-fuzzer/fuzzer-initialize/fuzzer_initialize.h
15 ↗(On Diff #149867)

I don't think this include is used in this file either. Can we remove it?

  • Removed unecessary includes and renamed library.
morehouse accepted this revision.Jun 4 2018, 4:37 PM

LGTM.

This revision was automatically updated to reflect the committed changes.
kcc added a comment.Jun 7 2018, 3:08 PM

Some feedback on the generated code:

while (1){

let's not have the while loops inside the for loops for now.
If the initial goal is to stress the loop optimizations (e.g. vectorizer),
loops likes this are just a distraction

for (int loop_ctr = 0

too verbose. Use 'i'm 'j', 'k'
also, alternate int, unsigned, size_t, long (later).

a[436863498 % s]=1;

this is good for keeping the code UB-free, but it will render the tests non-vectorizable.
need more tests that won't use % s

void foo(int *a, size_t s) {

ok for a starter, but will need a more rich signature in the future versions.