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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
