Bug 503 - aligned_allocator doesn't compile with C++11 stdlib
aligned_allocator doesn't compile with C++11 stdlib
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - general
unspecified
All All
: Normal Unknown
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-28 13:15 UTC by Marton Danoczy
Modified: 2014-03-19 13:30 UTC (History)
4 users (show)



Attachments

Description Marton Danoczy 2012-08-28 13:15:10 UTC

    
Comment 1 Marton Danoczy 2012-08-28 13:20:11 UTC
This test case shows the problem:

#include <Eigen/Core>
#include <set>

struct vertex : Eigen::Vector2d 
{
    vertex(double x, double y, int16_t i = -1) :
        Eigen::Vector2d(x,y), index(i) {}

    // strict weak ordering for std::set
    bool operator<(const vertex& v) const
    { return x() != v.x() ? x() < v.x() : y() < v.y(); }
    
    int16_t index;
};

int main()
{
    std::set<
        vertex,
        std::less<vertex>,
        Eigen::aligned_allocator<vertex>
    >   vertices;
    
    vertex v(1.0, 2.0);
    vertices.insert(v);
    return 0;
}


It doesn't compile with:
clang -I... -c bug.cpp -std=c++11 -stdlib=libc++

The fix is commenting out both overloads of construct() in aligned_allocator, as indicated in the accepted answer here: http://stackoverflow.com/questions/11648202/questions-about-hinnants-stack-allocator

Then, however compilation breaks with pre-11 CC++. So I guess something is wrong with construct(), but no idea what.
Comment 2 Gael Guennebaud 2012-08-28 13:44:11 UTC
Which Eigen version? Is't it a duplicate of bug 455
Comment 3 Marton Danoczy 2012-08-28 14:46:40 UTC
I don't think it's a duplicate. It breaks in 3.1 and in the default branch, 3.0 works though.
Comment 4 Gael Guennebaud 2012-08-30 11:17:49 UTC
The problem is that clang support for c++11 is really incomplete. Alos, commenting out both overloads does not work for me and clang:

In file included from /usr/include/c++/4.2.1/set:64:
/usr/include/c++/4.2.1/bits/stl_tree.h:379:22: error: no member named 'construct' in 'Eigen::aligned_allocator<vertex>'
          { get_allocator().construct(&__tmp->_M_value_field, __x); }
            ~~~~~~~~~~~~~~~ ^
/usr/include/c++/4.2.1/bits/stl_tree.h:839:24: note: in instantiation of member function 'std::_Rb_tree<vertex, vertex, std::_Identity<vertex>, std::less<vertex>, Eigen::aligned_allocator<vertex> >::_M_create_node' requested here
      _Link_type __z = _M_create_node(__v);
                       ^
/usr/include/c++/4.2.1/bits/stl_tree.h:987:31: note: in instantiation of member function 'std::_Rb_tree<vertex, vertex, std::_Identity<vertex>, std::less<vertex>, Eigen::aligned_allocator<vertex> >::_M_insert' requested here
          return pair<iterator,bool>(_M_insert(__x, __y, __v), true);
                                     ^
/usr/include/c++/4.2.1/bits/stl_set.h:307:9: note: in instantiation of member function 'std::_Rb_tree<vertex, vertex, std::_Identity<vertex>, std::less<vertex>, Eigen::aligned_allocator<vertex> >::_M_insert_unique' requested here
          _M_t._M_insert_unique(__x);
               ^
bug_503.cpp:25:14: note: in instantiation of member function 'std::set<vertex, std::less<vertex>, Eigen::aligned_allocator<vertex> >::insert' requested here
    vertices.insert(v);
             ^
1 error generated.

(when using clang 3.1)
Comment 5 Gael Guennebaud 2012-08-30 11:20:12 UTC
hm, ok it works if you also specify  -stdlib=libc++
Comment 6 Gael Guennebaud 2012-08-30 11:25:28 UTC
hm, now I'm puzzled. If I compile with both -std=c++11 and -stdlib=libc++, then the current code works fine. It only breaks if I remover -stdlib=libc++, but then removing the construct overloads also fails.
Comment 7 Marton Danoczy 2012-08-30 15:59:21 UTC
For me, commenting out construct() is only needed with libc++, which makes sense, since Clang's default STL implementation is not C++-11 compliant. Maybe specialzing allocatior_traits is the way to go? According to http://home.roadrunner.com/~hinnant/stack_alloc.html C++-03 allocators should work in C++11...
Comment 8 Martinho Fernandes 2013-09-11 10:27:48 UTC
One cannot remove both overloads of construct. The allocator requirements were weakened in C++11: construct is now *optional*, even though there is a new overload that is *allowed*.

