- User Since
- Feb 14 2017, 7:36 AM (78 w, 17 h)
Wed, Aug 8
Rebase and getting ready to land
Tue, Aug 7
Nice! Do you have an estimate on the performance improvement? The code becomes slightly harder to read, but I guess it's worth it.
Self-approval for a trivial change.
Mon, Aug 6
rebase and getting ready to land
LGTM, will land soon
Please update the CL summary and description. LGTM, otherwise.
Fri, Aug 3
Thu, Aug 2
Getting ready to land.
Wed, Aug 1
Left a couple minor comments. Looks good otherwise. Still not happy with the test, but can't think of anything better so far.
I don't like the test as it only tests that we do not completely break libFuzzer, but doesn't test the feature itself. I'll play with some ideas locally, will share those if anything works out. Otherwise, I guess we'll proceed with this test.
Tue, Jul 31
Mon, Jul 30
Fri, Jul 27
Haven't looked at the test yet, as the code needs a lot of clean up.
Thu, Jul 26
Wed, Jul 25
Tue, Jul 24
Can you re-phrase the CL description please, because it looks like a chat message I've sent to you in the past :)
Rebase and getting ready to commit.
Mon, Jul 23
Looks great! Left a couple comments + let's clean up the test. Since the test is specific for handle_unstable=3 mode, can we make it shorter, e.g. leave only 1 or 2 deterministic function, and so on. Also, let's rename the source file to HandleUnstablePoisonUnstableTest.cpp, the test file can be renamed to handle_unstable_poison_unstable.test for better readability.
Kevin, looks like test is not fully deterministic :(
Feel free to ask Matt / Kostya to review this once my comments are addressed.
I'm not sure I fully understand that approach. We put 0 into the counters for edges we think are unstable. That's fair, but every time we generate a new input that triggers such edge, we would re-run it twice just to learn again that it was unstable. Why not put something like 0xff to mark it like a covered edge, so that we would ignore any new testcases covering that?
Rebase and getting ready to commit.
Kevin, don't panic, ninja clean && ninja && ninja check-fuzzer made things work for me. Committing now.
Kevin, a bunch of tests seems to be failing for me with your change, but everything works perfectly without it. Do the tests pass for you?
Fri, Jul 20
Matt, thanks a lot for the review!
Matt, can you please take a look?
Kevin, can you think of a test that would verify that your handling approach works? We must have a test for this stuff to make sure it'll not get accidentally broken.
Thu, Jul 19
Wed, Jul 18
Rebase and getting ready to land.
Tue, Jul 17
Left some minor comments, but I think it's fine to ask Matt or Kostya to take a look and address all the feedback altogether, just to avoid changing things back and forth if we have different opinions.
Hey Matt, do you think we can land this or should also wait for Kostya's approval?
Mon, Jul 16
Actually, I think you might've already used that flag (it is used in LLVM Dev Starting Kit). In that case, you would need to add -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++, I think.
Reverted in https://reviews.llvm.org/rCRT337206.
Let me know if the rebuild fixes it. Otherwise, I'll take a closer look.
I'm seeing a build failure on one of the bots http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/13625/steps/build%20with%20ninja/logs/stdio :
Let me build everything completely from scratch. I'm sure something is messed up with my cmake config / cmake files in the build dir. I'll post an update. Thanks for your help!
Good call, but I have libcxx synced to ToT already :(