feedback on supporting libc++

feedback on supporting libc++

[Noah Watkins]

Out of the box on OSX Mavericks libc++ [1] is being used as opposed to
libstdc++. One of the issues is that stuff from tr1 isn't available
(e.g. std::tr1::shared_ptr), as they have moved to std in c++11.

I'm looking for any feedback on this patch set, or if there is a
better way forward.

A set of patches on ceph.git:wip-libc++ [2] adds initial support (with
a couple temporary hacks). These patches are very similar to the
method used to support libc++ in mongodb.

Summary of changes:

  std::tr1::shared/weak_ptr maps to ceph::shared/weak_ptr

  hash_map/set maps to ceph::unordered_map/set

which will choose tr1::unordered_map/set over ext/hash_map/set.

[1] http://libcxx.llvm.org/
[2] https://github.com/ceph/ceph/compare/wip-libc%2B%2B

Re: feedback on supporting libc++

[asomers]

This approach is basically what Kyua did for libc++ compatibility.  It
looks good.  The only things I don't like are the
CEPH_HASH_NAMESPACE_{START,END} macros.  I'm personally not a fan of
macros that change language syntax.  However, I can't come up with an
alternative that doesn't involve either more C++11 features or more
#ifdefs.

-Alan

On Tue, Oct 29, 2013 at 4:51 PM, Noah Watkins <noah.watkins@inktank.com> wrote:
> Out of the box on OSX Mavericks libc++ [1] is being used as opposed to
> libstdc++. One of the issues is that stuff from tr1 isn't available
> (e.g. std::tr1::shared_ptr), as they have moved to std in c++11.
>
> I'm looking for any feedback on this patch set, or if there is a
> better way forward.
>
> A set of patches on ceph.git:wip-libc++ [2] adds initial support (with
> a couple temporary hacks). These patches are very similar to the
> method used to support libc++ in mongodb.
>
> Summary of changes:
>
>   std::tr1::shared/weak_ptr maps to ceph::shared/weak_ptr
>
>   hash_map/set maps to ceph::unordered_map/set
>
> which will choose tr1::unordered_map/set over ext/hash_map/set.
>
> [1] http://libcxx.llvm.org/
> [2] https://github.com/ceph/ceph/compare/wip-libc%2B%2B
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: feedback on supporting libc++

[Josh Durgin]

On 10/29/2013 03:51 PM, Noah Watkins wrote:
> Out of the box on OSX Mavericks libc++ [1] is being used as opposed to
> libstdc++. One of the issues is that stuff from tr1 isn't available
> (e.g. std::tr1::shared_ptr), as they have moved to std in c++11.
>
> I'm looking for any feedback on this patch set, or if there is a
> better way forward.
>
> A set of patches on ceph.git:wip-libc++ [2] adds initial support (with
> a couple temporary hacks). These patches are very similar to the
> method used to support libc++ in mongodb.
>
> Summary of changes:
>
>    std::tr1::shared/weak_ptr maps to ceph::shared/weak_ptr

I'm fine with this approach in general, but we need to be careful about
changing librados.h[pp], since that may break the ABI.

We really should've hidden the entire implementation of ObjectIterator
to begin with, but now we're stuck with it in librados.hpp. This branch
changes a std::tr1::shared_ptr there to a ceph::shard_ptr, which looks
unsafe to me. Could you check whether you can run 'rados ls' compiled
against an old librados, but dynamically loading librados from this
branch compiled in c++98 mode?

Josh

>    hash_map/set maps to ceph::unordered_map/set
>
> which will choose tr1::unordered_map/set over ext/hash_map/set.
>
> [1] http://libcxx.llvm.org/
> [2] https://github.com/ceph/ceph/compare/wip-libc%2B%2B

Re: feedback on supporting libc++

[Noah Watkins]

On Wed, Oct 30, 2013 at 2:02 PM, Josh Durgin <josh.durgin@inktank.com> wrote:
> On 10/29/2013 03:51 PM, Noah Watkins wrote:
>
> unsafe to me. Could you check whether you can run 'rados ls' compiled
> against an old librados, but dynamically loading librados from this
> branch compiled in c++98 mode?

I'm still working on this, but my understanding so far from libc++
documentation is that libc++ and libstdc++ are API but not ABI
compatible, so there shouldn't be an expectation that librados binary
library built against libstc++ will work if dynamically linked against
libc++.

Re: feedback on supporting libc++

[Josh Durgin]

On 12/27/2013 03:34 PM, Noah Watkins wrote:
> On Wed, Oct 30, 2013 at 2:02 PM, Josh Durgin <josh.durgin@inktank.com> wrote:
>> On 10/29/2013 03:51 PM, Noah Watkins wrote:
>>
>> unsafe to me. Could you check whether you can run 'rados ls' compiled
>> against an old librados, but dynamically loading librados from this
>> branch compiled in c++98 mode?
>
> I'm still working on this, but my understanding so far from libc++
> documentation is that libc++ and libstdc++ are API but not ABI
> compatible, so there shouldn't be an expectation that librados binary
> library built against libstc++ will work if dynamically linked against
> libc++.

I meant if it was compiled against libstdc++ both times, I was curious
whether changing std::tr1::shared_ptr to ceph::shared_ptr would result
in any incompatibility.

I just tried this, and it worked fine (I think because it does not
actually create a new c++ type, but acts like a typedef and just
creates an alias), so I've got no issues with this approach.

Josh

Re: feedback on supporting libc++

[Noah Watkins]

Thanks for testing that Josh. Before cleaning up this patch set, I
have a few questions.

I'm still not clear on how to handle the "std::tr1::shared_ptr <
ObjListCtx > ctx;" in librados.hpp. If we change this to
ceph::shared_ptr, then we'll also need to some how ship with the
translations here:

  https://github.com/ceph/ceph/blob/port/libc%2B%2B/src/include/memory.h

It's also not clear that ceph::shared_ptr should be exposed publically
if there is a thought we might start switching out implementations of
ceph::shared_ptr via memory.h (e.g. by using boost implementation).

On Mon, Dec 30, 2013 at 5:19 PM, Josh Durgin <josh.durgin@inktank.com> wrote:
> On 12/27/2013 03:34 PM, Noah Watkins wrote:
>>
>> On Wed, Oct 30, 2013 at 2:02 PM, Josh Durgin <josh.durgin@inktank.com>
>> wrote:
>>>
>>> On 10/29/2013 03:51 PM, Noah Watkins wrote:
>>>
>>> unsafe to me. Could you check whether you can run 'rados ls' compiled
>>> against an old librados, but dynamically loading librados from this
>>> branch compiled in c++98 mode?
>>
>>
>> I'm still working on this, but my understanding so far from libc++
>> documentation is that libc++ and libstdc++ are API but not ABI
>> compatible, so there shouldn't be an expectation that librados binary
>> library built against libstc++ will work if dynamically linked against
>> libc++.
>
>
> I meant if it was compiled against libstdc++ both times, I was curious
> whether changing std::tr1::shared_ptr to ceph::shared_ptr would result
> in any incompatibility.
>
> I just tried this, and it worked fine (I think because it does not
> actually create a new c++ type, but acts like a typedef and just
> creates an alias), so I've got no issues with this approach.
>
> Josh

Re: feedback on supporting libc++

[Josh Durgin]

On 12/31/2013 08:59 AM, Noah Watkins wrote:
> Thanks for testing that Josh. Before cleaning up this patch set, I
> have a few questions.
>
> I'm still not clear on how to handle the "std::tr1::shared_ptr <
> ObjListCtx > ctx;" in librados.hpp. If we change this to
> ceph::shared_ptr, then we'll also need to some how ship with the
> translations here:
>
>    https://github.com/ceph/ceph/blob/port/libc%2B%2B/src/include/memory.h

I'd suggest treating it like we did buffer.h and associated headers -
make a symlink to it from include/rados/memory.h, and install a copy of
it with the librados headers.

> It's also not clear that ceph::shared_ptr should be exposed publically
> if there is a thought we might start switching out implementations of
> ceph::shared_ptr via memory.h (e.g. by using boost implementation).

We can't change the actual type used by librados, since AIUI that's
part of the ABI, so if we want to use another type internally we can
make include/rados/memory.h a copy of the original instead of a symlink,
and then change the internal include/memory.h however we like.

Josh

Re: feedback on supporting libc++

[Noah Watkins]

I've send up a pull request with an initial run at this patch set:

   https://github.com/ceph/ceph/pull/1064

The only thing that we haven't mentioned as possible solutions is to
only use boost variants, rather than switching between the ones in
std:: (in c++11) and std::tr1::.

On Tue, Dec 31, 2013 at 11:59 AM, Josh Durgin <josh.durgin@inktank.com> wrote:
> On 12/31/2013 08:59 AM, Noah Watkins wrote:
>>
>> Thanks for testing that Josh. Before cleaning up this patch set, I
>> have a few questions.
>>
>> I'm still not clear on how to handle the "std::tr1::shared_ptr <
>> ObjListCtx > ctx;" in librados.hpp. If we change this to
>> ceph::shared_ptr, then we'll also need to some how ship with the
>> translations here:
>>
>>    https://github.com/ceph/ceph/blob/port/libc%2B%2B/src/include/memory.h
>
>
> I'd suggest treating it like we did buffer.h and associated headers -
> make a symlink to it from include/rados/memory.h, and install a copy of
> it with the librados headers.
>
>
>> It's also not clear that ceph::shared_ptr should be exposed publically
>> if there is a thought we might start switching out implementations of
>> ceph::shared_ptr via memory.h (e.g. by using boost implementation).
>
>
> We can't change the actual type used by librados, since AIUI that's
> part of the ABI, so if we want to use another type internally we can
> make include/rados/memory.h a copy of the original instead of a symlink,
> and then change the internal include/memory.h however we like.
>
> Josh