Capturing temporary object by reference in completion_future

[mcopik]

Method then() in completion_future, responsible for launching a callback functor when completion_future is ready, takes a const reference to functor and then passes it by reference to newly created thread: https://github.com/RadeonOpenCompute/hcc/blob/master/include/hc.hpp#L953

Hence a reference to a temporary object may be saved by the thread. The result is quite obvious to foresee - a mysterious memory corruption when the callback is launched. It took us three days to figure it out why this piece of code is failing:

some_data * ptr = ....;
auto fut = device.create_marker();
fut.then([ptr]() { /* do something with ptr, which is already garbage */ });

C++AMP API declares this function as taking a const reference, not universal reference (which would enforce moving an rvalue), but the whole issue can be solved just by capturing the functor by value in the lambda passed to std::thread.

[david-salinas]

@AlexVlx @scchan I believe we can close this issue, as it is resolved with PRs #353 / #354 ?

[AlexVlx]

I'm not sure I see how any of those PRs does anything for this issue, which is one of interface design...

[mcopik]

@david-salinas @AlexVlx I don't think it's a problem with interface design since this API was almost entirely copied from C++AMP. It might not be perfect but there was an obvious bug in the implementation - catching a possibly temporary object by reference for later execution. Is it fixed?

[mcopik]

I don't know which branch is the main one but clang_tot_upgrade seems to be the most active one. The bug is not fixed there: https://github.com/RadeonOpenCompute/hcc/blob/clang_tot_upgrade/include/hc.hpp#L1284

[AlexVlx]

Whilst I worked on C++AMP (the original) and I have great appreciation for it, I don't think it got everything right, so saying that something was copy&pasted from there, whilst flattering does not axiomatically guarantee correctness. IMHO, the functor should've been passed by value and treated as a sink. Anyhow, nothing changed in the ::then implementation, and this issue is not addressed, hence my confusion in the original question I asked.