rcsb / mmtf-cpp

The pure C++ implementation of the MMTF API, decoder and encoder.
MIT License
21 stars 24 forks source link

Segmentation fault with -march=native #36

Closed Luthaf closed 3 years ago

Luthaf commented 4 years ago

OS: Ubuntu 18.04 LTS Compiler: GCC 7.4.0 (7.4.0-1ubuntu1~18.04.1) CPU: Intel(R) Core(TM) i5-4460S CPU @ 2.90GHz

Compiling the code with -march=native segfaults in the functions dealing with endianness. To reproduce:

cd mmtf-cpp
mkdir build && cd build
cmake -DBUILD_TEST=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS='-march=native' ..
make && ctest

Here is the corresponding LLDB session

(lldb) target create "./tests/mmtf_tests"
Current executable set to './tests/mmtf_tests' (x86_64).
(lldb) r
Process 29356 launched: './tests/mmtf_tests' (x86_64)
Process 29356 stopped
* thread #1, name = 'mmtf_tests', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x00005555555bd210 mmtf_tests`mmtf::BinaryDecoder::decodeFromBytes_(std::vector<int, std::allocator<int> >&) [inlined] __bswap_32(__bsx=<unavailable>) at byteswap.h:47
   44   static __inline unsigned int
   45   __bswap_32 (unsigned int __bsx)
   46   {
-> 47     return __builtin_bswap32 (__bsx);
   48   }
   49   # elif __GNUC__ >= 2
   50   #  if __WORDSIZE == 64 || (defined __i486__ || defined __pentium__        \
(lldb) bt
* thread #1, name = 'mmtf_tests', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x00005555555bd210 mmtf_tests`mmtf::BinaryDecoder::decodeFromBytes_(std::vector<int, std::allocator<int> >&) [inlined] __bswap_32(__bsx=<unavailable>) at byteswap.h:47
    frame #1: 0x00005555555bd210 mmtf_tests`mmtf::BinaryDecoder::decodeFromBytes_(std::vector<int, std::allocator<int> >&) [inlined] assignBigendian4(src=<unavailable>, dst=<unavailable>) at binary_decoder.hpp:124
    frame #2: 0x00005555555bd210 mmtf_tests`mmtf::BinaryDecoder::decodeFromBytes_(std::vector<int, std::allocator<int> >&) at binary_decoder.hpp:151
    frame #3: 0x00005555555bd10d mmtf_tests`mmtf::BinaryDecoder::decodeFromBytes_(this=0x0000555555858601, output=size=52) at binary_decoder.hpp:440
    frame #4: 0x00005555555bfde5 mmtf_tests`void mmtf::BinaryDecoder::decode<std::vector<int, std::allocator<int> > >(this=0x00007fffffffc520, output=size=52) at binary_decoder.hpp:304
    frame #5: 0x00005555555c0109 mmtf_tests`void mmtf::MapDecoder::decode<std::vector<int, std::allocator<int> > >(this=0x00007fffffffc880, key="bondAtomList", required=<unavailable>, target=size=52) const at map_decoder.hpp:196
    frame #6: 0x00005555555e77e3 mmtf_tests`mmtf::impl::decodeFromMapDecoder(data=0x00007fffffffccc0, md=0x00007fffffffc880) at msgpack_decoders.hpp:55
    frame #7: 0x00005555555e8315 mmtf_tests`mmtf::decodeFromFile(mmtf::StructureData&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) [inlined] mmtf::decodeFromMapDecoder(md=<unavailable>, data=<unavailable>) at decoder.hpp:103
    frame #8: 0x00005555555e830a mmtf_tests`mmtf::decodeFromFile(data=0x00007fffffffccc0, filename=<unavailable>) at decoder.hpp:123
    frame #9: 0x00005555555a0c3e mmtf_tests`::____C_A_T_C_H____T_E_S_T____0() at mmtf_tests.cpp:47
    frame #10: 0x000055555556bd5e mmtf_tests`Catch::RunContext::invokeActiveTestCase() [inlined] Catch::TestInvokerAsFunction::invoke(this=<unavailable>) const at catch.hpp:9918
    frame #11: 0x000055555556bd5b mmtf_tests`Catch::RunContext::invokeActiveTestCase() [inlined] Catch::TestCase::invoke(this=<unavailable>) const at catch.hpp:9819
    frame #12: 0x000055555556bd3e mmtf_tests`Catch::RunContext::invokeActiveTestCase(this=0x00007fffffffdd60) at catch.hpp:8701
    frame #13: 0x0000555555578b50 mmtf_tests`Catch::RunContext::runCurrentTest(this=0x00007fffffffdd60, redirectedCout="\x90����, redirectedCerr="\xb0����) at catch.hpp:8675
    frame #14: 0x0000555555593641 mmtf_tests`Catch::RunContext::runTest(this=0x00007fffffffdd60, testCase=<unavailable>) at catch.hpp:8464
    frame #15: 0x0000555555593dc6 mmtf_tests`Catch::(anonymous namespace)::runTests(config=std::__shared_ptr<Catch::Config, 4>::element_type @ 0x00005555558426a0) at catch.hpp:9006
    frame #16: 0x00005555555954e1 mmtf_tests`Catch::Session::runInternal(this=0x00007fffffffe0b0) at catch.hpp:9193
    frame #17: 0x00005555555955ca mmtf_tests`Catch::Session::run(this=0x00007fffffffe0b0) at catch.hpp:9147
    frame #18: 0x0000555555564823 mmtf_tests`main at catch.hpp:9115
    frame #19: 0x000055555556481b mmtf_tests`main
    frame #20: 0x0000555555564814 mmtf_tests`main(argc=<unavailable>, argv=0x00007fffffffe308) at catch.hpp:12496
    frame #21: 0x00007ffff706fb97 libc.so.6`__libc_start_main(main=(mmtf_tests`main at catch.hpp:12493), argc=1, argv=0x00007fffffffe308, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffe2f8) at libc-start.c:310
    frame #22: 0x0000555555565caa mmtf_tests`_start + 42

I have no idea what is going on here =/

Luthaf commented 4 years ago

Adding -fsanitize=undefined to the compiler flags gives something potentially related (same file/line)?

mmtf-cpp/include/mmtf/binary_decoder.hpp:124:25: runtime error: load of misaligned address 0x55a301b045f1 for type 'uint32_t', which requires 4 byte alignment
0x55a301b045f1: note: pointer points here
 4c 69 73  74 00 00 00 04 00 00 00  34 00 00 00 00 00 00 00  13 00 00 00 05 00 00 00  28 00 00 00 1b
              ^ 
mmtf-cpp/include/mmtf/binary_decoder.hpp:124:25: runtime error: load of misaligned address 0x55a301b045f5 for type 'uint32_t', which requires 4 byte alignment
0x55a301b045f5: note: pointer points here
 00 00 00 04 00 00 00  34 00 00 00 00 00 00 00  13 00 00 00 05 00 00 00  28 00 00 00 1b 00 00 00  3d
             ^ 
mmtf-cpp/include/mmtf/binary_decoder.hpp:124:25: runtime error: load of misaligned address 0x55a301b045f9 for type 'uint32_t', which requires 4 byte alignment
0x55a301b045f9: note: pointer points here
 00 00 00  34 00 00 00 00 00 00 00  13 00 00 00 05 00 00 00  28 00 00 00 1b 00 00 00  3d 00 00 00 30
              ^ 
mmtf-cpp/include/mmtf/binary_decoder.hpp:124:25: runtime error: load of misaligned address 0x55a301b045fd for type 'uint32_t', which requires 4 byte alignment
0x55a301b045fd: note: pointer points here
 00 00 00 00 00 00 00  13 00 00 00 05 00 00 00  28 00 00 00 1b 00 00 00  3d 00 00 00 30 00 00 00  53
             ^ 
mmtf-cpp/include/mmtf/binary_decoder.hpp:128:25: runtime error: load of misaligned address 0x55a301b0c9c1 for type 'uint16_t', which requires 2 byte alignment
0x55a301b0c9c1: note: pointer points here
 00 00 00  64 03 b4 00 8c 00 00 00  00 00 00 ff 74 00 8c 01  e5 00 00 00 00 00 00 00  00 00 00 00 00
gtauriello commented 4 years ago

Just so I understand this correctly: does the code work if you drop the -march=native?

Luthaf commented 4 years ago

Yes. Also, I am compiling in Release mode (-O3), I'll edit the first post with this.

Luthaf commented 4 years ago

Looks like the issue was undefined behavior of type punning in C++ (it breaks strict aliasing). The patch below fixes the issue for me (it should have the same performace, the compiler can recognize calls to memcpy)

diff --git a/include/mmtf/binary_decoder.hpp b/include/mmtf/binary_decoder.hpp
index 00b39bb..84d40b8 100644
--- a/include/mmtf/binary_decoder.hpp
+++ b/include/mmtf/binary_decoder.hpp
@@ -121,11 +121,17 @@ namespace {

 #ifndef __EMSCRIPTEN__
 void assignBigendian4(void* dst, const char* src) {
-    *((uint32_t*)dst) = ntohl(*((uint32_t*)src));
+    uint32_t tmp;
+    std::memcpy(&tmp, src, sizeof(uint32_t));
+    tmp = ntohl(tmp);
+    std::memcpy(dst, &tmp, sizeof(uint32_t));
 }

 void assignBigendian2(void* dst, const char* src) {
-    *((uint16_t*)dst) = ntohs(*((uint16_t*)src));
+    uint16_t tmp;
+    std::memcpy(&tmp, src, sizeof(uint16_t));
+    tmp = ntohs(tmp);
+    std::memcpy(dst, &tmp, sizeof(uint16_t));
 }
 #else
 // Need to avoid how emscripten handles memory
gtauriello commented 3 years ago

@Luthaf the issue has been fixed in the master branch now. Thanks once again for bringing this to our attention.

Luthaf commented 3 years ago

Thanks for fixing this!

pwrose commented 3 years ago

Thanks, Gerardo!

On Fri, Oct 23, 2020 at 6:15 AM Guillaume Fraux notifications@github.com wrote:

Thanks for fixing this!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://urldefense.com/v3/__https://github.com/rcsb/mmtf-cpp/issues/36*issuecomment-715333578__;Iw!!Mih3wA!VpPKXwUYz-5L5l8WXpPPh5GH3C8d0qxq0G_kVDKDZGEalhGfjw4EUgyPPhCAwaY$, or unsubscribe https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AA53AEEZCOWE7QHOWBGU2P3SMF6XTANCNFSM4KXF4H2A__;!!Mih3wA!VpPKXwUYz-5L5l8WXpPPh5GH3C8d0qxq0G_kVDKDZGEalhGfjw4EUgyPl60RaxQ$ .