efficient / libcuckoo

A high-performance, concurrent hash table
Other
1.62k stars 275 forks source link

Warnings and and test failure in test_resize on x86 Linux #137

Closed slacka closed 2 years ago

slacka commented 3 years ago

When building with Arch 32, I get the following warnings:

[ 62%] Building CXX object tests/unit-tests/CMakeFiles/unit_tests.dir/test_resize.cc.o In file included from /libcuckoo/tests/unit-tests/test_resize.cc:3: /libcuckoo/tests/unit-tests/test_resize.cc: In function 'void C_A_T_C_HT_E_S_T____33()': /libcuckoo/tests/unit-tests/test_resize.cc:49:28: warning: conversion from 'long long unsigned int' to 'size_t' {aka 'unsigned int'} changes value from '8589934592' to '0' [-Woverflow] 49 | (1ULL << 31) slot_per_bucket) == 31); | ~~~^~~~~~~ /libcuckoo/tests/unit-tests/test_resize.cc:51:34: warning: conversion from 'long long unsigned int' to 'size_t' {aka 'unsigned int'} changes value from '8589934596' to '4' [-Woverflow] 51 | ((1ULL << 31) + 1) slot_per_bucket) == 32); | ~~~~~^~~~~~~ /libcuckoo/tests/unit-tests/test_resize.cc:54:28: warning: conversion from 'long long unsigned int' to 'size_t' {aka 'unsigned int'} changes value from '9223372036854775808' to '0' [-Woverflow] 54 | (1ULL << 61) slot_per_bucket) == 61); | ~~~^~~~~~~ /libcuckoo/tests/unit-tests/test_resize.cc:56:34: warning: conversion from 'long long unsigned int' to 'size_t' {aka 'unsigned int'} changes value from '9223372036854775812' to '4' [-Woverflow] 56 | ((1ULL << 61) + 1) slot_per_bucket) == 62); | ~~~~~^~~~~~~

A make test fails with

/libcuckoo/tests/unit-tests/test_resize.cc:33 ...............................................................................

/libcuckoo/tests/unit-tests/test_resize.cc:48: FAILED: REQUIRE( UnitTestInternalAccess::reserve_calc( (1ULL << 31) * slot_per_bucket) == 31 ) with expansion: 0 == 31

=============================================================================== test cases: 79 | 78 passed | 1 failed assertions: 203952 | 203951 passed | 1 failed

Test time = 15.10 sec ---------------------------------------------------------- Test Failed.
manugoyal commented 3 years ago

Thanks for reporting! I tested this branch out on my machine, but it would be great if you could verify that these changes fix the warnings: https://github.com/efficient/libcuckoo/commit/fd7feea190db5625b1c5f5a85ee36733437aa631.

reneengelhard commented 3 years ago

since @slacka diddn't answer and I saw the same issue in Debian 32bit (see https://qa.debian.org/excuses.php?package=libcuckoo - regression is not really the right name, but anyway.... See https://ci.debian.net/data/autopkgtest/testing/armhf/libc/libcuckoo/16947291/log.gz and https://ci.debian.net/data/autopkgtest/testing/i386/libc/libcuckoo/16947441/log.gz) I tried it just now.

Jup, still happens with the patch, see my other reply

reneengelhard commented 3 years ago

patch applied - no build warning actually but still

test 1
      Start  1: unit_tests

1: Test command: /home/rene/Debian/Pakete/libcuckoo/libcuckoo/tests/unit-tests/unit_tests
1: Test timeout computed to be: 10000000
1: 
1: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1: unit_tests is a Catch v1.5.9 host application.
1: Run with -? for options
1: 
1: -------------------------------------------------------------------------------
1: reserve calc
1: -------------------------------------------------------------------------------
1: /home/rene/Debian/Pakete/libcuckoo/libcuckoo/tests/unit-tests/test_resize.cc:33
1: ...............................................................................
1: 
1: /home/rene/Debian/Pakete/libcuckoo/libcuckoo/tests/unit-tests/test_resize.cc:48: FAILED:
1:   REQUIRE( UnitTestInternalAccess::reserve_calc<IntIntTable>( (static_cast<size_t>(1) << 31) * slot_per_bucket) == 31 )
1: with expansion:
1:   0 == 31
1: 
1: ===============================================================================
1: test cases:     79 |     78 passed | 1 failed
1: assertions: 203952 | 203951 passed | 1 failed
1: 
 1/10 Test  #1: unit_tests .......................***Failed    3.51 sec

happens.

manugoyal commented 2 years ago

Thanks for the reminder @reneengelhard. I was able to recompile with a 32-bit architecture and reproduce the issue. I tried fixing this in a more architecture-agnostic way: c749c88864d286c3d875b1de2a082355b7838af7.

Let me know if this works on your machine, and I can merge it in!

reneengelhard commented 2 years ago

thanks @manugoyal for revisiting :)

Indeed that commit fixes both the warnings and the test failure

manugoyal commented 2 years ago

Great, just merged!