Fix test_collaborative_call_once SIGABRT for all 32-bit targets (#982)

[glaubitz]

Signed-off-by: John Paul Adrian Glaubitz [email protected]

[glaubitz]

FWIW, I noticed that this workaround is required on at least hppa and m68k as well. So we might just enable this workaround for all 32-bit targets.

[phprus]

Maybe check the value of the macro SIZE_MAX (https://en.cppreference.com/w/c/types/limits)?

#include <stdint.h>
#if SIZE_MAX <= 4294967295
// 32 bit
#else
// 64 bit
#endif

32-bit demo: https://godbolt.org/z/1bdWe67qa 64-bit demo: https://godbolt.org/z/aT9zv3cdo

[glaubitz]

Yeah, I actually wanted to send a more generic version later after this was merged.

But we can already do that now as tests have shown that every exotic 32-bit target I have tested so far (hppa, m68k, sh4) is affected by this issue as well.

[glaubitz]

I think it might be better to check for __SIZEOF_POINTER__ == 4 as it's easier to read.

Your version doesn't seem to work properly with clang for powerpc64 when testing for 64 bit.

[phprus]

__SIZEOF_POINTER__ - is non standard and not a portable solution. MSVC does not define this macro.

[glaubitz]

Ah, right, I forgot about MSVC, sorry. I will check what's available on MSVC.

[glaubitz]

Shall I use the suggested test for SIZE_MAX? Alternatively, we could test for _WIN32 and _WIN64 on Windows and _LP64 on Unix targets.

See: https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170

[glaubitz]

Maybe check the value of the macro SIZE_MAX (https://en.cppreference.com/w/c/types/limits)?

#include <stdint.h>
#if SIZE_MAX <= 4294967295
// 32 bit
#else
// 64 bit
#endif

32-bit demo: https://godbolt.org/z/1bdWe67qa 64-bit demo: https://godbolt.org/z/aT9zv3cdo

I have opted for this approach now. Should be good to merge (hopefully) ;).

[glaubitz]

Ping. Any update on this?

[pavelkumbrasev]

Could you please add description to PR? In general such solution looks kinda odd at least for me. Could you define macro in test config.h with meaningful name and use it here. Is sizeof(void*) == 4 acceptable?

[glaubitz]

Could you please add description to PR?

OK.

In general such solution looks kinda odd at least for me. Could you define macro in test config.h with meaningful name and use it here.

I'll look into it.

Is sizeof(void*) == 4 acceptable?

I'm not sure. I think SIZE_MAX is the most portable solution. CC @karcherm

Edit: The answer is simple: SIZE_MAX can be evaluated by the pre-processor while sizeof(void*) can be evaluated at runtime only.

[pavelkumbrasev]

Edit: The answer is simple: SIZE_MAX can be evaluated by the pre-processor while sizeof(void*) can be evaluated at runtime only.

If I'm not mistaken sizeof is compile time evaluation. https://godbolt.org/z/1Phzbc6nh

[glaubitz]

Edit: The answer is simple: SIZE_MAX can be evaluated by the pre-processor while sizeof(void*) can be evaluated at runtime only.

If I'm not mistaken sizeof is compile time evaluation. https://godbolt.org/z/1Phzbc6nh

You're right, of course. But it's still a difference compared to when the expression is evaluated by the pre-processor what the rest of the code seems to do in these situations, no?

I mean, we can use a macro as suggested to make the use of SIZE_MAX more readable.

[phprus]

If I'm not mistaken sizeof is compile time evaluation. https://godbolt.org/z/1Phzbc6nh

At compile time, but not at preprocessing time.

[pavelkumbrasev]

Could we use this approach? (I personally don't like the idea comparing with magical constant) https://stackoverflow.com/a/1505839

[phprus]

@pavelkumbrasev I think yes. We can.

Offtopic: The test_task_arena test has timeout on Windows.

26/138 Test  #26: test_task_arena ..........................***Timeout 185.82 sec
TBB Warning: The number of workers is currently limited to 1. The request for 3 workers is ignored. Further requests for more workers will be silently ignored until the limit changes.

https://github.com/oneapi-src/oneTBB/actions/runs/4112056617/jobs/7096513775

I have reproduced this timeout in following envirinment:

1. Tests are run in a virtual machine.
2. The number of virtual cores is small.
3. The physical machine is heavily overloaded - virtual cores do not work simultaneously, but one after another.

[pavelkumbrasev]

@pavelkumbrasev I think yes. We can.

Offtopic: The test_task_arena test has timeout on Windows.

26/138 Test  #26: test_task_arena ..........................***Timeout 185.82 sec
TBB Warning: The number of workers is currently limited to 1. The request for 3 workers is ignored. Further requests for more workers will be silently ignored until the limit changes.

https://github.com/oneapi-src/oneTBB/actions/runs/4112056617/jobs/7096513775

I have reproduced this timeout in following envirinment:

1. Tests are run in a virtual machine.
2. The number of virtual cores is small.
3. The physical machine is heavily overloaded - virtual cores do not work simultaneously, but one after another.

Currently we are limited with resources and don't have time :( Could you please create Issue for this problem?

[phprus]

@pavelkumbrasev Issue already created: https://github.com/oneapi-src/oneTBB/issues/952 My comment in issue: https://github.com/oneapi-src/oneTBB/issues/952#issuecomment-1336448704

I think the problem of hangs on ARM and multiple x86 test slowdowns on an overloaded system has a similar cause. On x86, these tests don't fully hang, but run 10-100-1000 times slower.

[pavelkumbrasev]

In general such solution looks kinda odd at least for me. Could you define macro in test config.h with meaningful name and use it here.

Could we use this approach? (I personally don't like the idea comparing with magical constant) https://stackoverflow.com/a/1505839

@glaubitz Could you please address these comments and I think we can move forward and merge this patch.

[glaubitz]

In general such solution looks kinda odd at least for me. Could you define macro in test config.h with meaningful name and use it here.

Could we use this approach? (I personally don't like the idea comparing with magical constant) https://stackoverflow.com/a/1505839

@glaubitz Could you please address these comments and I think we can move forward and merge this patch.

I just did. The __arm__ macro is not defined on arm64. I just verified that.

[pavelkumbrasev]

Could you please apply these two changes:

• Define macro in test config.h with meaningful name: like UTILS_ARCH_32 and UTILS_ARCH_64
• Use this approach to define these constants https://stackoverflow.com/a/1505839

[glaubitz]

Could you please apply these two changes:

* Define macro in test config.h with meaningful name: like UTILS_ARCH_32 and UTILS_ARCH_64

* Use this approach to define these constants https://stackoverflow.com/a/1505839

OK, I'll have a look.

[glaubitz]

On Jun 12, 2023, at 6:37 PM, Efraim Flashner @.***> wrote: @Millak commented on this pull request.

In test/tbb/test_collaborative_call_once.cpp:

@@ -285,12 +285,12 @@ TEST_CASE("handles exceptions - state reset") { TEST_CASE("handles exceptions - stress test") { #if TBB_TEST_LOW_WORKLOAD constexpr std::size_t N = 32; -#elif __TBB_x86_32 || arm || ANDROID +#elif SIZE_MAX <= 4294967295 // 32-bit targets

The comment below says that Android was added to decrease testing time. It'd probably be better then to do: (SIZE_MAX <= 4294967295) || ANDROID // 32-bit targets or Android Thanks. I will get around to do it after dinner now.