From d82326162166849cf208b6d82338a549fff0a194 Mon Sep 17 00:00:00 2001 From: Eloy Romero Date: Sun, 14 Mar 2021 09:03:28 -0400 Subject: [PATCH] Fix undefined behaviour in MapObjectDisk: reading a different union member than the one set; only intel compiler 2019 without flag -align seems to cause the spoiling of the read and written objects. --- include/qdp_map_obj_disk.h | 61 ++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/include/qdp_map_obj_disk.h b/include/qdp_map_obj_disk.h index 70ae45c24..480da102e 100644 --- a/include/qdp_map_obj_disk.h +++ b/include/qdp_map_obj_disk.h @@ -8,6 +8,7 @@ #define __qdp_map_obj_disk_h__ #include "qdp_map_obj.h" +#include #include namespace QDP @@ -123,11 +124,13 @@ namespace QDP private: //! Sometime ago, chose 16 bytes for stream positions. Yup, really big. - union priv_pos_type_t - { - unsigned char c[16]; - uint64_t p; - }; + /** + * NOTE: the intention was to simulate an uint128_t with a pair of uint64_t + * where the most significant bytes were set to zero always. But the code + * was not correctly ported to little endian machines and the positions have + * been stored on the most significant uint64_t. + */ + typedef std::array priv_pos_type_t; //! Type for the map typedef std::unordered_map MapType_t; @@ -155,7 +158,7 @@ namespace QDP //! Reader and writer interfaces mutable BinaryFileReaderWriter streamer; - + //! Convert to known size priv_pos_type_t convertToPrivate(const pos_type& input) const; @@ -207,9 +210,11 @@ namespace QDP typename MapObjectDisk::priv_pos_type_t MapObjectDisk::convertToPrivate(const pos_type& input) const { - priv_pos_type_t f; - f.p = static_cast(input); - return f; + const pos_type max_uint64 = pos_type(std::numeric_limits::max()) + pos_type(1u); + uint64_t less = uint64_t(input % max_uint64); + uint64_t more = input / max_uint64; + // NOTE comment on priv_pos_type_t + return (QDPUtil::big_endian() ? priv_pos_type_t{more, less} : priv_pos_type_t{less, more}); } //! Convert from known size @@ -217,7 +222,12 @@ namespace QDP typename MapObjectDisk::pos_type MapObjectDisk::convertFromPrivate(const priv_pos_type_t& input) const { - return static_cast(input.p); + const pos_type max_uint64 = pos_type(std::numeric_limits::max()) + pos_type(1u); + // NOTE comment on priv_pos_type_t + if (!QDPUtil::big_endian()) + return pos_type(input[0]) + pos_type(input[1]) * max_uint64; + else + return pos_type(input[1]) + pos_type(input[0]) * max_uint64; } @@ -293,8 +303,8 @@ namespace QDP QDPInternal::broadcast(user_len); QDPIO::cout << "Sanity Check 1" << std::endl; ; - uint64_t cur_pos = convertToPrivate(streamer.currentPosition()).p; - uint64_t exp_pos = + pos_type cur_pos = streamer.currentPosition(); + pos_type exp_pos = MapObjDiskEnv::getFileMagic().length()+sizeof(int) +user_len+sizeof(int) +sizeof(MapObjDiskEnv::file_version_t); @@ -317,8 +327,8 @@ namespace QDP QDPInternal::broadcast(user_len); QDPIO::cout << "Sanity Check 2" << std::endl; - uint64_t cur_pos = convertToPrivate(streamer.currentPosition()).p; - uint64_t exp_pos = + pos_type cur_pos = streamer.currentPosition(); + pos_type exp_pos = MapObjDiskEnv::getFileMagic().length()+sizeof(int) +user_len+sizeof(int) +sizeof(MapObjDiskEnv::file_version_t) @@ -574,11 +584,11 @@ namespace QDP // Key does not exist // Make note of current writer position - priv_pos_type_t pos = convertToPrivate(streamer.currentPosition()); + pos_type pos = streamer.currentPosition(); // Insert pos into map - src_map.insert(std::make_pair(bin.str(),pos)); - + src_map.insert(std::make_pair(bin.str(), convertToPrivate(pos))); + streamer.resetChecksum(); // Add position to the map @@ -591,8 +601,8 @@ namespace QDP // Get diagnostics. if (level >= 1) { - priv_pos_type_t end_pos = convertToPrivate(streamer.currentPosition()); - double MiBWritten = (double)(end_pos.p - pos.p)/(double)(1024*1024); + pos_type end_pos = streamer.currentPosition(); + double MiBWritten = (double)(end_pos - pos)/(double)(1024*1024); double time = swatch.getTimeInSeconds(); QDPIO::cout << " wrote: " << MiBWritten << " MiB. Time: " << time << " sec. Write Bandwidth: " << MiBWritten/time<second; + pos_type pos = convertFromPrivate(key_ptr->second); // Do the seek and time it StopWatch swatch; swatch.reset(); swatch.start(); - streamer.seek(convertFromPrivate(pos)); + streamer.seek(pos); swatch.stop(); double seek_time = swatch.getTimeInSeconds(); @@ -658,7 +668,7 @@ namespace QDP streamer.resetChecksum(); // Grab start pos: We've just seeked it - priv_pos_type_t start_pos = pos; + pos_type start_pos = pos; // Time the read swatch.reset(); @@ -667,11 +677,11 @@ namespace QDP swatch.stop(); double read_time = swatch.getTimeInSeconds(); - priv_pos_type_t end_pos = convertToPrivate(streamer.currentPosition()); + pos_type end_pos = streamer.currentPosition(); // Print data if (level >= 1) { - double MiBRead = (double)(end_pos.p - start_pos.p)/(double)(1024*1024); + double MiBRead = (double)(end_pos - start_pos)/(double)(1024*1024); QDPIO::cout << " seek time: " << seek_time << " sec. read time: " << read_time << " " << MiBRead <<" MiB, " << MiBRead/read_time << " MiB/sec" << std::endl; @@ -765,8 +775,7 @@ namespace QDP typename MapObjectDisk::priv_pos_type_t MapObjectDisk::readCheckHeader(void) { - priv_pos_type_t md_position; - bzero(&md_position.c, sizeof(priv_pos_type_t)); + priv_pos_type_t md_position{0, 0}; if( streamer.is_open() ) {