Am I using the pointer class properly in this generic unique_ptr<>() deleter?

Question

I created a generic deleter template that can be used to create unique_ptr<>() sub-types allowing for a Deleter other than just delete ptr.

It works great with the default optimization flags (i.e. -O0), however, when I use -O3 the T & operator * () function, somehow, returns 0 instead of the f_pointer contents.

I would like to make sure that we agree that there is something wrong in the compiler and that my template is correct. The following is a complete piece of code that should compile as is under Ubuntu 16.04 and Ubuntu 18.04 and probably other versions as long as they support C++14 (see below for tested g++ versions).

// RAII Generic Deleter -- allow for any type of RAII deleter
//
// To break compile with:
//     g++ --std=c++14 -O3 -DNDEBUG ~/tmp/b.cpp -o b

#include <memory>
#include <iostream>
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

template<class T, T null_value, class D, D deleter>
class raii_generic_deleter
{
public:
    class pointer
    {
    private:
        T f_pointer = null_value;

    public:
        pointer(T p)
            : f_pointer(p)
        {
        }

        pointer(std::nullptr_t = nullptr)
            : f_pointer(null_value)
        {
        }

        explicit operator bool () const
        {
            return f_pointer != null_value;
        }

        bool operator == (pointer const rhs) const
        {
            return f_pointer == rhs.f_pointer;
        }

        bool operator != (pointer const rhs) const
        {
            return f_pointer != rhs.f_pointer;
        }

        T & operator * ()
        {
            return f_pointer;
        }
    };

    void operator () (pointer p)
    {
        deleter(*p);
    }
};


typedef std::unique_ptr<int,
            raii_generic_deleter<int, -1, decltype(&::close), &::close>>
                        raii_fd_t;


int main(int argc, char * argv [])
{
    int fd = -1;

    {
        raii_fd_t safe_fd;

        std::cout << "default initialization: safe_fd = " << *safe_fd
                  << std::endl;

        fd = open("/tmp/abc.tmp", O_RDWR | O_CREAT, 0700);

        std::cout << "fd = " << fd << std::endl;

        safe_fd.reset(fd);

        std::cout << "safe_fd after the reset(" << fd
                  << ") = " << *safe_fd << std::endl;
    }

    if(fd != -1)
    {
        // assuming the safe_fd worked as expected, this call returns an error
        //
        int r = close(fd);
        int e(errno);

        std::cout << "second close returned " << r
                  << " (errno = " << e << ")" << std::endl;
    }

    return 0;
}

(For original, see raii_generic_deleter.h in libsnapwebsites)

There is the output I'm getting when I use -O0 (no optimizations):

default initialization: safe_fd = -1
fd = 3
safe_fd after the reset(3) = 3
second close returned -1 (errno = 9)

In this case the *safe_fd call returns -1 and 3 as expected. This calls the template T & pointer::operator * () function.

With any level of optimization (-O1, -O2, -O3) the output looks like this:

default initialization: safe_fd = 0
fd = 3
safe_fd after the reset(3) = 0
second close returned -1 (errno = 9)

As we can see, the safe file descriptor returns 0 instead of -1 after initialization and then again 0 when it should then be 3. However, the destructor properly closes the file since the second close fails as expected. In other words, somehow, the file description (3) is known and properly used by the deleter.

When I update the pointer operator in this way:

        T & operator * ()
        {
            std::cout << "f_pointer within operator * = " << f_pointer
                      << std::endl;
            return f_pointer;
        }

Then the output with any level of optimization is correct:

f_pointer within operator * = -1
default initialization: safe_fd = -1
fd = 3
f_pointer within operator * = 3
safe_fd after the reset(3) = 3
f_pointer within operator * = 3
second close returned -1 (errno = 9)

Which is probably because that specific function doesn't get optimized out completely.

Compilers:

I tested with stock g++ on Ubuntu 16.04

