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

bug fix - When TBB_USE_EXCEPTIONS is disabled task_dispatcher fails to link #864

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fran6co
Copy link

@fran6co fran6co commented Aug 2, 2022

Description

When TBB_USE_EXCEPTIONS is disabled task_dispatcher fails to link.

ld.lld: error: undefined symbol: tbb::detail::r1::do_throw_noexcept(void (*)())
>>> referenced by task_dispatcher.h:358 (/tmp/apg/external/tbb/src/tbb/task_dispatcher.h:358)
>>>               arena.cpp.o:(tbb::detail::d1::task* tbb::detail::r1::task_dispatcher::local_wait_for_all<true, tbb::detail::r1::outermost_worker_waiter>(tbb::detail::d1::task*, tbb::detail::r1::outermost_worker_waiter&)) in archive ../../gnu_9.3_cxx17_64_debug/libtbb_debug.a
>>> referenced by task_dispatcher.h:358 (/tmp/apg/external/tbb/src/tbb/task_dispatcher.h:358)
>>>               arena.cpp.o:(tbb::detail::d1::task* tbb::detail::r1::task_dispatcher::local_wait_for_all<false, tbb::detail::r1::outermost_worker_waiter>(tbb::detail::d1::task*, tbb::detail::r1::outermost_worker_waiter&)) in archive ../../gnu_9.3_cxx17_64_debug/libtbb_debug.a
>>> referenced by task_dispatcher.h:358 (/tmp/apg/external/tbb/src/tbb/task_dispatcher.h:358)
>>>               task_dispatcher.cpp.o:(tbb::detail::d1::task* tbb::detail::r1::task_dispatcher::local_wait_for_all<true, tbb::detail::r1::external_waiter>(tbb::detail::d1::task*, tbb::detail::r1::external_waiter&)) in archive ../../gnu_9.3_cxx17_64_debug/libtbb_debug.a
>>> referenced by task_dispatcher.h:358 (/tmp/apg/external/tbb/src/tbb/task_dispatcher.h:358)
>>>               task_dispatcher.cpp.o:(tbb::detail::d1::task* tbb::detail::r1::task_dispatcher::local_wait_for_all<false, tbb::detail::r1::external_waiter>(tbb::detail::d1::task*, tbb::detail::r1::external_waiter&)) in archive ../../gnu_9.3_cxx17_64_debug/libtbb_debug.a
>>> referenced by task_dispatcher.h:358 (/tmp/apg/external/tbb/src/tbb/task_dispatcher.h:358)
>>>               task_dispatcher.cpp.o:(tbb::detail::d1::task* tbb::detail::r1::task_dispatcher::local_wait_for_all<true, tbb::detail::r1::coroutine_waiter>(tbb::detail::d1::task*, tbb::detail::r1::coroutine_waiter&)) in archive ../../gnu_9.3_cxx17_64_debug/libtbb_debug.a
>>> referenced by task_dispatcher.h:358 (/tmp/apg/external/tbb/src/tbb/task_dispatcher.h:358)
>>>               task_dispatcher.cpp.o:(tbb::detail::d1::task* tbb::detail::r1::task_dispatcher::local_wait_for_all<false, tbb::detail::r1::coroutine_waiter>(tbb::detail::d1::task*, tbb::detail::r1::coroutine_waiter&)) in archive ../../gnu_9.3_cxx17_64_debug/libtbb_debug.a
collect2: error: ld returned 1 exit status

Type of change

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

When TBB_USE_EXCEPTIONS is disabled task_dispatcher fails to compile
@pavelkumbrasev
Copy link
Contributor

Hi @fran6co, could you provide the steps to reproduce it?

@fran6co
Copy link
Author

fran6co commented Nov 16, 2022

Just try to compile with TBB_USE_EXCEPTIONS disabled

@pavelkumbrasev
Copy link
Contributor

I successfully built and linked library with TBB_USE_EXCEPTIONS=0 using such command cmake -DCMAKE_BUILD_TYPE=debug -DCMAKE_CXX_COMPILER=g++ -DCMAKE_CXX_FLAGS='-DTBB_USE_EXCEPTIONS=0' ..
I built it on both release and debug configurations.

@fran6co
Copy link
Author

fran6co commented Nov 16, 2022

That wouldn't work because the CMake define will not be translate to a C++ preprocessor define. You need to pass it like this:

-DCMAKE_CXX_FLAGS=-DTBB_USE_EXCEPTIONS=0

@pavelkumbrasev
Copy link
Contributor

I built it with verbose and define was there also I add something like "dsadasd not compile" under macro and it not compiled with enabled macro and compiled with disabled

@pavelkumbrasev
Copy link
Contributor

There is no difference between -DCMAKE_CXX_FLAGS='-DTBB_USE_EXCEPTIONS=0' and -DCMAKE_CXX_FLAGS=-DTBB_USE_EXCEPTIONS=0.
I still built it successfully. Could you try to do it on environment?

@isaevil
Copy link
Contributor

isaevil commented Nov 16, 2022

@fran6co please share more info about configuration you are facing that issue on (OS, compiler, etc)?

@pavelkumbrasev
Copy link
Contributor

Hi @fran6co, any updates?

@fran6co
Copy link
Author

fran6co commented Nov 18, 2022

