search

mapbox / protozero

Minimalist protocol buffer decoder and encoder in C++
BSD 2-Clause "Simplified" License
292 stars 78 forks source link

Some tests fail on FreeBSD 11.1 with clang/libc++ #90

Closed AMDmi3 closed 6 years ago

AMDmi3 commented 6 years ago

When built with gcc/libstdc++ the tests pass, but when build with clang/libc++ (which is the default on FreeBSD): 

Test project /ssd/portstrees/batchports-mem/devel/protozero/work/protozero-1.6.2                                                                                                                                                                                     
    Start 1: pbf-decoder-no-args                                                                                                                                                                                                                                     
1/9 Test #1: pbf-decoder-no-args ..............   Passed    0.00 sec                                                                                                                                                                                                 
    Start 2: pbf-decoder-help                                                                                                                                                                                                                                        
2/9 Test #2: pbf-decoder-help .................   Passed    0.00 sec                                                                                                                                                                                                 
    Start 3: pbf-decoder-empty                                                                                                                                                                                                                                       
3/9 Test #3: pbf-decoder-empty ................   Passed    0.00 sec                                                                                                                                                                                                 
    Start 4: pbf-decoder-data                                                                                                                                                                                                                                        
4/9 Test #4: pbf-decoder-data .................   Passed    0.00 sec                                                                                                                                                                                                 
    Start 5: pbf-decoder-vt                                                                                                                                                                                                                                          
5/9 Test #5: pbf-decoder-vt ...................   Passed    0.39 sec                                                                                                                                                                                                 
    Start 6: pbf-decoder-fail                                                                                                                                                                                                                                        
6/9 Test #6: pbf-decoder-fail .................   Passed    0.01 sec                                                                                                                                                                                                 
    Start 7: pbf-decoder-fail-msg                                                                                                                                                                                                                                    
7/9 Test #7: pbf-decoder-fail-msg .............   Passed    0.01 sec                                                                                                                                                                                                 
    Start 8: reader_tests                                                                                                                                                                                                                                            
8/9 Test #8: reader_tests .....................   Passed    0.03 sec                                                                                                                                                                                                 
    Start 9: unit_tests                                                                                                                                                                                                                                              
9/9 Test #9: unit_tests .......................***Failed    0.06 sec                                                                                                                                                                                                 
% test/unit/unit_tests 

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                                                                                                      
unit_tests is a Catch v1.12.0 host application.                                                                                                                                                                                                                      
Run with -? for options                                                                                                                                                                                                                                              

-------------------------------------------------------------------------------                                                                                                                                                                                      
next() should throw when illegal tag is encountered                                                                                                                                                                                                                  
-------------------------------------------------------------------------------                                                                                                                                                                                      
test/unit/test_basic.cpp:40                                                                                                                                                                                                                                          
...............................................................................                                                                                                                                                                                      

test/unit/test_basic.cpp:60: FAILED:                                                                                                                                                                                                                                 
  REQUIRE_THROWS_AS( item.next(), const protozero::invalid_tag_exception& )                                                                                                                                                                                          
because no exception was thrown where one was expected:                                                                                                                                                                                                              

-------------------------------------------------------------------------------                                                                                                                                                                                      
next() works when a legal tag is encountered                                                                                                                                                                                                                         
-------------------------------------------------------------------------------                                                                                                                                                                                      
test/unit/test_basic.cpp:63                                                                                                                                                                                                                                          
...............................................................................                                                                                                                                                                                      

test/unit/test_basic.cpp:83: FAILED:                                                                                                                                                                                                                                 
  REQUIRE( item.next() )                                                                                                                                                                                                                                             
with expansion:                                                                                                                                                                                                                                                      
  false                                                                                                                                                                                                                                                              

===============================================================================                                                                                                                                                                                      
test cases:      37 |      35 passed | 2 failed                                                                                                                                                                                                                      
assertions: 1051517 | 1051515 passed | 2 failed

Haven't investigated further yet.

springmeyer commented 6 years ago

Interesting. Which version of clang++?

AMDmi3 commented 6 years ago

4.0.0

joto commented 6 years ago

@AMDmi3 Can you try to find out which of the SECTIONs in those tests fail? Comment out each of the SECTIONs and test them one by one.

Maybe some us have to be replaced by uls to make sure we have long ints? I have tested this on 32bit Linux, but maybe something is different on 32bit BSD (assuming this is even a 32 bit system) or with clang4?

AMDmi3 commented 6 years ago

Hmm, moving a code into a standalone app shows that the exception is thrown correctly. Then I've narrowed it to the following: tests pass if I comment out check every possible value for single byte in buffer. Could it be that there's an UB somewhere there?

AMDmi3 commented 6 years ago

Further down, the tests are fixed after this line is commented out:

https://github.com/mapbox/protozero/blob/d5d8debf1b17c6bb652395957b76cde7787e5377/test/unit/test_basic.cpp#L29

could it be that it advances past the end of 1 char buffer?

joto commented 6 years ago

I have added some asserts and tests that might help to zero in on the problem if your hunch is correct. Can you try the newest master?

AMDmi3 commented 6 years ago

Nothing has changed.

joto commented 6 years ago

It might be that this is a problem of the Catch library used:

https://github.com/catchorg/Catch2/blob/46c7c9d3a0cb975dabf5970cc65fd8fc72d4c5b5/docs/limitations.md#clangg----skipping-leaf-sections-after-an-exception

I have implemented a workaround by not using SECTIONs. If the problem goes away, we found it. If not, we can see better which case is the problematic one.

AMDmi3 commented 6 years ago

Yes, on 99ca512 tests pass fine.

joto commented 6 years ago

Okay, so it was the "catch" problem after all. Rather annoying that our unit test framework or the C++ std lib has this problem, but I guess there isn't anything we can do about this here. But as it works now for you, I'll close this here. Thanks for bringing this to our attention and helping with the problem solving.