Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ck_ec test fails on 32-bit musl systems #208

Open
nmeum opened this issue Apr 3, 2023 · 1 comment
Open

ck_ec test fails on 32-bit musl systems #208

nmeum opened this issue Apr 3, 2023 · 1 comment

Comments

@nmeum
Copy link

nmeum commented Apr 3, 2023

While working on enabling the concurrencykit test suite for our Alpine Linux package, I noticed that the tests fail on 32-bit architectures. More specifically, the issue is the ck_ec regression test which fails with the following error message:

$ uname -m
i686
$ make -C regressions/ck_ec/validate check
./ck_ec_smoke_test
test_update_counter SP passed.
test_update_counter MP passed.
test_deadline passed.
test_wait passed.
pred wait: 0.002000
pred wait: 0.016000
pred wait: 0.128000
pred wait: 1.000000
test_wait_pred passed.
test_threaded SP passed.
test_threaded MP passed.
./prop_test_slow_wakeup -max_total_time=60
./prop_test_timeutil_add -max_total_time=60
Assertion failed: (uint32_t)actual.tv_sec == (uint32_t)(nanos / (NSEC_MAX + 1)) (prop_test_timeutil_add.c: test_timespec_add: 94)
make: *** [Makefile:28: check] Aborted

Within the prop_test_timeutil_add.c test the problem is the following test case:

{
TIME_MAX - 1,
1000
},
{
2,
NSEC_MAX
}

This test case is supposed to check that timespec_add clamps its return values to TIME_MAX / NSEC_MAX on overflow. This is achieved by testing the return value of that function against ts_to_nanos which is defined as follows:

static dword_t ts_to_nanos(const struct timespec ts)
{
return (dword_t)ts.tv_sec * (NSEC_MAX + 1) + ts.tv_nsec;
}

The type dword_t is probably supposed to mean “double word” (or something along those lines) and should presumably be twice as large (in terms of byte size) as time_t. The dword_t type is defined here:

#if ULONG_MAX > 4294967295
typedef unsigned __int128 dword_t;
#else
typedef uint64_t dword_t;
#endif

The implicit assumption here being that time_t is a 32-bit value. However, TIME_MAX does not make this assumption and is defined over sizeof(time_t) as follows:

#define TIME_MAX ((time_t)((1ULL << ((sizeof(time_t) * CHAR_BIT) - 1)) - 1))

This causes the overflow handling of ts_to_nanos to not match the handling of timespec_add if time_t is a 64-bit value on a 32-bit architecture. This is exactly the case on musl libc based systems (like Alpine Linux) since, contrary to glibc, musl-based 32-bit systems are 2038-ready and hence use a 64-bit time_t. Therefore, the test case fails on 32-bit Alpine Linux systems:

(gdb) n
94                      assert(actual.tv_sec == (time_t)(nanos / (NSEC_MAX + 1)));
(gdb) p actual
$1 = {tv_sec = 9223372036854775807, tv_nsec = 999999999}
(gdb) p nanos
$2 = 1000000999
(gdb) n
Assertion failed: actual.tv_sec == (time_t)(nanos / (NSEC_MAX + 1)) (prop_test_timeutil_add.c: test_timespec_add: 94)

I ran into this on concurrencykit 0.7.1, but this should be reproducible with current Git HEAD as well. Since __int128 cannot be used on i686 I am not sure how to best fix this...

P.S.: The same issue applies to prop_test_timeutil_add_ns.c.

@sbahra
Copy link
Member

sbahra commented Sep 12, 2024

@nmeum Any chance we could get a patch? Happy to assist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants