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

Require TBB without captured exceptions #8169

Merged
merged 2 commits into from
May 18, 2021

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented May 12, 2021

Related to ROOT-10636

@hahnjo hahnjo requested review from oshadura and eguiraud May 12, 2021 13:11
@hahnjo hahnjo self-assigned this May 12, 2021
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

hahnjo added 2 commits May 12, 2021 15:58
If TBB_USE_CAPTURED_EXCEPTION is set, tbb_config.h checks if the
support for exact exception propagation is available.

Related to ROOT-10636
@hahnjo hahnjo force-pushed the tbb-captured-exceptions branch from a5c98c7 to c849f8e Compare May 12, 2021 13:59
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

Copy link
Contributor

@oshadura oshadura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hahnjo thanks a lot, I wanted to ask if TBB_USE_CAPTURED_EXCEPTION is limited to particular version of TBB? I don't know where we cant test it, there is probably no suitable node is available in CI, so I trust you that you manage to fix it in your setup :) Please feel free to reassign https://sft.its.cern.ch/jira/browse/ROOT-10636 to yourself!

@hahnjo
Copy link
Member Author

hahnjo commented May 18, 2021

@hahnjo thanks a lot, I wanted to ask if TBB_USE_CAPTURED_EXCEPTION is limited to particular version of TBB?

My understanding is that TBB_USE_CAPTURED_EXCEPTION is the fallback if TBB cannot determine that it can properly forward exceptions from the workers. In my setup, this happens with the packaged version TBB 2018 on CentOS 8 when compiling with Clang (because that advertises compatibility with GCC 4.2.1 and only 2019.U4 learned to ignore this).

@hahnjo hahnjo merged commit 86181e8 into root-project:master May 18, 2021
@hahnjo hahnjo deleted the tbb-captured-exceptions branch May 18, 2021 08:54
if(TBB_FOUND)
set(CMAKE_REQUIRED_INCLUDES "${TBB_INCLUDE_DIRS}")
check_cxx_source_compiles("
#include <tbb/tbb_config.h>
Copy link

@makortel makortel May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oneTBB 2021.2 does not have this header, so this check fails falsely claiming TBB being built with "tbb::captured_exception" that doesn't even exist anymore in that version.

(found out in cms-sw/cmsdist#6936)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing and reporting. Indeed, oneTBB doesn't have the header anymore nor the "captured exceptions" thing, so #8239 skips the test under those conditions.

@oshadura our FindTBB.cmake cannot detect versions of newer TBBs because, guess what, they don't have the header tbb/tbb_stddef.h either. I was first checking TBB_VERSION, but that's empty in such case. Probably somebody with proper CMake skills should take a look and make the detection work properly...

Copy link
Contributor

@oshadura oshadura May 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hahnjo thanks a lot for fix! could you please open a GH issue/improvement to revisit FindTBB.cmake? I will recheck it ASAP

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.

4 participants