morganstanley / hobbes

A language and an embedded JIT compiler
http://hobbes.readthedocs.io/
Apache License 2.0
1.16k stars 105 forks source link

fix reading before initialization #428

Closed mo-xiaoming closed 2 years ago

mo-xiaoming commented 2 years ago

Ran hobbes-test --tests Storage with valgrind, got two types of errors

==1021546== Conditional jump or move depends on uninitialised value(s)
==1021546==    at 0x76024D: hobbes::UCReader::loadNextNode() (cbindings.C:288)
==1021546==    by 0x760389: hobbes::UCReader::loadReadState(unsigned long) (cbindings.C:284)
==1021546==    by 0x7603E0: hobbes::UCReader::UCReader(unsigned long, unsigned long, unsigned long) (cbindings.C:209)
==1021546==    by 0x75CECB: hobbes::makeUCReader(unsigned long, unsigned long, unsigned long) (cbindings.C:305)

and

==1021546== Conditional jump or move depends on uninitialised value(s)
==1021546==    at 0x53EDD1: std::_Rb_tree<char*, std::pair<char* const, hobbes::fregion::falloc>, std::_Select1st<std::pair<char* const, hobbes::fregion::falloc> >, std::less<char*>, std::allocator<std::pair<char* const, hobb
es::fregion::falloc> > >::_M_lower_bound(std::_Rb_tree_node<std::pair<char* const, hobbes::fregion::falloc> >*, std::_Rb_tree_node_base*, char* const&) (stl_tree.h:1935)
==1021546==    by 0x53EEAC: std::_Rb_tree<char*, std::pair<char* const, hobbes::fregion::falloc>, std::_Select1st<std::pair<char* const, hobbes::fregion::falloc> >, std::less<char*>, std::allocator<std::pair<char* const, hobb
es::fregion::falloc> > >::find(char* const&) (stl_tree.h:2553)
==1021546==    by 0x53EF20: std::map<char*, hobbes::fregion::falloc, std::less<char*>, std::allocator<std::pair<char* const, hobbes::fregion::falloc> > >::find(char* const&) (stl_map.h:1170)
==1021546==    by 0x53EF60: hobbes::fregion::unmapFileData(hobbes::fregion::imagefile*, void const*, unsigned long) (fregion.H:694)
==1021546==    by 0x76025C: hobbes::UCReader::loadNextNode() (cbindings.C:289)
==1021546==    by 0x760389: hobbes::UCReader::loadReadState(unsigned long) (cbindings.C:284)
==1021546==    by 0x7603E0: hobbes::UCReader::UCReader(unsigned long, unsigned long, unsigned long) (cbindings.C:209)
==1021546==    by 0x75CECB: hobbes::makeUCReader(unsigned long, unsigned long, unsigned long) (cbindings.C:305)
mo-xiaoming commented 2 years ago

when std::array used as a std::map key, all elements in array are used, they must be initialized

mo-xiaoming commented 2 years ago

There is another error reported by valgrind in Arrays.C:WithStdVector

==3779368== Invalid read of size 32
==3779368==    at 0xAE3C06C: ???
==3779368==    by 0x51F7ED: TestCoord::runTestGroups(Args const&) (Main.C:64)
==3779368==    by 0x5203C0: main (Main.C:143)
==3779368==  Address 0x4e61d50 is 0 bytes inside a block of size 4 alloc'd
==3779368==    at 0x483EDA9: operator new(unsigned long) (in /nix/store/j5dkzyl1hq51rk84mmw9yn4h5kx7n1iv-valgrind-3.16.1/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3779368==    by 0x479825: __gnu_cxx::new_allocator<int>::allocate(unsigned long, void const*) (new_allocator.h:115)
==3779368==    by 0x478DD2: std::allocator_traits<std::allocator<int> >::allocate(std::allocator<int>&, unsigned long) (alloc_traits.h:460)
==3779368==    by 0x478123: std::_Vector_base<int, std::allocator<int> >::_M_allocate(unsigned long) (stl_vector.h:346)
==3779368==    by 0x476AB0: void std::vector<int, std::allocator<int> >::_M_range_initialize<int const*>(int const*, int const*, std::forward_iterator_tag) (stl_vector.h:1582)
==3779368==    by 0x475AA1: std::vector<int, std::allocator<int> >::vector(std::initializer_list<int>, std::allocator<int> const&) (stl_vector.h:629)
==3779368==    by 0x4744B5: test_Arrays_WithStdVector() (Arrays.C:54)
==3779368==    by 0x51F7ED: TestCoord::runTestGroups(Args const&) (Main.C:64)
==3779368==    by 0x5203C0: main (Main.C:143)

I think this is a false positive, and can be safely suppressed. The following line is the offender

0xae3c06c:   vmovups (%r12),%ymm0

(%r12) is an address on 32bytes boundary inside (or on the right boundary) vector data, and jitted code tries to load following data by vectorization, it always tries to load it in a 32bytes batch. valgrind considers this as a reading violation when reading after vector boundary

There is a very old valgrind bug report, Bug 264936 - vectorization might lead to 'false positives', after 10 years, it is still there