Jump to content

Leaderboard

Popular Content

Showing content with the highest reputation on 01/01/2021 in all areas

  1. Paul Floyd

    A few Valgrind issues

    Here is a full analysis of the first problem, and a proposed patch. Here's what I saw running the executable under Valgrind via the GDB server (this is required to execute the mo(nitor) command). 257 result_p->digit = (sc_digit*)sc_core::sc_temp_heap.allocate( 258 sizeof(sc_digit)*result_p->ndigits ); 259 #if defined(_MSC_VER) 260 // workaround spurious initialisation issue on MS Visual C++ 261 memset( result_p->digit, 0, sizeof(sc_digit)*result_p->ndigits ); 262 #else 263 result_p->digit[result_p->ndigits-1] = 0; 264 #endif >265 right_non_zero = m_right_p->concat_get_data( result_p->digit, 0 ); The above is copied from gdb. result_p->nbits is 95, result_p->ndigits is 4 and result_p->digit contains all zeroes (gdb) x /4x result_p->digit 0x5825bb0: 0x00000000 0x00000000 0x00000000 0x00000000 However, the 1st 3 words are uninitialized, only the 4th word is initialized, as seen on line 263 (gdb) mo xb 0x5825bb0 16 ff ff ff ff ff ff ff ff 0x5825BB0: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 ff ff ff ff 00 00 00 00 0x5825BB8: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 Inside concat_get_data we execute >337 dst_p[dst_i] = dst_p[dst_i] & ~mask; Where dst_i is 0, mask is 0x3fffffff, dst_p[dst_i] is all uninitialized After this assignment, the lower 30 bits are initialized but the top 2 bits are still uninitialized: (gdb) p &dst_p[dst_i] $27 = (sc_dt::sc_digit *) 0x5825bb0 (gdb) mo xb 0x5825bb0 4 00 00 00 c0 0x5825BB0: 0x00 0x00 0x00 0x00 However, I don't think that the problem is here. The next call 266 left_non_zero = m_left_p->concat_get_data(result_p->digit, m_len_r); is where I reckon those two bits ought to be written to, but aren't. Watch this space. Update: In the second call to concat_get_data, the first word of dst_p doesn't get written (since dst_i is 1 rather than 0). So in summary, the 1st call to concat_get_data doesn't initialize all of word 0 of dst_p but then the 2nd call assumes that it was all initialized. For comparison, the concatenation with 29 bits behaves in a similar fashion to 30 bits for the 1st call, bit the second call has dst_i set to 0 so word 0 does get fully initialized. In the case of 31 bits, a different path is taken in in the first call to concat_get_data (sc_unsigned.cpp around line 352) which writes 2 whole words rather than just part of 1 word. I suggest as a fix changing this block // ALL DATA TO BE MOVED IS IN A SINGLE WORD: if ( dst_i == end_i ) { mask = ~(~0U << real_bits) << left_shift; dst_p[dst_i] = dst_p[dst_i] & ~mask; } to be // ALL DATA TO BE MOVED IS IN A SINGLE WORD: if ( dst_i == end_i ) { if ( left_shift == 0 ) { dst_p[dst_i] = 0; } else { mask = ~(~0U << real_bits) << left_shift; dst_p[dst_i] = dst_p[dst_i] & ~mask; } } (and the same thing for signed) It is not easy to test this thoroughly. The problem is with the circular buffer, which reuses memory. The re-used memory never becomes uninitialized, so to check that I made this change +#include "/usr/home/paulf/tools/valgrind/include/valgrind/memcheck.h" + template<class T> T* sc_vpool<T>::allocate() { T* result_p; // Entry to return. result_p = &m_pool_p[m_pool_i]; + + VALGRIND_MAKE_MEM_UNDEFINED(result_p, sizeof(T)); + m_pool_i = (m_pool_i + 1) & m_wrap; return result_p; } I've attached a patch that seems to solve the issue. Lastly, this can also probably be changed #if defined(_MSC_VER) // workaround spurious initialisation issue on MS Visual C++ memset( result_p->digit, 0, sizeof(sc_digit)*result_p->ndigits ); #else result_p->digit[result_p->ndigits-1] = 0; #endif The comment is al least half wrong - it is a genuine workaround. With the attached patch the memset should not be necessary. concat_init.patch
    1 point
×
×
  • Create New...