search

boostorg / thread

Boost.org thread module
http://boost.org/libs/thread
199 stars 161 forks source link

Simplify TSS cleanup routines. Fixes #236 #249

Closed Kojoley closed 5 years ago

Kojoley commented 5 years ago

Instead of wrapping a default or user provided destructor into a virtual class and placing it into a shared_ptr it is now stored directly with an elided type, to not introduce UB it is not called directly but through a helper function which casts it back to the original type before calling.

I tried to touch as little lines as possible to make the review simpler.

viboes commented 5 years ago

Thanks for working on this. I believed that the issue was the the sanitizer itself. Please, could you describe where the UB reported in #236 was so that I understand better the PR?

@pdimov Please, could you check this and tell us what do you think?

Of course, I couldn't accept this change for boost.1.69, as it is too late.

Kojoley commented 5 years ago

I believed that the issue was the the sanitizer itself.

There is no consensus. GGC and Clang devs says that it is not a bug in sanitizer.

Please, could you describe where the UB reported in #236 was so that I understand better the PR?

The PR can be reviewed without thinking about #236, it is just a nice side effect.

You can check my bug report to LLVM https://bugs.llvm.org/show_bug.cgi?id=39191 for the simple example.

The problem is that virtual methods (operator() and destructor in the case) are called on objects (delete_data/run_custom_cleanup_function) that passes DSO boundaries as base (tss_cleanup_function), but their symbols are hidden. Making these symbols visible will solve the problem too, but as there is no consensus on the topic and because it is possible not to use virtual classes for solving the TSS needs.

pdimov commented 5 years ago

The original code is a bit odd, I suspect it dates from premodern times when shared_ptr didn't have deleters and boost::function didn't exist. Nowadays you could just use shared_ptr<void>( value, func ), or boost::function.

The patch works too, I suppose. It's a matter of taste which one to pick.

viboes commented 5 years ago

@Kojoley Could we summarize that there is no real bug and that the patch is just silencing the sanitizer? Are we on UB land? Could you clarify what is this fixing aside #236?

@pdimov yes the code dates a lot.

Kojoley commented 5 years ago

The original code is a bit odd, I suspect it dates from premodern times when shared_ptr didn't have deleters and boost::function didn't exist. Nowadays you could just use shared_ptr( value, func ), or boost::function.

The code is now that old, it is from 2007, and it was using boost::function 332dd988e461fda0b5d7c038aa3110c61979b8d6.

@Kojoley Could we summarize that there is no real bug and that the patch is just silencing the sanitizer?

You can treat it like that.

Are we on UB land?

This case is not documented to be UB, even throwing and catching hidden symbols is not explicitly banned (see boostorg/function#24).

Again:

There is no consensus. GGC and Clang devs says that it is not a bug in sanitizer.

Could you clarify what is this fixing aside #236?

Nothing.

viboes commented 5 years ago

If this is not fixing nothing clear yet, why do you want I apply this PR then?

Kojoley commented 5 years ago

OMG, you are killig me. The PR:

  1. Removes (potential) UB.
  2. Simplifies the code by reducing number of dependencies.
  3. Makes TSS faster by removing shared_ptr.
viboes commented 5 years ago

I've decided to give it a chance to this PR. We have now the time to fix it if there will be some regressions.