Fix build failure when using TBB_USE_EXCEPTIONS=0

[GertyP]

Description

This is just an alternative (arguably simplifying) fix in comparison to PR 864 and also discussed in issue 1208. I did comment in PR 864, to describe what I've done here, but since 864 has been open for a long time, I thought it might be useful to show my suggested changes here and see if it gains any traction.

We get a link error from 'task_dispatcher.h's reference to 'do_throw_noexcept(...)', which is not defined when building with 'TBB_USE_EXCEPTIONS=0'. E.g. -

cmake ... -DTBB_COMMON_COMPILE_FLAGS:STRING="/DTBB_USE_EXCEPTIONS=0" ...

Since any throw from a 'noexcept' function results in a 'std::terminate', and all current uses of 'do_throw_noexcept' do nothing more than throw a custom exception, 'do_throw_noexcept' is equivalent to 'std::terminate', and replacing with 'std::terminate' makes things a bit clearer & cleaner.

Fixes # - 1208

• - git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

• 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

Notify the following users

List users with @ to send notifications

Other information

[pavelkumbrasev]

It might make sense to keep do_throw_noexcept because it gives more information to user.
Exception message + terminate vs just terminate.
I would say it make sense to use DO_THROW in task_dispatcher instead of do_throw_noexcept.
Or define another do_throw_noexcept as part of else statement #else statement.

[GertyP]

OK. That is going a bit further than fixing the issue and keeping the same behaviour; this initial change just keeps the exact same behaviour but fixes the TBB_USE_EXCEPTIONS==0 problem.

If you prefer, I don't object to going a step further and writing out a message in the cases that have otherwise always quietly terminated.

See the recent Better exception error reporting commit for my interpretation of your suggestion.

... I've just seen it's upset the tests

TEST CASE:  terminate_on_exception: enabled
  internal exception

D:\a\oneTBB\oneTBB\test\conformance\conformance_global_control.cpp(359): FATAL ERROR: test case CRASHED: SIGABRT - Abort (abnormal termination) signal

This looks like the test is expecting something like a terminate instead of an abort. I'll look in to that when I get time but, ignoring that for now, do the added reporting mechanisms look like what you're after?

A couple of things to note about the additions -

• task_dispatcher.h's 'local_wait_for_all' catch block does (and did) nothing with any caught exception, so I'm reluctant to complicate it further with additional catch clauses, just to get some more info from a e.what()
• We now have the following set of different DO_THROW call uses against 3 different build/run-time configurations to handle -

	terminate_on_exception==true	terminate_on_exception==false	TBB_USE_EXCEPTIONS==0
DO_THROW(except_type,"string literal")	do_terminate_on_exc("except_type","string literal")	throw except_type("string literal")	do_exceptions_disabled_error("except_type","string literal")
DO_THROW(except_type,runtime_buf)	do_terminate_on_exc("except_type",runtime_buf)	throw except_type(runtime_buf)	do_exceptions_disabled_error("except_type",runtime_buf)
DO_THROW(except_type2)	do_terminate_on_exc("except_type2")	throw except_type2()	do_exceptions_disabled_error("except_type2")

The table shows the 3 different DO_THROW use-case styles we have down the left column and the cells along each row show what they expand/evaluate to under the 3 different build and run-time exception configurations we can have.

[GertyP]

... Yes, the tests are understandably expecting terminate, not abort, when using the terminate_on_exception control 😄 . So, for consistency, all exception-related fatal error reporting (whether though use of terminate_on_exception or TBB_USE_EXCEPTIONS=0) and now always ends in a std::terminate().

... Whether that's preferable from a user's point of view is a separate question that I don't think should be addressed here; changing that in this PR would go even further beyond the intended scope of just fixing a broken build configuration while keeping the originally intended behaviour (i.e. it's always essentially been a std::terminate in all cases).

[pavelkumbrasev]

@GertyP Thank you for the update. It turned out this place is tricky. (It will take some time for me to figure out how we can proceed with this)

[GertyP]

Thanks for having a look at this @pavelkumbrasev
As mentioned above, the first commit ('Fix build failure when using TBB_USE_EXCEPTIONS=0') is the lowest impact change that strictly fixes the issue in question, while preserving exactly the same existing behaviour ('i.e. quietly terminating when hitting exceptions when the run-time terminate_on_exception is enabled, and now the same for when TBB_USE_EXCEPTIONS is disabled).

It only got a little more tricky to consider the matrix of uses and build/run combinations at your request to try to get more useful error info out to the user 😄 , which I think is perfectly reasonable and which hopefully I've managed to do in the latest commit... But if you're unwilling to take both together (and we can't agree on what else might need further changes), then perhaps consider just taking just the first change.

[pavelkumbrasev]

Thanks for having a look at this @pavelkumbrasev As mentioned above, the first commit ('Fix build failure when using TBB_USE_EXCEPTIONS=0') is the lowest impact change that strictly fixes the issue in question, while preserving exactly the same existing behaviour ('i.e. quietly terminating when hitting exceptions when the run-time terminate_on_exception is enabled, and now the same for when TBB_USE_EXCEPTIONS is disabled).

It only got a little more tricky to consider the matrix of uses and build/run combinations at your request to try to get more useful error info out to the user 😄 , which I think is perfectly reasonable and which hopefully I've managed to do in the latest commit... But if you're unwilling to take both together (and we can't agree on what else might need further changes), then perhaps consider just taking just the first change.

I have tried to clarify whether oneTBB (library part) should be build with TBB_USE_EXCEPTIONS disabled. Seems there was a decision that we don't support such build configuration because it would increase test scope.
So there is a question: Do you need such configuration and why?

[GertyP]

there was a decision that we don't support such build configuration because it would increase test scope

This is a documented feature macro here.

I can sympathise that it would suggest the need for additional build tests if you really wanted to be rigorous ... but a phrase also comes to mind: "Don't let perfection be the enemy of good". In other words, I fear that there's a possibility that you find testing a full and complete set of tests with configuration 'TBB_USE_EXCEPTIONS=0' far too much of a burden and so decide it's easer to drop it than to implement an exhaustive set of extra tests. But there is another practical compromise alternative, which is to add minimal testing that hits this link error at least. E.g. -

cmake ... -DCMAKE_BUILD_TYPE=Debug -DTBB_CPF=OFF -DTBB_BUILD=on -DTBB_TEST=off -DTBBMALLOC_BUILD=on -DTBB4PY_BUILD=off -DTBB_COMMON_COMPILE_FLAGS:STRING="/DTBB_USE_EXCEPTIONS=0" ...
cmake --build ... --config Debug

which takes just a few seconds before revealing this issue as a link error.

But even if there aren't any auto-tests to cover the build and/or run-time testing of this configuration, it's still preferable, from my point of view, if you're still willing to take fixes for cases which you've decided to no longer officially support but which can technically still be used. I think the vast majority of TBB users would not blame you for not covering absolutely 100% of all build and run-time config permutations, so it would be a shame to not take small fixes when they are discovered. It would also be a shame to remove a fairly simple, quite well isolated feature like this (it's all quite nicely contained within exception.cpp) for fear of not having comprehensive enough auto-testing. I don't mind that I've found an untested, documented feature issue... so long as you'll take fixes 😃

Do you need such configuration and why?

Need may be too strong a word but, in addition to another answer to this question, I think that, in some circumstances, use of exceptions can facilitate worse code to read and understand as well as well as facilitating worse system design decisions. It's a big topic and a bit philosophical but I think throwing and catching exceptions too easily promotes "exceptional circumstances as normal cases" to be considered and handled throughout more code than usually necessary, encouraging programmers think at a very zoomed in level about how to try to gracefully handle and 'soldier on' with things like bad inputs, bad results, out-of-memory, misuse of an interface, etc. instead of being confronted by the clean simplicity of a hard break/abort/crash, where you're forced to better understand the systems you use, the assumptions you have, and as a result, usually come up with a better designed system (e.g. more comprehensive and careful up-front partitioning and budgeting of memory given to various systems instead of lower-level run-time code that's polluted with bloated retry flailing upon failure to allocate memory).

[GertyP]

I'd be interested to know if there's any update on this. At least a follow-up decision (with some rationale, depending on the decision) would be more satisfactory than an open, ignored PR.

As I mentioned above, the first commit really is a minimal change that fixes this build config while keeping the same behaviour in the usual TBB_USE_EXCEPTIONS==1 case, so I think I'd struggle to see any reason not to take at least that.

However, your suggestion to improve the error reporting has resulted in the follow-up commit, which I think not only improves the TBB_USE_EXCEPTIONS==0 scenario (that you're reluctant to maintain / keen to drop), but it also improves error messages over the previous/existing code for cases that you are using/maintaining: I.e. anything making use of global_control::terminate_on_exception; anything going though throw_exception(exception_id), DO_THROW, and the task_dispatcher's catch around the infinite loop, would all just quietly terminate when using global_control::terminate_on_exception but will now terminate with a much more informative message... So, if you're uncomfortable taking this change on the grounds of it fixing something you have that you're not keen to maintain, then please consider taking it on the grounds of it improving something I presume you are wanting to maintain 😃

[pavelkumbrasev]

Hi @GertyP, there is still no decision regarding this problem. I'm concerned with such a solution because it's just hides the problem.
Library doesn't support build without exception so TBB_USE_EXCEPTIONS=0 during library build doesn't make sense. (TBB still will throw exception from other places)
So I don't know what is a real value of such fix. @vossmjp what do you think about it?

[GertyP]

I'm concerned with such a solution because it's just hides the problem.

I'm unclear on what you mean by this.

"The problem" in this case would seem to be just for me, but maybe others too, who try to use something that's documented but which doesn't build. This "solution" fixes the broken build when using this feature. What problem is being hidden by this solution that doesn't already exist?

I wonder if you mean that this solution does not add new tests to cover this use case, leaving the possibility that some future change again breaks the ability to build with TBB_USE_EXCEPTIONS=0. But in that case, that speculative future possibility that you may be worried about is a reality right now. And if you are worried about it in the future, why not care about getting a fix for the existing problem now?... Of if you're really not worried about the future problem possibility, then what's the problem?

I don't see how you're losing anything by taking this. In fact, as I said above, the follow-up commit not only fixes something (even if it's something you don't care about), it also improves the existing functionality in use cases you do care about (actual error messages instead of quiet terminates by throwing from noexcept funcs) as well, I'd argue, as improving the code readability.

TBB still will throw exception from other places

That still leaves a lot of places/scenarios where nothing useful is shown to the user upon an error: It currently doesn't do anything useful and will quietly terminate, without any useful info, if catching an error from the task_dispatcher::local_wait_for_all loop when the documented feature global_control::terminate_on_exception is used. The same goes for any existing calls to throw_exception, DO_THROW, or do_throw when this global control is used... or have I misunderstood or overlooked something?

Because it seems like there are only things to gain/improve from these changes and nothing to lose (no new regressions, no new loss of test coverage), it feels like I must be missing something or I haven't explained something so perhaps it'll help both/either of us better understand if you could try to answer these, please? -

• What new issue do you think is being introduced by this change?
• Do you prefer the behaviour for users of global_control::terminate_on_exception when dealing with errors to be quiet termination (as it is before this PR... or do you think the behaviour's different?) vs outputting a useful error message followed by termination?
• Do you think the behaviour of a throw from a noexcept function is universally understood or is do_throw_noexcept more understandable than a do_terminate_on_exception error reporting function?

I promise I'm not trying to be objectionable or argumentative here; I'm genuinely confused and of the opinion that there's a big misunderstanding that I'm now intrigued to better understand 😕