Closed pauldreik closed 4 years ago
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 -DCMAKE_CXX_FLAGS="-fsanitize=fuzzer-no-link"
quite often. I guess we should try to set this option automatically so the build becomes easier for the user. Is this option only required for SSE/AVX2/AV512 or also for the libdivide scalar functions?
Ideally I would like to build libdivide with fuzzing using only:
cmake . -DBUILD_FUZZERS=ON
Thanks for the kind words, glad to contribute to this awesome library!
I pushed some changes to address your review comments The -fsanitize=fuzzer flags are needed to get instrumentation and the fuzzing engine linked in to binary. They are needed for all code that is of interest to the fuzzer (not coupled to simd or not). Since this is a header only library it is perfectly fine to set them only for the fuzzer_*.cpp files. I modified the build and now set them inside the cmake file.
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=ON
A more advanced user, explicitly troubleshooting simd for a particular simd version could use:
export CXX=clang++
cmake .. -DBUILD_FUZZERS=ON -DLIBDIVIDE_AVX2=On
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.
The warnings should be gone now! And they indicated a real bug, removing the 0/UINT_MAX from being tested.
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
I have seen that when I forced AVX512 on a not so recent intel cpu (if I remember correctly). I googled and read bug reports for a while, I think it is a bug in clang relating to linking to compiler-rt. What kind of cpu were you testing this on, one with AVS512 support? Does it remain if you force AVX2?
I got the clang-6 on ubuntu 18.04 scenario to work by manually linking with /usr/lib/llvm-6.0/lib/clang/6.0.0/lib/linux/libclang_rt.builtins-x86_64.a
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
Does it remain if you force AVX2?
Yes.
The issue goes away if I remove the flag -fsanitize=undefined
. Can you confirm this?
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.
Can you confirm this?
Yep, works fine. Ship it! :-)
Thanks for your contribution!
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.