[libcxx] r203587 - Implement LWG 2360: 'reverse_iterator::operator*() is unimplementable'. Note that this is a (small) behavior change in the library. Reverse iterators whose base iterators' operator* return references to 'within themselves' have been sacrificed to the greater goal of avoiding data races.

[John McCall]

On Mar 11, 2014, at 6:12 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Tue, Mar 11, 2014 at 5:16 PM, John McCall <rjmccall at apple.com> wrote:
> On Mar 11, 2014, at 2:56 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Tue, Mar 11, 2014 at 2:41 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
>> 
>> On Mar 11, 2014, at 2:14 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>>> I’m open to suggestions here:
>>> a) I can put the instance variable back, and never use it. No ABI break, but an unused iterator in every reverse iterator.
>>> 
>>> Any code that passes a reverse_iterator between TUs built against different libc++ versions would still not work.
>> 
>> Ok, I missed this comment; I thought it applied to the change - not the proposed fix.
>> I disagree. I do not believe that putting the instance variable back and not using it will not break code across TUs.
>> 
>> The only use of the instance variable __t is in the operator*() for the reverse iterator, which used to say:
>> 	_LIBCPP_INLINE_VISIBILITY reference operator*() const {__t = current; return *--__t;}
>> (i.e, the variable is set immediately before use)
>> but now says:
>> 	_LIBCPP_INLINE_VISIBILITY reference operator*() const {_Iter __tmp = current; return *--__tmp;}
>> 
>> The purpose of having this instance variable “stick around”, rather than being a local variable, was to support iterators whose operator* returned a reference to something inside of the iterator. If the iterator was a local variable, then the things contained inside of it would be destroyed when the iterator was destroyed.
>> 
>> However, in #2360, STL at microsoft.com pointed out that this was inviting silent data races, and the LWG decided not to support those kind of iterators any more.
>> (see the comments on LWG issue #198, which #2360 partially reversed).
>> 
>> Richard — clearly you think otherwise.
>> I’d like to hear your explanation as to why.
>> 
>> On further study, I think the approach you're talking about is fine. You'd need to keep the actual instance variable, not just storage appropriate for it (you still need to call the default constructor and destructor, if any), and you'll probably need it to still be 'mutable' to avoid more subtle changes.
>> 
>> (... and we should remove that member in the 'unstable ABI' mode when we have it!)
> 
> For what it’s worth, I’ve been planning to submit a patch soon which would allow data-structure-specific ABI versioning in order to help deal with the std::pair problem, among others, along the lines of what Richard suggested except leaving unaffected data structures at version __1.
> 
> I would propose that libc++ should default to ABI instability (except, obviously, with its own dynamic library), but that a user should be able to simply pass -D_LIBCPP_ABI_VERSION=3 and get reliable interoperability.
> 
> I was thinking something along the lines of:
> 
> 
> 
> /* __config */
> 
> // ABI version log:
> //   1 - The beginning of time.
> //   2 - A std::pair of trivially copyable types should be trivially copyable.
> //   3 - ...
> #ifndef _LIBCPP_ABI_VERSION
> #define _LIBCPP_ABI_VERSION 3   // bump the current number
> #endif
> 
> #ifndef _LIBCPP_PAIR_ABI_VERSION
> #if _LIBCPP_ABI_VERSION >= 2
> #define _LIBCPP_PAIR_ABI_VERSION 2
> #else
> #define _LIBCPP_PAIR_ABI_VERSION 1
> #endif
> #endif
> 
> // …more clauses here as ABI changes warrant...
> 
> // Everything that hasn’t had an ABI revision.
> #ifndef _LIBCPP_DEFAULT_ABI_VERSION
> #define _LIBCPP_DEFAULT_ABI_VERSION 1
> #endif
> 
> …
> 
> #define _LIBCPP_BEGIN_NAMESPACE_STD(VERSION) namespace std {inline namespace _LIBCPP_NAMESPACE(VERSION) {
> #define _LIBCPP_BEGIN_NAMESPACE_STD_DEFAULT _LIBCPP_BEGIN_NAMESPACE_STD(_LIBCPP_DEFAULT_ABI_VERSION)
> 
> …
> 
> 
> 
> /* utility */
> 
> _LIBCPP_BEGIN_NAMESPACE_STD_DEFAULT
> /* everything up to std::pair */
> _LIBCPP_END_NAMESPACE_STD
> 
> _LIBCPP_BEGIN_NAMESPACE_STD(_LIBCPP_PAIR_ABI_VERSION)
> template <class T, class U> class pair {
> #if _LIBCPP_PAIR_ABI_VERSION < 2
>   /* copy constructor etc. */
> #endif
> };
> _LIBCPP_END_NAMESPACE_STD
> 
> 
> 
> Thoughts?  Better ideas for the main version sequence?
> 
> Can we do without the separate _LIBCPP_ABI_VERSION and _LIBCPP_PAIR_ABI_VERSION? Something like:
> 
> _LIBCPP_BEGIN_NAMESPACE_STD_DEFAULT
> // ...
> _LIBCPP_END_NAMESPACE_STD
> 
> _LIBCPP_BEGIN_NAMESPACE_STD_VERSION(1, 2)
> template <class T, class U> class pair {
> #if _LIBCPP_ABI_VERSION < 2
> // ...
> #endif
> // ...
> };
> _LIBCPP_END_NAMESPACE_STD
> 
> ... where the _VERSION macro is passed the set of ABI versions where pair changed, and it picks the latest supplied number that is <= _LIBCPP_ABI_VERSION.

I don’t think that macro is possible to write without something like Boost Preprocessor (which is way too heavyweight to inflict on every user of the standard library).  Remember that we need it to token-expand to a literal, not just fold to a constant.

But you’re right that we neither need nor want to support mixing and matching different ABI versions of different data structures, which at least removes some #ifndef checks.

> We'll need to be careful to version-bump everything that depends on 'pair' when we version-bump ‘pair’.

Yeah, that might make this annoying.  On the other hand, for a lot of things like std::map, we can just use _LIBCPP_PAIR_ABI_VERSION until there are multiple ABI breaks in play for that data structure, which is an argument in favor of actually having those macros.  We could move them into the appropriate headers, though.

I think we actually *can’t* just bump the version across the board; there are symbols in the __1 namespace that are only defined in the shared library (and not just explicit instantiations).

> It'd be nice if we had some enforcement that a __1 name can't use a >= __2 name.

That seems like a reasonable tool for somebody else to whip up. :)

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140311/41a68f86/attachment.html>