g++ (Ubuntu 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609

And also on Ubuntu 18.04

g++ (Ubuntu 7.3.0-16ubuntu3) 7.3.0

I also went ahead and reported this as a bug on the GNU website.

Answer 1

The issue seems to be due to libstdc++ implementation of unique_ptr::operator*. Here it is in a very simplified, pared-down way:

struct pointer
{
    pointer(int val = -42) : z(val) { }
    int z = -42;
    int& operator*() { return z; }
};

struct my_unique_ptr
{
    pointer rep;
    pointer get() { return rep; }
#ifdef PROBLEM
    int& operator*() { return *get(); } // libstdc++ implementation
#else
    int& operator*() { return *rep; } // libc++ implementation
#endif
};

int main()
{
    my_unique_ptr q;
    std::cout << *q << "\n";
}

Now it is abundantly clear that libstdc++ cannot possibly work with your implementation of pointer, because it returns a reference to a local temporary object from operator*. Any pointer that stores its own pointee will have the same issue.

Standard-wise, this doesn't seem to be a bug in libstdc++. The standard specifies that unique_ptr::operator*() returns *get(), which libstdc++ faithfully does.

If anything, this is a defect in the standard.

An immediate fix is to stop defining operator* in your pointer class. unique_ptr doesn't need it (NullablePointer is not required to provide it).

Since pointer is in fact nothing more than a wrapper around T that provides value-initialisation to a given constant, it would make more sense to define an operator T() for it, and use get() to "dereference" the corresponding unique_ptr.

Answer 2

I'm adding my own answer to show the changes in the code for others interested by such an RAII deleter (very practical as you don't need to specify the deleter each time you have an instantiation!).

As mentioned by @n.m. in the accepted answer, int is not a pointer and thus as such it can't be accessed as a pointer. So the T & operator * () is not logical for such a type (as operator [] () and operator -> () would also not make sense).

The problem with the code is because of the get() being used by the operator * () in the unique_ptr<>() implementation. It looks like this:

/// Return the stored pointer.
pointer
get() const noexcept
{ return std::get<0>(_M_t); }

As we can see, the get() returns a pointer copy. Such an object is temporary! This means you can't return a reference from it and hope for it to remain valid. The operator * () does such, though:

/// Dereference the stored pointer.
typename add_lvalue_reference<element_type>::type
operator*() const
{
  _GLIBCXX_DEBUG_ASSERT(get() != pointer());
  return *get();
}

As we can see, this function returns the content of the reference returned by get() but then says it wants a reference. This is why my operator * () implementation had to return a non-const reference.

By removing that constrain and using operator T () const instead of operator * () to retrieve the value of the unique_ptr<>(), I get the correct value.

// RAII Generic Deleter -- allow for any type of RAII deleter
//
// To break compile with:
//     g++ --std=c++14 -O3 -DNDEBUG ~/tmp/b.cpp -o b

#include <memory>
#include <iostream>
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

template<class T, T null_value, class D, D deleter>
class raii_generic_deleter
{
public:
    class pointer
    {
    private:
        T f_pointer = null_value;

    public:
        pointer(T p)
            : f_pointer(p)
        {
        }

        pointer(std::nullptr_t = nullptr)
            : f_pointer(null_value)
        {
        }

        explicit operator bool () const
        {
            return f_pointer != null_value;
        }

        bool operator == (pointer const rhs) const
        {
            return f_pointer == rhs.f_pointer;
        }

        bool operator != (pointer const rhs) const
        {
            return f_pointer != rhs.f_pointer;
        }

        operator T () const
        {
            return f_pointer;
        }
    };

    void operator () (pointer p)
    {
        deleter(static_cast<T>(p));
    }
};


typedef std::unique_ptr<int,
            raii_generic_deleter<int, -1, decltype(&::close), &::close>>
                        raii_fd_t;


int main(int argc, char * argv [])
{
    int fd = -1;

    {
        raii_fd_t safe_fd;

        std::cout << "default initialization: safe_fd = " << safe_fd.get()
                  << std::endl;

        fd = open("/tmp/abc.tmp", O_RDWR | O_CREAT, 0700);

        std::cout << "fd = " << fd << std::endl;

        safe_fd.reset(fd);

        std::cout << "safe_fd after the reset(" << fd
                  << ") = " << safe_fd.get() << std::endl;
    }

    if(fd != -1)
    {
        // assuming the safe_fd worked as expected, this call returns an error
        //
        int r = close(fd);
        int e(errno);

        std::cout << "second close returned " << r
                  << " (errno = " << e << ")" << std::endl;
    }

    return 0;
}

So two main changes (1) T & operator * () has become operator T () const and (2) in main(), change *safe_fd in safe_fd.get().

Note: I also cast f_pointer to T for the deleter function in case the deleter function is not a one to one match and automatic casting would fail. (i.e. in my case here close(int) has an exact T as input so it would work just fine. At times the deleter may not be that perfect.)