Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execute tests with clang sanitizers #394

Closed
nlohmann opened this issue Dec 13, 2016 · 10 comments · Fixed by #410
Closed

Execute tests with clang sanitizers #394

nlohmann opened this issue Dec 13, 2016 · 10 comments · Fixed by #410

Comments

@nlohmann
Copy link
Owner

Issue #389 could have been prevented if the UB sanitizer had been used during continuous integration. We shall add a Travis job for this if possible.

Required flags:

-g -O2 -fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer
@nlohmann
Copy link
Owner Author

@nlohmann
Copy link
Owner Author

For some reason, the code does not compile with sanitizers switched on, see https://travis-ci.org/nlohmann/json/jobs/183684093.

@Daniel599
Copy link
Contributor

Daniel599 commented Dec 23, 2016

Hi, I think I found out why it didn't compile (I had the same problem on my Ubuntu 14.04)
I used clang++-3.4 and I think that by default it uses gcc's stdlib (which is gcc 4.8 on Ubuntu 14.04),
now this stdlib isn't fully c++11 compliant, e.g vector::insert( iterator pos, InputIt first, InputIt last) returns void, and I think that your travis-ci has the same problem since it is Ubuntu 14.04 also.

by adding : -stdlib=libc++
clang_sanitize: clean CXX=clang++ CXXFLAGS="-stdlib=libc++ -g -O2 -fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer" $(MAKE)

It compiled but seems it didn't run the tests, I will try to work on it and give you a PR, but since it my first time here, I need to find out how.
Anyway, I hope it helps.

@Daniel599
Copy link
Contributor

Daniel599 commented Dec 24, 2016

I was able to make it compile and run on travis-ci
https://travis-ci.org/Daniel599/json/jobs/186527572#L5753
but sadly some of the errors don;t give callstack so I don't know how to fix them.
Should I make a PR or investigate more?

@nlohmann
Copy link
Owner Author

Thanks @Daniel599, I shall try this in a feature branch.

nlohmann added a commit that referenced this issue Dec 25, 2016
@nlohmann
Copy link
Owner Author

@Daniel599 I can no reproduce your observation: https://travis-ci.org/nlohmann/json/jobs/186632504#L5756

The test case that triggers the undefined behavior is a bit weird anyway. It is an allocator which can be configured to throw a bad_alloc in some situations. Unfortunately, I cannot reproduce the error on my machine yet.

@Daniel599
Copy link
Contributor

@nlohmann I have been thinking about it some more, and I think that something here is odd;
by going with my logic above (clang uses gcc-4.8's stdlibc++), none of your clang builds should have work,
but they do, so I think i`m missing something.

Please note that there is another run-time error in unit-algorithms (but seems to be only with clang's libc++) .
Anyway, I have been trying to run clang_sanitize on my other PC (ubuntu 16.04 with gcc 5.4),
but there is a bug in libc++ there
so I tried with libstdc++ of gcc 5.4, and it works :), I think that your code has 0 errors with libstdc++,
trying on travis now (by adding package g++-6 to apt).

but considering my first lines of this comment, I`m still not sure that's the right way.
sorry for long post.

@nlohmann
Copy link
Owner Author

No worries. Anything that helps us execute the tests with sanitizers is good 😄

@Daniel599
Copy link
Contributor

the build on travis is done:
https://travis-ci.org/Daniel599/json/jobs/186639548
I thought it would be green (was OK on my PC with gcc 5.4) but it still has LeakSanitizer errors,
the good news, the UB errors are gone 😄
how do I know what packages did travis install? I don't see it the logs

Still no idea how your clang build works, would like to look into that later,
I`ll try to investigate about the leaks.

side note:
On my PC I got some warnings regarding shifts (using gcc 5.4) which I didn't see on travis (which is also weird o.0)

In file included from src/unit-cbor.cpp:31:
../src/json.hpp:6244:60: warning: shift count >= width of type
[-Wshift-count-overflow]
vec.push_back(static_cast<uint8_t>((number >> 070) & 0xff));
^ ~~~
../src/json.hpp:6724:21: note: in instantiation of function template
specialization 'nlohmann::basic_json<std::map, std::vector,
std::__cxx11::basic_string, bool, long long, unsigned long long,
double, std::allocator>::add_to_vector' requested here
add_to_vector(v, 1, N);
^
../src/json.hpp:7622:9: note: in instantiation of member function
'nlohmann::basic_json<std::map, std::vector,
std::__cxx11::basic_string, bool, long long, unsigned long long,
double, std::allocator>::to_cbor_internal' requested here
to_cbor_internal(j, result);
^
src/unit-cbor.cpp:44:39: note: in instantiation of member function
'nlohmann::basic_json<std::map, std::vector,
std::__cxx11::basic_string, bool, long long, unsigned long long,
double, std::allocator>::to_cbor' requested here
const auto result = json::to_cbor(j);
^`

I don't think it's big deal since all your uses are with constant a mount of bytes and their size matches,
but if people use with your code -Werror, that might get in their way
I can look into that as well if you think it's needed

Daniel599 added a commit to Daniel599/json that referenced this issue Dec 25, 2016
@Daniel599
Copy link
Contributor

@nlohmann Hi
I did some digging, and I found out why clang_sanitize didn't use libc++:
it's because clang_sanitize build in the MakeFile overridden by mistake the CXXFLAGS provided by .travis.yml.
I tried to run the sanitize with clang's libc++ but it give errors within the lib itself,
so now, I`m creating the new branch with clang and g++-6's libstdc++ which works.
Also, I did menage to fix the leak in the unit-tests, but it's weird the valgrind job didn't find it, I think there is a problem in the return-code of that job.

Daniel599 added a commit to Daniel599/json that referenced this issue Dec 30, 2016
@nlohmann nlohmann added this to the Release 2.0.10 milestone Jan 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants