open-source-parsers / jsoncpp

A C++ library for interacting with JSON.
Other
8.13k stars 2.64k forks source link

Failing ValueTest/integers unit test (32-bit gcc Linux RelWithDebInfo) #90

Closed emarthinsen closed 9 years ago

emarthinsen commented 9 years ago

I'm building with CMake and getting the following error from the unit tests:

Testing ValueTest/checkNormalizeFloatingPointStr: OK
Testing ValueTest/memberCount: OK
Testing ValueTest/objects: OK
Testing ValueTest/arrays: OK
Testing ValueTest/null: OK
Testing ValueTest/strings: OK
Testing ValueTest/bools: OK
Testing ValueTest/integers: FAILED
Testing ValueTest/nonIntegers: OK
Testing ValueTest/compareNull: OK
Testing ValueTest/compareInt: OK
Testing ValueTest/compareUInt: OK
Testing ValueTest/compareDouble: OK
Testing ValueTest/compareString: OK
Testing ValueTest/compareBoolean: OK
Testing ValueTest/compareArray: OK
Testing ValueTest/compareObject: OK
Testing ValueTest/compareType: OK
Testing ValueTest/offsetAccessors: OK
Testing ValueTest/typeChecksThrowExceptions: OK
Testing ReaderTest/parseWithNoErrors: OK
Testing ReaderTest/parseWithNoErrorsTestingOffsets: OK
Testing ReaderTest/parseWithOneError: OK
Testing ReaderTest/parseChineseWithOneError: OK
Testing ReaderTest/parseWithDetailError: OK
Testing WriterTest/dropNullPlaceholders: OK
* Detail of ValueTest/integers test failure:
/home/eric/Development/jsoncpp/src/test_lib_json/main.cpp(664): kfint32max == val.asFloat()
  Expected: 2147483648
  Actual  : 2147483648
25/26 tests passed (1 failure(s))
make[2]: *** [jsoncpp/bin/jsoncpp_test] Error 1
make[1]: *** [jsoncpp/src/test_lib_json/CMakeFiles/jsoncpp_test.dir/all] Error 2
make: *** [all] Error 2

Is this an issue in the code or maybe specific to my setup?

cdunn2001 commented 9 years ago

Whoa! Those numbers look pretty similar to me. Can you debug this on your system?

You should see 647, not 648. See the String test 2 lines lower? For some reason, on your system kint32max is calculated as 2^31. Look at include/json/config.h. We'll need your help tracking this down. Perhaps compile with -E to see the output of the preprocessor. We run Travis for Continuous Integration, but of course we do not try every O/S and hardware configuration. I hope you can find the cause for this.

emarthinsen commented 9 years ago

Happy to help out.

BTW, I compiled on Ubuntu 12.04.5, 32-bit running in VirtualBox.

Do I pass -E into cmake or into make?

cdunn2001 commented 9 years ago

Tell cmake to show the system-calls. Then, cut and paste the g++ line, and repeat it in a shell. After it's repeatable, add -E to the compile line, and look at the .o file, which will be pure text. Look for the Int and Int64 typedefs. That might be enlightening.

Ultimately, we might need your help to know how to define the types properly for your environment.

rpavlik commented 9 years ago

You can actually generate the preprocessed stuff more easily (reasonably automatically) with a CMake-generated project by going into the directory in the build tree and doing make SourceFileName.i - it'll show up in the CMakeFiles subdirectory IIRC.

emarthinsen commented 9 years ago

Okay, I ran the g++ line with -E. Here's what it looked like:

/usr/bin/c++ -Werror=strict-aliasing -O3 -DNDEBUG -I../../include -o json_reader.cpp.o -c json_reader.cpp -E

I inspected the .o file. Here are the typedefs for Int and Int64.

# 1 "../../include/json/config.h" 1
# 90 "../../include/json/config.h"
namespace Json {
typedef int Int;
typedef unsigned int UInt;
# 103 "../../include/json/config.h"
typedef long long int Int64;
typedef unsigned long long int UInt64;

typedef Int64 LargestInt;
typedef UInt64 LargestUInt;
}

Does this help? Happy to assist in any way.

cdunn2001 commented 9 years ago

Ugh! If we know we have a recent compiler, we can use int32_t. So let's try that.

#include <cstdint>
...
typedef int32_t Int;
typedef uint32_t UInt;

And see whether the tests pass. If so, we just need to create the right #ifdef for your system. You might also need #include <stdint.h> instead of <cstdint>.

cdunn2001 commented 9 years ago

make SourceFileName.i

@rpavlik, that's a nice trick! Thanks.

emarthinsen commented 9 years ago

Looks like the same error. I updated the types to be int32_t and uint32_t (note, I needed stdint.h). I got the same failure on the same test. I inspected the .o file for json_reader.cpp.o and saw these typedefs:

typedef int32_t Int;
typedef uint32_t UInt;

So, it looks like the updated types were incorporated, but they didn't impact the failing test.

Muon commented 9 years ago

Actually, that test will never pass. All single-precision floating point integers with magnitude greater than 2^24 are even since you run out of bits in the mantissa, so 2^31-1 gets rounded to 2^31.

EDIT: No, that's not quite right. I just checked the code, and you are casting the integer to a float. Hmm. (Though what I said about single precision floating point still stands.)

My current hypothesis is that the compiler is screwing up the rounding because you're compiling for 32-bit x86 and what's actually in the FPU is the true value. We don't see the difference because it gets rounded to float for printing. Could you place a breakpoint at that comparison and dump the FPU registers?

