Fixed optimized away hooks. Fixes #316

[Kojoley]

MSVC learned to not emit unreferenced symbols with internal linkage and the hooks were defined in unnamed namespace which forces internal linkage, even if you mark a variable extern.

Since Boost does not have a stable ABI, does not mangle the namespace with the version, and the hooks are in boost namespace (boost::on_*) -- there is no point in trying to hide some symbols because mixing different versions of boost static libraries will not work already.

I also renamed the __xl_ca variable for consistency and because using double underscored identifiers is forbidden. ([lex.name]/3)

The extern const is for verbosity and because they are indeed const (it is done via pragma already).

[Kojoley]

I did not add a test because tss_test is already catching the issue. I also checked that this fixes Clang build which had the issue a long before.

[Lastique]

@pdimov Since @viboes is not responding, maybe you could merge this fix?

[pdimov]

OK, done. I'm getting crashes in some future tests on clang-win, but that's a preexisting problem. Maybe @Kojoley knows why.

[Lastique]

Thanks, Peter. Please, remember to merge to master as well.

[pdimov]

I will.

Looking into the clang-win issue, one potential problem might be that at https://github.com/boostorg/thread/blob/96cd717b331e62095aa58d6768dab3baa03fcdce/src/win32/tss_pe.cpp#L219-L227 Clang issues a warning that dw is unused, so it's probably optimized out and the reference to _tls_used is eliminated.

[Kojoley]

Maybe @Kojoley knows why.

I have no idea. It crashes with access violation on trying to rethrow/read the exception from the future, reproducible only on 32bit build with optimizations.

Clang issues a warning that dw is unused, so it's probably optimized out and the reference to _tls_used is eliminated.

When I was debugging the #316 issue I saw the symbol, it is not optimized away, and since it is marked as C extern I am not sure the comment makes any sense.

[Lastique]

If it is indeed optimized out, I would call it a compiler bug. The warning does not indicate that it is removed, though.

[pdimov]

This looks like a clang codegen bug. In

    boost::promise<int> p;
    p.set_exception( boost::copy_exception( X() ) );

boost::copy_exception returns a valid exception_ptr, and set_exception receives an invalid one. Oh well.

[Kojoley]

This looks like a clang codegen bug

I was guessing it to be also, simply because Clang has known issues with exceptions on MS ABI (https://groups.google.com/forum/#!topic/llvm-dev/ZcNUP_1550M).

[Kojoley]

Weird thing, this crashes:

#include <boost/thread/future.hpp>
#include <thread>

struct wrap
{
};

template <typename T>
boost::exception_ptr make_exception_ptr(T) {
  return boost::copy_exception(wrap{});
}

void func2(boost::promise<int> p)
{
    p.set_exception(make_exception_ptr(3));
}

int main()
{
    boost::promise<int> p;
    auto f = p.get_future();
    std::thread(func2, std::move(p)).detach();
    try
    {
        f.get();
    }
    catch (wrap const&)
    {
        return 0;
    }
    return 1;
}

And this does not:

#include <boost/thread/future.hpp>
#include <thread>

struct wrap
{
};

boost::exception_ptr make_exception_ptr(int) {
  return boost::copy_exception(wrap{});
}

void func2(boost::promise<int> p)
{
    p.set_exception(make_exception_ptr(3));
}

int main()
{
    boost::promise<int> p;
    auto f = p.get_future();
    std::thread(func2, std::move(p)).detach();
    try
    {
        f.get();
    }
    catch (wrap const&)
    {
        return 0;
    }
    return 1;
}

-template <typename T>
-boost::exception_ptr make_exception_ptr(T) {
+boost::exception_ptr make_exception_ptr(int) {
   return boost::copy_exception(wrap{});
 }

[pdimov]

Probably affects what's getting inlined where. This is what crashes for me:

#include <boost/thread/future.hpp>

struct X
{
};

int main()
{
    boost::promise<int> p;
    boost::unique_future<int> f = p.get_future();

    p.set_exception( boost::copy_exception( X() ) );

    try
    {
        f.get();
    }
    catch( X const& )
    {
    }
}

No need to use a thread at all. The p.set_exception line is miscompiled. I suppose one doesn't even need future here.

[pdimov]

Yeah, this crashes too:

#include <boost/exception_ptr.hpp>

struct X
{
};

void set_exception( boost::exception_ptr p )
{
    try
    {
        boost::rethrow_exception( p );
    }
    catch( X const& x )
    {
    }
}

int main()
{
    set_exception( boost::copy_exception( X{} ) );
}

[Lastique]

If it's caused by inlining, does BOOST_NOINLINE applied to set_exception help?

[pdimov]

In that last example, changing set_exception to take by const& eliminates the crash.

But I'm not sure how much we need to care about this issue, as it only affects clang-cl 32 bit and nobody has reported it as a problem.

[Lastique]

I'd say, we shouldn't wait until it hits someone, especially if we have a workaround.

[pdimov]

The right thing to do is to file a bug against Clang, then maybe work around it.

[Kojoley]

https://bugs.llvm.org/show_bug.cgi?id=46584