[squid-dev] [PATCH] Deletors for std::unique_ptr WAS: Re: Broken trunk after r14735

[Amos Jeffries]

On 30/07/2016 6:29 a.m., Alex Rousskov wrote:
> On 07/29/2016 09:27 AM, Amos Jeffries wrote:
>>>> typedef std::unique_ptr<BIO, std::function<decltype(BIO_free)>> BIO_Pointer;
> 
>> I got this config parsing crash replicated here and tried a dozen or so
>> combinations. It does seem to keep coming back to my earlier approach of
>> using per-type Functors as the most reliable solution.
> 
> AFAICT, this is not a question of reliability, and we should not be
> fixing this by trying various combination of typedef characters until we
> find one that does not immediately crash Squid. We should figure out
> _why_ trunk code does not work (and then fix it accordingly).

Please clear your head of the notion that I'm tying things bindly or
randomly. As mentioned in IRC the other day I do know exactly what is
going wrong. Your outline in the paragraphs below is correct.

The difficulty as I mentioned already has been in finding a working
solution that is easily used without risking future bugs from people
misunderstanding the use of these Pointers. That is what I meant by
"reliable". There are several different implementations that work and
are correct, but with varying amounts of complicated-looking/confusing
syntax needed.

I'm using the scientific method here. Reading the C++ documentation to
form hypothesis about the behaviour that will happen with the various
ways of using unique_ptr. Then performing an explicit and planned
experiment to (dis)prove it - including ways I know wont work just to
prove that knowledge correct.

By relibility above I was referring to ease of use with least future
bugs resulting from others not knowing the tricky details.

You have previously assured me[1] that using std::function<declype(X)>
was the way to go and would allow removal of the macros. That usage has
turned out to cause the null derefernce. For the reasons you outline
below. Those problem results should have been obvious, but hindsight is
clearer than foresight.

> 
> The trunk code cannot work because we are telling std::unique_ptr that
> our Deleter is std::function<decltype(BIO_free)>. What is
> std::function<decltype(BIO_free)>? It is an std::function class. An
> instance of that class can call any function that has the same
> declaration as BIO_free. So, which function should our BIO_Pointer call?
> We did not specify that! Thus, we get a bad_function_call exception when
> unique_ptr object is trying to destroy its target pointer [using a
> missing/unspecified deleter function].
> 
> In other words, trunk code is equivalent to this pseudo code:
> 
>   BIO *bio = ...;
>   std::function<int(BIO*)> fun = NULL;
>   fun(bio); // throws -- cannot call an unspecified function
> 

Exactly. You understand the problem with the solution you previously
convinced me to implement.

> What we should be writing is this:
> 
>   BIO *bio = ...;
>   std::function<int(BIO*)> fun = BIO_free;
>   fun(bio); // frees bio
> 
> Since std::function supports default construction, the "= NULL" part is
> implicit -- we do not need to pass NULL to std::unique for the default
> std::function constructor to construct an empty/non-callable functor.
> 

Exactly. Though actual implementation needs to be different. Repeating
that code setup every time we want to make a local variable or class
member would be terribly annoying.

> 
>> We could also wrap the std::unique_ptr definition parts inside the macro
>> for much smaller visible code. But that would prevent re-use of the
>> macro for other plain Functor definitions (not that anything else is
>> using it).
> 
> If possible, we should avoid macros and should learn how to use C++11
> correctly. In general, it makes little sense to suffer the pains of
> switching to C++11 and then use macros for things C++11 is supposed to
> provide or support natively.
> 

We have been in agreement on that for a long time. Thats why you so
easily convinced me to go with std::function in the first place.

Using C++11 "correctly" for this resulting bug involves choosing between
several varyingly complicated ways to typedef the Pointer, or to declare
instances of it.


> However, in this specific case, some macros may be unavoidable or even a
> good idea because we are also dealing with C libraries that we cannot
> convert to C++11. Please give me a few hours to think whether there is a
> macro-free solution available.

Sure. The documentation indicates that we could use a template Functor
which expands instead of a Macro expansion. But gives no examples and my
memory of expanding template parameters dependent on other template
parameters was failing me yesterday.

I see from your followup posts that you found the two ways to go there.
I'm running my own tests on the resulting patch using the simple
template instead of macro and will apply that if it passes for me as well.


Amos