kcc added a reviewer: bogner. kcc added a comment. +bogner@ FYI
================ Comment at: clang/tools/clang-fuzzer/ExampleClangProtoFuzzer.cpp:25 + +static void MaybePrint(const std::string &S) { + static const char *env = getenv("CXXFUZZ_PRINT"); ---------------- this is debug code, not worth having here, plz remove ================ Comment at: clang/tools/clang-fuzzer/ExampleClangProtoFuzzer.cpp:34 + MaybePrint(S); + HandleCXX(S, {"-O2", "-mllvm", "-scalar-evolution-max-arith-depth=4"}); + if (getenv("CXX_FUZZ_MORE")) { ---------------- Remove "-mllvm", "-scalar-evolution-max-arith-depth=4". It's there as a workaround for a performance bug (https://bugs.llvm.org/show_bug.cgi?id=33494) but it shouldn't be here. ================ Comment at: clang/tools/clang-fuzzer/ExampleClangProtoFuzzer.cpp:35 + HandleCXX(S, {"-O2", "-mllvm", "-scalar-evolution-max-arith-depth=4"}); + if (getenv("CXX_FUZZ_MORE")) { + HandleCXX(S, {"-O1", "-triple", "arm-apple-darwin10", "-mllvm", ---------------- Remove this section. In a later change, please allow to change the tripple (and any other flags) similar to https://reviews.llvm.org/D36275 ================ Comment at: clang/tools/clang-fuzzer/cxx_proto.proto:16 + +syntax = "proto2"; +//option cc_api_version = 2; ---------------- vitalybuka wrote: > vitalybuka wrote: > > I'd suggest proto3 > proto3 has no required, to avoid backward compatibility issues. > Same is useful for us, we don't wont to discard corpus if we drop some field > in the future. I'm afraid it's much more convenient to have 'required' here. How else could you express a binary op node? ================ Comment at: clang/tools/clang-fuzzer/cxx_proto.proto:93 +} + +package clang_fuzzer; ---------------- vitalybuka wrote: > morehouse wrote: > > vitalybuka wrote: > > > message CxxInput { > > > required Function f = 1; > > > required int/enum opt_level = 2; > > > required enum tripple = 3; > > > required scalar-evolution-max-arith-depth ... > > > } > > Interesting idea. This would allow for protobuf-mutator to choose > > different option combinations, if I understand correctly. > > > > Is that worth adding to this initial patch, though? > yes, instead of CXX_FUZZ_MORE For now, keep it as is, please (see my other comment about flags) https://reviews.llvm.org/D36324 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits