Add optional -Werror flag #5

GretaCB · 2017-08-30T20:24:31Z

Per #2

Next Actions

Fix current warnings (which are now erroring)
Set MAX for compress/decompress size
Remove std::string functions and only allow char * (per Update Readme #1 (comment))
Fix compile errors/Travis tests 🍏
~~Add tidy and format scripts~~
add handy Dockerfile
Code Review

springmeyer · 2017-08-31T00:14:05Z

CMakeLists.txt

+option(WERROR "Add -Werror flag to build (turns warnings into errors)" ON)
+
+# Note: -D_GLIBCXX_USE_CXX11_ABI=0 is needed to support mason packages that are precompiled libs
+# Currently we only depend on a header only library, but this will help avoid issues when more libs are added via mason
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -pedantic -Wsign-compare -Wconversion -Wshadow")


Looks as if you grabbed the comments about -D_GLIBCXX_USE_CXX11_ABI=0 without the actual flag being added to the CMAKE_CXX_FLAGS? Best to pull that over too.

springmeyer · 2017-08-31T23:52:35Z

@GretaCB just reviewed overall and have some suggestions to consider as you push tomorrow:

Move to using CHECK_THROWS_WITH or REQUIRE_THROWS_WITH so you can start asserting on the message of the exception (since in the current state they are potentially going to change depending on debug or release mode). You can use catch::Contains to just assert on a substring like: CHECK_THROWS_WITH(gzip::compress(pointer, l), Catch::Contains("too large to fit into unsigned int type"));
Since behavior may different depend on Debug/Release mode how about we change the makefile to test both when you run make test:

diff --git a/Makefile b/Makefile
index 179e1bb..75c578f 100644
--- a/Makefile
+++ b/Makefile
@@ -4,13 +4,16 @@ export WERROR ?= true
 default: release
 
 release:
-       mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Release -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
+       mkdir -p build/Release && cd build/Release && cmake ../../ -DCMAKE_BUILD_TYPE=Release -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
 
 debug:
-       mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Debug -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
+       mkdir -p build/Debug && cd build/Debug && cmake ../../ -DCMAKE_BUILD_TYPE=Debug -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
 
-test:
-       @if [ -f ./build/unit-tests ]; then ./build/unit-tests; else echo "Please run 'make release' or 'make debug' first" && exit 1; fi
+test: release debug
+       @echo "Running tests for Debug mode"
+       ./build/Debug/unit-tests
+       @echo "Running tests for Release mode"
+       ./build/Release/unit-tests
 
 tidy:
        ./scripts/clang-tidy.sh

GretaCB · 2017-09-01T22:29:13Z

@springmeyer I just pushed updates per chat earlier:

MAX_SIZE limits are now const and uneditable
Moved DEBUG tests into their own place. These work since the int overflow check happens before the max size check, but not sure if that will cause unnecessary tripups.
Also, not completely sure why the compress pointer test is failing. Assuming it has something to do with the size I'm passing in?

springmeyer · 2017-09-02T00:25:50Z

MAX_SIZE limits are now const and uneditable

Great.

Moved DEBUG tests into their own place. These work since the int overflow check happens before the max size check, but not sure if that will cause unnecessary tripups.

Good call 👍

Also, not completely sure why the compress pointer test is failing. Assuming it has something to do with the size I'm passing in?

In the near term I think we should stop comparing lengths. Because there are legitimate reasons for the encoded string to be longer than the raw string. So we should do away with all these comparisons. That leaves the question of how to test. I think in the short term we could simply ensure round-tripping works. In the long germ I think we should move to having:

file based fixtures for input data
an adjacent file of gzip coded data
then compare them byte for byte
optional: add an UPDATE environment variable to be able to put the tests into a mode where they are updateable

This way we can ensure the output is exactly as we expect. And we would pick up differences, say, between our results and another implementation like #7.

GretaCB · 2017-09-19T20:46:13Z

Ok cool so:

string param functions are gone
tests updated and passing locally
docs updated
asserts on test string results are patched for now. I'll split out @springmeyer 's suggestion above into a separate ticket for the future.

Now just need to troubleshoot the compile errors in Travis

GretaCB · 2017-09-22T00:05:52Z

Update

I was able to solve the a couple compiler errors:
- g++5's -fsanitize didnt like unsigned-integer-overflow. Solved the issue by not including the flag when compiler is GNU
- None of the Travis tests liked numeric_limits. I just needed to include <limits> explicitly.
I was unable to solve a memory leak error happening only during the sanitize Travis job. I attempted to replicate the leak and isolate one of the failing tests and take Catch out of the equation. I'm not 100% sure how to proceed to fully solve this.

springmeyer · 2017-09-26T08:16:31Z

g++5's -fsanitize didnt like unsigned-integer-overflow. Solved the issue by not including the flag when compiler is GNU

I don't think we need/want to have -funsigned-integer-overflow in the CMakeLists.txt at all.We already apply it for a specific travis job for clang++. I'm comfortable just testing the sanitizers through clang++ and not g++. If we remove from the CmakeLists.txt then no custom handling will be needed.

…ecreate memory leak

GretaCB · 2017-09-28T09:10:28Z

Makefile

 default: release

 release:
-	mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Release && VERBOSE=1 cmake --build .
+	mkdir -p build/Release && cd build/Release && cmake ../../ -DCMAKE_BUILD_TYPE=Release -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .


@springmeyer I'm noticing this is much different than the bench and the hpp-skel master Makefiles. Curious if you have a preference. I also noticed that this conflicts with how clang-tidy runs (I have it setup locally, not in this branch). Perhaps it has something to do with the extra ../?

I'm noticing this is much different than the bench and the hpp-skel master Makefiles.

This was done so we could have make test run both release and debug tests while developing. I think we should roll back to match hpp-skel now that development is slowing down.

I also noticed that this conflicts with how clang-tidy runs (I have it setup locally, not in this branch).

Whoops, that was an oversight. So, hopefully rolling back to match hpp-skel will help the clang-tidy scripts find things right.

Syncing with master and I'll keep the WERROR flag in there

mmmm, perhaps only for Debug build

GretaCB · 2017-09-28T09:18:09Z

Makefile

-	mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Debug && VERBOSE=1 cmake --build .
+	mkdir -p build/Debug && cd build/Debug && cmake ../../ -DCMAKE_BUILD_TYPE=Debug -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
+
+test: release debug


I didnt merge the fancy if logic from master that checks for the existence of the unit-tests binary since we're compiling both debug and release during tests.

GretaCB · 2017-09-28T09:18:30Z

Makefile


 bench:
 	@if [ -f ./build/bench-tests ]; then ./build/bench-tests; else echo "Please run 'make release' or 'make debug' first" && exit 1; fi

-test: default 
-	@if [ -f ./build/unit-tests ]; then ./build/unit-tests; else echo "Please run 'make release' or 'make debug' first" && exit 1; fi
+tidy:


removing and will add in a separate branch

GretaCB · 2017-09-28T09:18:43Z

Makefile


 coverage:
 	./scripts/coverage.sh

 clean:
 	rm -rf build

+format:


removing and will add in a separate branch

GretaCB · 2017-09-28T09:40:05Z

Shoot, after re-adding the tests, looks like a couple more errors popped up. Taking a peek. Also debugging the Dockerfile now that the make commands have changed.

GretaCB · 2017-09-28T10:14:43Z

Looks like we've got a segfault in the tests.

Trying to spin up the Docker image to try and recreate/troubleshoot, but the Dockerfile is failing when checking for the CXX compiler:

Carols-MacBook-Pro:gzip-hpp carol$ docker run -it --cap-add SYS_PTRACE gzip-hpp
mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Release && VERBOSE=1 cmake --build .
-- The CXX compiler identification is unknown
-- Check for working CXX compiler: /usr/local/src/.toolchain/bin/clang++
-- Check for working CXX compiler: /usr/local/src/.toolchain/bin/clang++ -- broken
CMake Error at mason_packages/linux-x86_64/cmake/3.8.2/share/cmake-3.8/Modules/CMakeTestCXXCompiler.cmake:44 (message):
  The C++ compiler "/usr/local/src/.toolchain/bin/clang++" is not able to
  compile a simple test program.

  It fails with the following output:

   Change Dir: /usr/local/src/build/CMakeFiles/CMakeTmp

  

  Run Build Command:"/usr/bin/make" "cmTC_207cd/fast"

  make[1]: Entering directory '/usr/local/src/build/CMakeFiles/CMakeTmp'

  /usr/bin/make -f CMakeFiles/cmTC_207cd.dir/build.make
  CMakeFiles/cmTC_207cd.dir/build

  make[2]: Entering directory '/usr/local/src/build/CMakeFiles/CMakeTmp'

  Building CXX object CMakeFiles/cmTC_207cd.dir/testCXXCompiler.cxx.o

  /usr/local/src/.toolchain/bin/clang++ -fsanitize=address,undefined
  -fno-sanitize-recover=all -o
  CMakeFiles/cmTC_207cd.dir/testCXXCompiler.cxx.o -c
  /usr/local/src/build/CMakeFiles/CMakeTmp/testCXXCompiler.cxx

  /usr/local/src/.toolchain/bin/clang++: 1:
  /usr/local/src/.toolchain/bin/clang++: Syntax error: "(" unexpected

  CMakeFiles/cmTC_207cd.dir/build.make:65: recipe for target
  'CMakeFiles/cmTC_207cd.dir/testCXXCompiler.cxx.o' failed

  make[2]: *** [CMakeFiles/cmTC_207cd.dir/testCXXCompiler.cxx.o] Error 2

  make[2]: Leaving directory '/usr/local/src/build/CMakeFiles/CMakeTmp'

  Makefile:126: recipe for target 'cmTC_207cd/fast' failed

  make[1]: *** [cmTC_207cd/fast] Error 2

  make[1]: Leaving directory '/usr/local/src/build/CMakeFiles/CMakeTmp'

  

  

  CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
  CMakeLists.txt:2 (project)


-- Configuring incomplete, errors occurred!
See also "/usr/local/src/build/CMakeFiles/CMakeOutput.log".
See also "/usr/local/src/build/CMakeFiles/CMakeError.log".
Makefile:7: recipe for target 'release' failed
make: *** [release] Error 1

The same error occurs when I revert back to the Makefile we had earlier this week when we got the image compiling successfully.

springmeyer · 2017-09-28T10:20:04Z

I'm not sure if docker ignores hidden directories in its COPY. If not then I wonder if the dockerfile problem is due to the .toolchain directory getting copied from your host to the container. Maybe try adding .toolchain to the .dockerignore and then rebuilding/running: does that allow the build in docker to work?

GretaCB · 2017-09-28T10:24:03Z

@springmeyer 👍 That did the trick. Uncovered another leak, diving in 🏊‍♀️

springmeyer · 2017-09-28T19:31:00Z

Noticing the last commit failed only with clang++ and only with the release build. Very peculiar. It fails with:

round trip compression - gzip
  strategy - invalid compression
-------------------------------------------------------------------------------
/home/travis/build/mapbox/gzip-hpp/test/test.cpp:89
...............................................................................
/home/travis/build/mapbox/gzip-hpp/test/test.cpp:89: FAILED:
  {Unknown expression after the reported line}
with expansion:
due to a fatal error condition:
  SIGSEGV - Segmentation violation signal

https://travis-ci.org/mapbox/gzip-hpp/jobs/280809852#L638

Looks like that the gzip::compress segfaulting when 99 is passed for the strategy arg:

gzip-hpp/test/test.cpp

Line 94 in e765e3d

CHECK_THROWS(gzip::compress(pointer, data.size(), level, strategy));

.

Thoughts are:

catch.hpp catches the segfaul signal such that it is not possible for logbt to get a backtrace out. I've also found this to be an annoying default in catch so I often comment out this part of catch.hpp to re-enable the actual segfault
The fact that this is only crashing in release mode makes me wonder if this is triggering some kind of undefined behavior in zlib, which perhaps is not our problem but nevertheless a problem.

springmeyer · 2017-09-28T20:00:51Z

Tried to replicate locally with the Dockerfile and could not (yet).

springmeyer · 2017-09-28T20:16:14Z

was able to extract a crashlog using logbt on travis (werror...crash-debug):

Core was generated by `./build/unit-tests'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000454972 in deflateEnd ()
Thread 1 (LWP 6071):
#0  0x0000000000454972 in deflateEnd ()
#1  0x000000000040447d in gzip::compress(char const*, unsigned long, int, int) ()
#2  0x0000000000414aac in ____C_A_T_C_H____T_E_S_T____10() ()
#3  0x000000000042ec0b in Catch::RunContext::runCurrentTest(std::string&, std::string&) ()
#4  0x000000000041fd39 in Catch::RunContext::runTest(Catch::TestCase const&) ()
#5  0x0000000000405b9d in Catch::runTests(Catch::Ptr<Catch::Config> const&) ()
#6  0x00000000004399d7 in Catch::Session::run() ()
#7  0x00000000004130cb in main ()

springmeyer · 2017-09-28T20:20:24Z

^^ was missing line numbers since -g was not in the release flags (refs mapbox/hpp-skel#51). So I quickly added to get line numbers:

Core was generated by `./build/unit-tests'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000454972 in deflateEnd ()
Thread 1 (LWP 6153):
#0  0x0000000000454972 in deflateEnd ()
#1  0x000000000040447d in gzip::compress (data=0x75adf8 "this is a sentence that will be compressed into something", size=57, level=<optimized out>, strategy=<optimized out>) at /home/travis/build/mapbox/gzip-hpp/include/gzip/compress.hpp:31
#2  0x0000000000414aac in ____C_A_T_C_H____T_E_S_T____10 () at /home/travis/build/mapbox/gzip-hpp/test/test.cpp:94
#3  0x000000000042ec0b in Catch::TestCase::invoke (this=<optimized out>) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:8136
#4  Catch::RunContext::invokeActiveTestCase (this=0x7ffee0395b48) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:6697
#5  Catch::RunContext::runCurrentTest (this=0x7ffee0395b48, redirectedCout=..., redirectedCerr=...) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:6664
#6  0x000000000041fd39 in Catch::RunContext::runTest (this=0x7ffee0395b48, testCase=...) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:6477
#7  0x0000000000405b9d in Catch::runTests (config=...) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:6837
#8  0x00000000004399d7 in Catch::Session::run (this=<optimized out>) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:6971
#9  0x00000000004130cb in Catch::Session::run (this=0x7ffee0395d30, argc=1, argv=<optimized out>) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:6924
#10 main (argc=<optimized out>, argv=<optimized out>) at /home/travis/build/mapbox/gzip-hpp/test/catch.hpp:11176

GretaCB · 2017-09-28T20:59:12Z

Amazing sleuthing @springmeyer , thank you for poking at this. Looks like it's crashing at deflateEnd(...):

    if (deflateInit2(&deflate_s, level, Z_DEFLATED, 31, 8, strategy) != Z_OK)
    {
        deflateEnd(&deflate_s); // explicitly end deflate to avoid memory leak
        throw std::runtime_error("deflate init failed");
    }

...wondering if it makes sense to end deflate here if deflate was never able to fully initiate.

springmeyer · 2017-09-29T12:21:46Z

Removing deflateEnd like you did in a936540 looks like the right fix. This highlights for me that we are missing handling of errors in InflateInit2 (

gzip-hpp/include/gzip/decompress.hpp

Line 20 in e3a946a

inflateInit2(&inflate_s, 32 + 15);

) since we don't respond to a error return code there.

Overall, great work, stoked to see this 🍏!

GretaCB · 2017-10-02T07:54:02Z

Great catch @springmeyer . Committed the changes, Travis is 🍏 , going to 🚢 .

Next PR is for tidy/format.

springmeyer · 2017-10-12T17:08:33Z

Noting here that this branch was a big lift because we had a number of aggressive warnings enabled which started error-ing when we added -Werror. Like this project, we'll want to consistently have these more aggressive warnings enabled (like -Wconversion which highlight potential integer overflow or truncation issues). So, linking to refs mapbox/cpp#37 from here so that others which follow refs mapbox/cpp#37 can see this effort (git github interlinking) and learn from it.

add optional -Werror flag

908771d

springmeyer reviewed Aug 31, 2017

View reviewed changes

gretacb added 3 commits August 31, 2017 12:17

reproduce implicit conversion and throw

925861c

add checks for implicit conversion

ba04b83

starting to set size limit

f9b7a53

gretacb added 3 commits September 1, 2017 15:13

assert specific error thrown

4a3f79a

reset maxsize in tests

1c8c2bf

detangle DEBUG tests and enforce const max

7d6c327

GretaCB mentioned this pull request Sep 14, 2017

Add benchmarks #8

Merged

2 tasks

gretacb added 4 commits September 19, 2017 14:16

short term fix to ensure roundtripping

0c6f3bc

remove string arg function

344ad85

update readme

1374863

remove string arg boolean function and update tests

88d2c16

GretaCB mentioned this pull request Sep 19, 2017

Soup up test data handling #10

Open

gretacb added 6 commits September 21, 2017 10:28

handle flags per compiler

daa89d1

only need if for gnu

3d7e6ab

see if including limits helps with errors

7412f05

include limits in decompress as well

3e37ecc

try require instead of check

9c0dd3c

try to replicate leak without catch.hpp

d5844e0

gretacb added 4 commits September 26, 2017 11:15

remove compiler-specific set logic and properly handle exception to r…

804c5f7

…ecreate memory leak

properly end deflate

1ca7f3f

Merge branch 'master' into werror

f6747c3

re-add tests

238dba3

GretaCB commented Sep 28, 2017

View reviewed changes

gretacb added 5 commits September 28, 2017 11:21

will add in a separate cleanup branch

2026ac9

sync with master, keep werror flag for debug

99a1554

add handy Dockerfile for local debugging

11ec96f

turn werror on by default

9cce0d9

add dockerignore file

4bd8899

gretacb added 2 commits September 28, 2017 12:27

manage memory leak in decompress

81fb1ca

add explicit make debug

e765e3d

remove deflateEnd during failed deflateInit

a936540

handle init errors

ccf354c

GretaCB merged commit 55badfe into master Oct 2, 2017

GretaCB deleted the werror branch October 2, 2017 07:54

This was referenced Oct 2, 2017

Make warnings errors #2

Closed

Move to single compress API #4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optional -Werror flag #5

Add optional -Werror flag #5

GretaCB commented Aug 30, 2017 •

edited

Loading

springmeyer Aug 31, 2017

springmeyer commented Aug 31, 2017

GretaCB commented Sep 1, 2017 •

edited

Loading

springmeyer commented Sep 2, 2017

GretaCB commented Sep 19, 2017 •

edited

Loading

GretaCB commented Sep 22, 2017 •

edited

Loading

springmeyer commented Sep 26, 2017

GretaCB Sep 28, 2017

springmeyer Sep 28, 2017

GretaCB Sep 28, 2017 •

edited

Loading

GretaCB Sep 28, 2017

GretaCB Sep 28, 2017

GretaCB Sep 28, 2017

GretaCB Sep 28, 2017

GretaCB commented Sep 28, 2017 •

edited

Loading

GretaCB commented Sep 28, 2017

springmeyer commented Sep 28, 2017

GretaCB commented Sep 28, 2017

springmeyer commented Sep 28, 2017

springmeyer commented Sep 28, 2017

springmeyer commented Sep 28, 2017

springmeyer commented Sep 28, 2017 •

edited

Loading

GretaCB commented Sep 28, 2017 •

edited

Loading

springmeyer commented Sep 29, 2017

GretaCB commented Oct 2, 2017

springmeyer commented Oct 12, 2017

Add optional -Werror flag #5

Add optional -Werror flag #5

Conversation

GretaCB commented Aug 30, 2017 • edited Loading

Next Actions

springmeyer Aug 31, 2017

Choose a reason for hiding this comment

springmeyer commented Aug 31, 2017

GretaCB commented Sep 1, 2017 • edited Loading

springmeyer commented Sep 2, 2017

GretaCB commented Sep 19, 2017 • edited Loading

GretaCB commented Sep 22, 2017 • edited Loading

Update

springmeyer commented Sep 26, 2017

GretaCB Sep 28, 2017

Choose a reason for hiding this comment

springmeyer Sep 28, 2017

Choose a reason for hiding this comment

GretaCB Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

GretaCB Sep 28, 2017

Choose a reason for hiding this comment

GretaCB Sep 28, 2017

Choose a reason for hiding this comment

GretaCB Sep 28, 2017

Choose a reason for hiding this comment

GretaCB Sep 28, 2017

Choose a reason for hiding this comment

GretaCB commented Sep 28, 2017 • edited Loading

GretaCB commented Sep 28, 2017

springmeyer commented Sep 28, 2017

GretaCB commented Sep 28, 2017

springmeyer commented Sep 28, 2017

springmeyer commented Sep 28, 2017

springmeyer commented Sep 28, 2017

springmeyer commented Sep 28, 2017 • edited Loading

GretaCB commented Sep 28, 2017 • edited Loading

springmeyer commented Sep 29, 2017

GretaCB commented Oct 2, 2017

springmeyer commented Oct 12, 2017

GretaCB commented Aug 30, 2017 •

edited

Loading

GretaCB commented Sep 1, 2017 •

edited

Loading

GretaCB commented Sep 19, 2017 •

edited

Loading

GretaCB commented Sep 22, 2017 •

edited

Loading

GretaCB Sep 28, 2017 •

edited

Loading

GretaCB commented Sep 28, 2017 •

edited

Loading

springmeyer commented Sep 28, 2017 •

edited

Loading

GretaCB commented Sep 28, 2017 •

edited

Loading