illuusio commented 9 years ago

Can you determine how to make it precicely so I can test it..

cdunn2001 commented 9 years ago

Still hoping for help on this. Maybe I will have to start a 32-bit Linux VM.

emarthinsen commented 9 years ago

The steps to reproduce it are pretty straightforward. Just grab the ISO for Ubuntu 12.04.5 (32-bit) and do a default install into a VM. I used VirtualBox. Clone the jsoncpp repo and build it using cmake. It was a while ago, so I don't remember which prerequisites I installed, but I think just build essentials.

illuusio commented 9 years ago

Yes I can get this bug on openSUSE x86 13.2 also. I digged little bit and numbers are equal when you print them out. I sound like compiler bug but because it happens wide range compilers I doub it that nobody else has spotted it..

cdunn2001 commented 9 years ago

All tests pass for me. Here are details of my VM:

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 14.04.1 LTS
Release:    14.04
Codename:   trusty

$ dmesg ...
Linux version 3.13.0-32-generic (buildd@roseapple) (gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1) ) #57-Ubuntu SMP Tue Jul 15 03:51:12 UTC 2014 (Ubuntu 3.13.0-32.57-generic 3.13.11.4)

$ uname -a
Linux cd-VirtualBox 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15 03:51:12 UTC 2014 i686 i686 i686 GNU/Linux

$ file /sbin/init
/sbin/init: ELF 32-bit LSB  shared object, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, BuildID[sha1]=c394677bccc720a3bb4f4c42a48e008ff33e39b1, stripped

I guess I need WAY more details. E.g.

Or can you simply give me shell access to your machine?

cdunn2001 commented 9 years ago

Aha! It fails for the optimized (Release) build.

$ cmake -DCMAKE_BUILD_TYPE=Release -DJSONCPP_LIB_BUILD_SHARED=OFF -DCMAKE_VERBOSE_MAKEFILE=ON -G "Unix Makefiles" ../..
$ make
* Detail of ValueTest/integers test failure:
/home/cd/gh/jsoncpp/src/test_lib_json/main.cpp(676): kfint32max == val.asFloat()
  Expected: 2147483648
  Actual  : 2147483648
25/26 tests passed (1 failure(s))

I can debug this now.

cdunn2001 commented 9 years ago

More info:

$ cmake -DCMAKE_BUILD_TYPE=RELWITHDEBINFO -DJSONCPP_LIB_BUILD_SHARED=OFF -DCMAKE_VERBOSE_MAKEFILE=ON -G "Unix Makefiles" ../..
$ cd src/test_lib_json
$ gdb ./jsoncpp_test
Breakpoint 2, TestValueTestintegers::runTestCase (this=0x809d8b0)
    at /home/cd/gh/jsoncpp/src/test_lib_json/main.cpp:676
676   JSONTEST_ASSERT_EQUAL(kfint32max, val.asFloat());
(gdb) p kfint32max
$1 = 2.14748365e+09
(gdb) p val.asDouble()
$2 = 2147483647
(gdb) p val.asString()
$3 = {static npos = <optimized out>, 
  _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x809daac "2147483647"}}
(gdb) p val
$4 = {static null = @0x8090898, static minLargestInt = -9223372036854775808, 
  static maxLargestInt = 9223372036854775807, 
  static maxLargestUInt = 18446744073709551615, static minInt = -2147483648, 
  static maxInt = 2147483647, static maxUInt = 4294967295, 
  static minInt64 = -9223372036854775808, static maxInt64 = 9223372036854775807, 
  static maxUInt64 = 18446744073709551615, value_ = {int_ = 2147483647, 
    uint_ = 2147483647, real_ = 1.0609978949885705e-314, bool_ = 255, 
    string_ = 0x7fffffff <error: Cannot access memory at address 0x7fffffff>, 
    map_ = 0x7fffffff}, type_ = Json::intValue, allocated_ = 0, comments_ = 0x0, 
  start_ = 0, limit_ = 0}
Muon commented 9 years ago

Sanity check: -ffast-math is off, right?

Something you could try is -mfpmath=sse or -ffloat-store when compiling. If either of those makes the test pass in optimized mode, it's almost certainly a compiler bug.

cdunn2001 commented 9 years ago

Thanks for the suggestions! Results:

So that at least gives us some options. What do you suggest? I don't want to force people to use specific flags, but I guess we could use those only for the tests.

Muon commented 9 years ago

That's odd, that warning suggests that it didn't use SSE. Could you check again? Is there a -march option or -mtune or something?

Only using them for tests would kinda defeat the purpose of the tests, no? :stuck_out_tongue:

cdunn2001 commented 9 years ago

Well, we don't perform any math within the parser, so I think it's ok for the float/SSE settings to be chosen by the user. This particular error has nothing to do with any jsoncpp code. It's strictly the behavior of an if-statement in the test code.

I'll look into -march and -mtune etc. soon. Thanks!

Muon commented 9 years ago

Another way of fixing it on i386 would be to move kfint32max into a separate translation unit and make it external. Unless LTO is performed (and perhaps even then), it should load the rounded value onto the FPU stack, rather than the original, thus making the comparison work. Alternatively, you could explicitly replace it with 2.14748365e+09f (and quite probably even 2147483647.f) instead of float(kint32max).

cdunn2001 commented 9 years ago

I found a simple fix. @Muon's ideas seem fine too. And if this ever recurs, we can apply -ffloat-store.

mcauto commented 3 years ago

@cdunn2001 Hi! I want to use ExternalProject_Add(). How to use -ffloat-store ??