Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upadd fuzzing #60
add fuzzing #60
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hi Paul, Thanks for your contribution! I had a first look at your code and overall I am very satisfied with the quality. Reading through your comments I see that you mention the option Ideally I would like to build libdivide with fuzzing using only:
|
This comment has been minimized.
This comment has been minimized.
|
Thanks for the kind words, glad to contribute to this awesome library! I pushed some changes to address your review comments The end user can now use the simplified invocation, the only caveat is that the C++ compiler must be clang (if not, cmake will output an error message): export CXX=clang++
cmake .. -DBUILD_FUZZERS=ONA more advanced user, explicitly troubleshooting simd for a particular simd version could use: export CXX=clang++
cmake .. -DBUILD_FUZZERS=ON -DLIBDIVIDE_AVX2=On |
This comment has been minimized.
This comment has been minimized.
|
Thanks for the changes. I have tested it on my PC and it worked fine. I have found just one minor issue: I was able to generate a warning with clang8. Can you please fix it? $ CXXFLAGS="-Wall -Wextra -pedantic" CC=clang-8 CXX=clang++-8 cmake .. -DBUILD_FUZZERS=ON
-- Configuring done
-- Generating done
-- Build files have been written to: /home/kim/Downloads/libdivide-paul-fuzzing/build
$ make
[ 10%] Building CXX object CMakeFiles/fuzzer_scalar.dir/test/fuzzer_scalar.cpp.o
/home/kim/Downloads/libdivide-paul-fuzzing/test/fuzzer_scalar.cpp:24:68: warning: comparison of integers of different signs: 'unsigned long' and 'int' [-Wsign-compare]
(numerator == std::numeric_limits<Integer>::min() && divisor == -1)) {
~~~~~~~ ^ ~~
/home/kim/Downloads/libdivide-paul-fuzzing/test/fuzzer_scalar.cpp:65:7: note: in instantiation of function template specialization 'applyOnScalars<unsigned long>' requested here
applyOnScalars<std::uint64_t>(Data, Size);
^
/home/kim/Downloads/libdivide-paul-fuzzing/test/fuzzer_scalar.cpp:24:68: warning: comparison of integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
(numerator == std::numeric_limits<Integer>::min() && divisor == -1)) {
~~~~~~~ ^ ~~
/home/kim/Downloads/libdivide-paul-fuzzing/test/fuzzer_scalar.cpp:68:7: note: in instantiation of function template specialization 'applyOnScalars<unsigned int>' requested here
applyOnScalars<std::uint32_t>(Data, Size);
^
2 warnings generated. |
This comment has been minimized.
This comment has been minimized.
|
The warnings should be gone now! And they indicated a real bug, removing the 0/UINT_MAX from being tested. |
This comment has been minimized.
This comment has been minimized.
|
I have found another issue on Ubuntu 18.04 for both Clang 6 and Clang 8. It would be nice if we could fix this issue. $ CXXFLAGS="-Wall -Wextra -pedantic" CC=clang CXX=clang++ cmake .. -DBUILD_FUZZERS=ON -DCMAKE_BUILD_TYPE=Debug && make -j5
-- Configuring done
-- Generating done
-- Build files have been written to: /home/kim/Downloads/libdivide-paul-fuzzing/build
[ 70%] Built target tester
[ 70%] Built target benchmark
[ 60%] Linking CXX executable fuzzer_scalar
[ 70%] Built target fuzzer_simd
[ 90%] Built target benchmark_branchfree
CMakeFiles/fuzzer_scalar.dir/test/fuzzer_scalar.cpp.o: In function `libdivide::libdivide_mullhi_s64(long, long)':
/home/kim/Downloads/libdivide-paul-fuzzing/libdivide.h:264: undefined reference to `__muloti4'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
CMakeFiles/fuzzer_scalar.dir/build.make:94: recipe for target 'fuzzer_scalar' failed
make[2]: *** [fuzzer_scalar] Error 1
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/fuzzer_scalar.dir/all' failed
make[1]: *** [CMakeFiles/fuzzer_scalar.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2 |
This comment has been minimized.
This comment has been minimized.
|
I have seen that when I forced AVX512 on a not so recent intel cpu (if I remember correctly). I |
This comment has been minimized.
This comment has been minimized.
|
I got the clang-6 on ubuntu 18.04 scenario to work by manually linking with |
This comment has been minimized.
This comment has been minimized.
|
The problem seems to go away if the debug case is built with -Og or higher: $ CXXFLAGS="-Wall -Wextra -pedantic -Og" CC=clang CXX=clang++ cmake .. -DBUILD_FUZZERS=ON -DCMAKE_BUILD_TYPE=Debug && make -j5
-- Generating done
-- Build files have been written to: /home/paul/code/delaktig/libdivide/problemrelease
Scanning dependencies of target fuzzer_scalar
Scanning dependencies of target benchmark
Scanning dependencies of target benchmark_branchfree
Scanning dependencies of target tester
Scanning dependencies of target fuzzer_simd
[ 20%] Building CXX object CMakeFiles/fuzzer_scalar.dir/test/fuzzer_scalar.cpp.o
[ 20%] Building C object CMakeFiles/benchmark.dir/test/benchmark.c.o
[ 30%] Building CXX object CMakeFiles/tester.dir/test/tester.cpp.o
[ 40%] Building CXX object CMakeFiles/fuzzer_simd.dir/test/fuzzer_simd.cpp.o
[ 50%] Building CXX object CMakeFiles/benchmark_branchfree.dir/test/benchmark_branchfree.cpp.o
[ 60%] Linking C executable benchmark
[ 60%] Built target benchmark
[ 70%] Linking CXX executable fuzzer_scalar
[ 70%] Built target fuzzer_scalar
[ 80%] Linking CXX executable fuzzer_simd
[ 80%] Built target fuzzer_simd
[ 90%] Linking CXX executable benchmark_branchfree
[ 90%] Built target benchmark_branchfree
[100%] Linking CXX executable tester
[100%] Built target tester
|
This comment has been minimized.
This comment has been minimized.
Yes. The issue goes away if I remove the flag So for me this is a compiler issue as well. However I think we should just remove this option for the time being. We can put it back once this issue gets fixed in LLVM/Clang. |
This comment has been minimized.
This comment has been minimized.
Yep, works fine. Ship it! :-) |
This comment has been minimized.
This comment has been minimized.
|
Thanks for your contribution! |
pauldreik commentedDec 3, 2019
•
edited
This adds fuzzer targets. I could not test the avx512 support, because I do not have access to hardware supporting it.
No bugs were found.