Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2418763004:
Add SKSL fuzzer (Closed)
DescriptionAdd SKSL fuzzer
BUG=skia:5490
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2418763004
Committed: https://skia.googlesource.com/skia/+/e719577fe8ac3de38795cde2007337f854d97435
Patch Set 1 #Patch Set 2 : Add mustForceNegatedAtanParamToFloat #
Total comments: 4
Patch Set 3 : Rework ifs #Messages
Total messages: 20 (11 generated)
Description was changed from ========== First try at SKSL fuzzer BUG= ========== to ========== First try at SKSL fuzzer BUG= GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2418763004 ==========
The CQ bit was checked by kjlubick@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kjlubick@google.com changed reviewers: + ethannicholas@google.com
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/2418763004/diff/20001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/2418763004/diff/20001/fuzz/fuzz.cpp#newcode74 fuzz/fuzz.cpp:74: if (FLAGS_type[0][2] == 's') { //sksl2glsl It may be time to rewrite this parsing. It's getting kind of complex now, and it'd be easier to read with strings, e.g. ... if (0 == strcmp("skp" , FLAGS_type[0]) { return fuzz_skp (bytes); } if (0 == strcmp("sksl2glsl", FLAGS_type[0]) { return fuzz_sksl2glsl(bytes); } ... Side note: I just had to type sksl2glsl a couple times and it's a complete finger twister. It can probably be something pithier like sksl? https://codereview.chromium.org/2418763004/diff/20001/fuzz/fuzz.cpp#newcode425 fuzz/fuzz.cpp:425: reinterpret_cast<const char*>(bytes->data()), default_caps(), &output); This probably shouldn't be reinterpret_cast. It'll work fine but it gives off a strong "I am doing something weird and byte-punny here" vibe. Really we're just turning bytes into bytes... nothing weird. Try (const char*)bytes->data(), or maybe just bytes->bytes() if that works?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benjaminwagner@google.com changed reviewers: + benjaminwagner@google.com - mtklein@chromium.org
Would it be easy to run the output through a GLSL validator?
What GLSL validator did you have in mind? And yes, that could probably be added, with an abort if the validator returns false to catch any validation errors. https://codereview.chromium.org/2418763004/diff/20001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/2418763004/diff/20001/fuzz/fuzz.cpp#newcode74 fuzz/fuzz.cpp:74: if (FLAGS_type[0][2] == 's') { //sksl2glsl On 2016/10/14 at 15:26:18, mtklein_C wrote: > It may be time to rewrite this parsing. It's getting kind of complex now, and it'd be easier to read with strings, e.g. > > ... > if (0 == strcmp("skp" , FLAGS_type[0]) { return fuzz_skp (bytes); } > if (0 == strcmp("sksl2glsl", FLAGS_type[0]) { return fuzz_sksl2glsl(bytes); } > ... > > Side note: I just had to type sksl2glsl a couple times and it's a complete finger twister. It can probably be something pithier like sksl? Done. I'm keeping sksl2glsl because I imagine we'll want to have sksl2spirv in the near future. https://codereview.chromium.org/2418763004/diff/20001/fuzz/fuzz.cpp#newcode425 fuzz/fuzz.cpp:425: reinterpret_cast<const char*>(bytes->data()), default_caps(), &output); On 2016/10/14 at 15:26:18, mtklein_C wrote: > This probably shouldn't be reinterpret_cast. It'll work fine but it gives off a strong "I am doing something weird and byte-punny here" vibe. Really we're just turning bytes into bytes... nothing weird. > > Try (const char*)bytes->data(), or maybe just bytes->bytes() if that works? Done. bytes->bytes() did not work
On 2016/10/14 at 16:28:05, kjlubick wrote: > What GLSL validator did you have in mind? And yes, that could probably be added, with an abort if the validator returns false to catch any validation errors. That was really a question for Ethan. I have no idea.
On 2016/10/14 16:46:06, dogben wrote: > On 2016/10/14 at 16:28:05, kjlubick wrote: > > What GLSL validator did you have in mind? And yes, that could probably be > added, with an abort if the validator returns false to catch any validation > errors. > > That was really a question for Ethan. I have no idea. The reference validator is called "glslangValidator". It's probably worth trying, though I'm not sure how bad the false positive situation will be (I don't generally view it as a huge problem if skslc isn't quite as strict as the glsl compiler about certain things, since it's just a question of which piece of software ends up reporting the error).
lgtm
Description was changed from ========== First try at SKSL fuzzer BUG= GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2418763004 ========== to ========== Add SKSL fuzzer BUG=skia:5490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2418763004 ==========
The CQ bit was checked by kjlubick@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add SKSL fuzzer BUG=skia:5490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2418763004 ========== to ========== Add SKSL fuzzer BUG=skia:5490 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2418763004 Committed: https://skia.googlesource.com/skia/+/e719577fe8ac3de38795cde2007337f854d97435 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/e719577fe8ac3de38795cde2007337f854d97435 |