Sorry, I didn't have time to check it out. Over the weekend I should be able to

@fran6co
Copy link
Author

fran6co commented Nov 21, 2022

So, for some reason it's not enough to pass it to DCMAKE_CXX_FLAGS. I had to add it to cmake itself for it to fail when building:

diff --git a/src/tbb/CMakeLists.txt b/src/tbb/CMakeLists.txt
index 7f8228c0..8207375f 100644
--- a/src/tbb/CMakeLists.txt
+++ b/src/tbb/CMakeLists.txt
@@ -52,6 +52,7 @@ endif()
 
 target_compile_definitions(tbb
                            PUBLIC
+                           TBB_USE_EXCEPTIONS=0
                            $<$<CONFIG:DEBUG>:TBB_USE_DEBUG>
                            PRIVATE
                            __TBB_BUILD
diff --git a/src/tbbmalloc/CMakeLists.txt b/src/tbbmalloc/CMakeLists.txt
index 5a851851..77698d1a 100644
--- a/src/tbbmalloc/CMakeLists.txt
+++ b/src/tbbmalloc/CMakeLists.txt
@@ -24,6 +24,7 @@ add_library(TBB::tbbmalloc ALIAS tbbmalloc)
 
 target_compile_definitions(tbbmalloc
                            PUBLIC
+                           TBB_USE_EXCEPTIONS=0
                            $<$<CONFIG:DEBUG>:TBB_USE_DEBUG>
                            PRIVATE
                            __TBBMALLOC_BUILD

@fran6co
Copy link
Author

fran6co commented Nov 21, 2022

In my project TBB is part of the build so I just have a add_definitions("-DTBB_USE_EXCEPTIONS=0") and without this fix it fails to build

@fran6co
Copy link
Author

fran6co commented Nov 21, 2022

For the sake of completeness:

OS: Ubuntu 22.04
Compiler: GCC 11.3.0

@isaevil
Copy link
Contributor

isaevil commented Nov 21, 2022

In my project TBB is part of the build so I just have a add_definitions("-DTBB_USE_EXCEPTIONS=0") and without this fix it fails to build

@fran6co do you add oneTBB as subdirectory (add_subdirectory) of your project?

@fran6co
Copy link
Author

fran6co commented Nov 21, 2022

Yes, but even when I build the TBB project standalone I get the same linking error:

/usr/bin/ld: ../gnu_11.3_cxx11_64_debug/libtbb_debug.so.12.9: undefined reference to `tbb::detail::r1::do_throw_noexcept(void (*)())'

@fran6co
Copy link
Author

fran6co commented Nov 21, 2022

I'm actually surprised that it builds for you, because do_throw does reference do_throw_noexcept but it's behind the #ifdef

@isaevil
Copy link
Contributor

isaevil commented Nov 21, 2022

According to specification https://spec.oneapi.io/versions/latest/elements/oneTBB/source/configuration/feature_macros.html:

The TBB_USE_EXCEPTIONS macro controls whether the library headers use exception-handling constructs such as try, catch, and throw. The headers do not use these constructs when TBB_USE_EXCEPTIONS=0.

Caution
The runtime library may still throw an exception when TBB_USE_EXCEPTIONS=0.

Is there some special usecase where you need to compile oneTBB library (not your program using oneTBB) without exceptions?

@fran6co
Copy link
Author

fran6co commented Nov 21, 2022

TBB_USE_EXCEPTIONS makes it harder for us to deal with crash dumps so we need it disabled library wide so we can debug stuff in an easier way

@yilei
Copy link

yilei commented Jul 6, 2023

Should this try/catch block be guarded by TBB_USE_EXCEPTIONS? When exception is disabled, this line with a throw; fails to compile.

@GertyP
Copy link
Contributor

GertyP commented Sep 26, 2023

I also ran in to this problem, which I mention in this issue and was directed to this (thanks, @isaevil).

I wonder if others have an explanation for what seems to be needlessly elaborate/complicated existing code, which I hope could be cleaner and simpler and also fix this particular issue.

Since a throw from a noexcept function will (by definition) result in a std::terminate() then any existing call to do_throw_noexcept(...) ultimately becomes std::terminate().

There are only 2 places where do_throw_noexcept(...) is called -

  • In 'task_dispatcher.h', near the end of task_dispatcher::local_wait_for_all(...)
  • and in 'exception.cpp', within void do_throw(F throw_func) if terminate_on_exception() is enabled

What benefit is there in do_throw_noexcept(...) over std::terminate()? ... The only thing I can see is that it calls a user/custom function before terminating, allowing for some other bit of final work before we exit. However, unless I've overlooked something, all ways that we can reach do_throw_noexcept from do_throw simply use a lambda that does nothing more than throws a particular exception... Which means that the 2 calls to do_throw_noexcept lose nothing if they were simply changed to std::terminate.

I'd also argue that doing so fixes the problem mentioned here with fairly minimal changes, allows us to remove the slightly odd do_throw_noexcept entirely, as well as improves the code readability: Does everyone know what the standard says about a noexcept function that throws? Even if most effortlessly translate this to a terminate, what benefit are we currently getting by going through this potentially jarring function, instead of simply calling std::terminate?

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

Successfully merging this pull request may close these issues.

None yet

5 participants