Still, the C++11 overload is supposed to accept *any* pointer type, not just the allocator's `pointer` type. The code should still work fine like this, as allocator_traits is supposed to default to placement-new if a given call to construct is not well-formed. Why it doesn't work with certain compiler/library combinations, I can't tell for sure, but it sounds like a compiler/library bug.

Since the construct overloads are optional, and the current implementations merely perform the default behaviour anyway, they can both be removed for C++11.

Remember however, that I mentioned the requirements were *weakened*. In C++03, the single argument overload is still *required*.

In light of that I removed only the C++11 overload and the sample code compiles and runs fine with all of the following setups:

 - gcc + libstdc++ @ C++03
 - gcc + libstdc++ @ C++11
 - clang + libstdc++ @ C++03
 - clang + libstdc++ @ C++11
 - clang + libc++ @ C++03
 - clang + libc++ @ C++11

(commit here https://bitbucket.org/martinhofernandes/eigen/commits/450a9024838125a0569879450ffd3d512934e914)

I issued pull request #33 (https://bitbucket.org/eigen/eigen/pull-request/33/fix-for-bug-503/diff) for this.
Comment 9 Marton Danoczy 2013-10-01 15:58:37 UTC
I can confirm that both...

* by deleting the C++11 overload, i.e. by reversing the commit https://bitbucket.org/eigen/eigen/commits/ad274b0ede15e89e554f151e21041b7630144f1e

* or by changing the overload as dictated by the standard (see http://en.cppreference.com/w/cpp/memory/allocator/construct)

    template <typename U, typename... Args>
    void  construct(U* p, Args&&... args)
    {
      ::new((void*)p) T(std::forward<Args>(args)...);
    }

the issue is remedied. Could pull request #33 be considered by any chance?
Comment 10 Gael Guennebaud 2013-10-29 11:40:58 UTC
pull request accepted. I guess that in bug 455 we hit a compiler bug since I cannot reproduce the issue we had at that time.
Comment 11 Martinho Fernandes 2014-01-10 11:27:50 UTC
Hmm, sadly, that pull request was kinda sorta not so good.

Thing is, removing the overload throws us into the adorable hell that is the spawn of the illicit relationship between implicit conversions and overload resolution.

Yes, `std::allocator_traits` will fill in whatever `construct` overloads are necessary, but in some cases it will think that the existing C++03 overload is good enough.

Consider:

    struct foo {
        foo(int);
        foo(foo const&) = delete;
    };

This type has an implicit conversion from int and cannot be copy-constructed.

Now suppose we call `v.emplace_back(42)` on some vector of `foo` with our allocator. `allocator_traits` check if `a.construct(42)` is well-formed for our allocator `a`, and if not, will provide its default implementation that does placement-new. But `a.construct(42)` is perfectly well-formed! `42` implicitly converts to a `foo` temporary, and a `foo` temporary matches the existing C++03 overload `construct(foo const&)`. allocator_traits will be extremely happy that for once one of those lazy allocator bastards has taken upon itself to do some work and just forward to that overload.

It's a story that ends in tears: that overload performs a copy and our type is not copyable.

So the C++11 overload *must exist* so that allocator_traits does the right thing and picks it over the C++03 overload. The correct implementation is:

    template <typename U, typename... Args>
    void construct( U* u, Args&&... args)
    {
        ::new( static_cast<void*>(u) ) U( std::forward<Args>( args )... );
    }

See PR#40 https://bitbucket.org/eigen/eigen/pull-request/40
Comment 12 Gael Guennebaud 2014-01-14 13:23:23 UTC
What about making aligned_allocator inherit std::allocator so we do not have to bother about all these details and be more future proof?

We probably also have to add 'using base::construct' to be sure overloads are reachable.
Comment 13 Gael Guennebaud 2014-02-14 15:38:51 UTC
I finally applied the pull-request, but I'd still be interested by a simpler implementation based on std::allocator:

template<class T>
class aligned_allocator : public std::allocator<T>
{
public:
  typedef size_t    size_type;
  typedef std::ptrdiff_t difference_type;
  typedef T*        pointer;
  typedef const T*  const_pointer;
  typedef T&        reference;
  typedef const T&  const_reference;
  typedef T         value_type;

  template<class U>
  struct rebind
  {
      typedef aligned_allocator<U> other;
  };

  aligned_allocator() : std::allocator<T>() { }

  aligned_allocator( const aligned_allocator& other ) : std::allocator<T>(other) { }

  template<class U>
  aligned_allocator( const aligned_allocator<U>& other )
    : std::allocator<T>(other)
  { }

  ~aligned_allocator() { }

  pointer allocate( size_type num, const void* hint = 0 )
  {
    EIGEN_UNUSED_VARIABLE(hint);
    internal::check_size_for_overflow<T>(num);
    return static_cast<pointer>( internal::aligned_malloc( num * sizeof(T) ) );
  }

  void deallocate( pointer p, size_type /*num*/ )
  {
    internal::aligned_free( p );
  }
};

any opinion?
Comment 14 Steven Lovegrove 2014-03-18 14:26:01 UTC
I've been hitting this issue too, but in a slightly modified form. In the code sample below where std::map is used instead of std::set, PR#40 doesn't fully solve the issue. Clang (Output from Apple LLVM version 5.1 (clang-503.0.38) (based on LLVM 3.4svn)) raises compilations errors when using the current Eigen development branch. It compiles fine with gcc 4.6.4.

clang -std=c++0x -stdlib=libc++ -I /usr/local/include/eigen3/ main.cpp 
...
In file included from /usr/local/include/eigen3/Eigen/Core:284:
/usr/local/include/eigen3/Eigen/src/Core/util/Memory.h:768:16: error: static_cast from 'const vertex *' to 'void *' is not allowed
...

#include <Eigen/Core>
#include <map>

struct vertex : Eigen::Vector2d
{
    vertex(double x, double y, int16_t i = -1) :
        Eigen::Vector2d(x,y), index(i) {}

    // strict weak ordering for std::set
    bool operator<(const vertex& v) const
    { return x() != v.x() ? x() < v.x() : y() < v.y(); }

    int16_t index;
};

int main()
{
    std::map<vertex, int,
             std::less<vertex>,
             Eigen::aligned_allocator<
             std::pair<vertex, int>
            > > vertices;

    vertex v(1.0, 2.0);
    vertices[v] = 1.0;

    return 0;
}
Comment 15 Gael Guennebaud 2014-03-19 10:51:26 UTC
(In reply to comment #11)
Martinho, do you have a self-contained example of your use case with class foo, because the following piece of code does not compile with gcc or clang, even without our aligned_allocator.

#include <vector>

struct Foo {
  Foo(int);
  Foo(Foo const&) = delete;
};
    
int main()
{    
  std::vector<Foo> vec;
  vec.emplace_back(42);
  return 0;
}
Comment 16 Gael Guennebaud 2014-03-19 10:53:31 UTC
(In reply to comment #14)

My proposal in comment #13 fixes your issue, though it's weird that construct is called with a const pointer.

I'm trying to gather all the tricky cases we found so far before applying that change.
Comment 17 Martinho Fernandes 2014-03-19 11:29:34 UTC
Apologies for my overly simplistic explanation above.

The issue with the bad `construct` being called shows up with types that are not copyable, as described. The type `foo` in the example is neither movable nor copyable, since providing a deleted definition of the copy constructor inhibits the automatic generation of the move constructor, which means it won't work in most containers, but that is unrelated to the issue with the allocator.

It should look as follows to be usable in vector, and still trigger the bad overload resolution.

struct foo {
  foo(int) {}
  foo(foo &&) = default;
  foo(foo const&) = delete;
};
Comment 18 Gael Guennebaud 2014-03-19 13:30:48 UTC
Right. So I finally make aligned_allocator inherit std::allocator:

https://bitbucket.org/eigen/eigen/commits/f5b7700fef82/
Changeset:   f5b7700fef82
User:        ggael
Date:        2014-03-19 13:28:50
Summary:     Simpler and hopefully more future-proof fix for bug 503 (aligned_allocator with c++11)

Note You need to log in before you can comment on or make changes to this bug.