llvm dev - Oct 2010 - [LLVMdev] MS VS2010 std implementation: "Cannot assign iterators to two different blocks!"

When using MS VS2010 there is an issue with std:

 

`SuccIterator` implements a partial assignment operator:

 

inline const _Self &operator=(const _Self &I) {

    assert(Term == I.Term &&"Cannot assign iterators to two
different
blocks!");

    idx = I.idx;

    return *this;

    }

 

For copy construction, MS VS2010 std reserves the right, and sometimes
calls,

a method to reverse elements which does so by swap:

 

 template<class _Ty> inline

 void swap(_Ty& _Left, _Ty& _Right)

 {             // exchange values stored at _Left and _Right

   _Ty _Tmp = _Move(_Left);

   _Left = _Move(_Right); <<<<<<<<<<<<<
Needs FULL assignment operator!!!!

   _Right = _Move(_Tmp);

 }

 

When `_Ty` is `SuccIterator const` the assert "Cannot assign iterators to
two different blocks!" can fail.

 

Can this be a workaround:

 

inline const _Self &operator=(const _Self &I) {

  new (this) _Self(I);

  return *this;

}

 

The larger question is can both the assert be preserved and swap satisfied?

  

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20101004/edd32556/attachment.html>

Hi Bob, was this issue resolved?

Ciao,

Duncan.
> When using MS VS2010 there is an issue with std:
>
> `SuccIterator` implements a partial assignment operator:
>
> inline const _Self &operator=(const _Self &I) {
>
> assert(Term == I.Term &&"Cannot assign iterators to two
different blocks!");
>
> idx = I.idx;
>
> return *this;
>
> }
>
> For copy construction, MS VS2010 std reserves the right, and sometimes
calls,
>
> a method to reverse elements which does so by swap:
>
> template<class _Ty> inline
>
> void swap(_Ty& _Left, _Ty& _Right)
>
> { // exchange values stored at _Left and _Right
>
> _Ty _Tmp = _Move(_Left);
>
> _Left = _Move(_Right); <<<<<<<<<<<<<
Needs FULL assignment operator!!!!
>
> _Right = _Move(_Tmp);
>
> }
>
> When `_Ty` is `SuccIterator const` the assert "Cannot assign iterators
to two
> different blocks!" can fail.
>
> Can this be a workaround:
>
> inline const _Self &operator=(const _Self &I) {
>
> new (this) _Self(I);
>
> return *this;
>
> }
>
> The larger question is can both the assert be preserved and swap satisfied?
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Duncan,

As I am both an LLVM user and new to it I do not check in any changes, so I
have not made a fix.

However, to make LLVM work under VS2010 in my environment I replaced the
assignment constructor with the following
which fixes the issue on the simple tests in my environment which I have
tried. Any developer can validate this and check it in, although this fix
deletes the assert check and is not particularly pretty to my taste.

Bob

In CFG.h, replace the assignment constructor with:

////////////////////////////////////////////////////////////////////////////
//////////
  inline const _Self &operator=(const _Self &I) {
    this->~SuccIterator();
    new (this) _Self(I);
    // VS2010 std implements deque (on which stack is built) copy
constructor
    // as reserving the right to swap elements. The swap method uses
`operator=`
    // as follows:
    // template<class _Ty> inline
    //    void swap(_Ty& _Left, _Ty& _Right)
    //	  {	// exchange values stored at _Left and _Right
    //	    _Ty _Tmp = _Move(_Left);
    //	    _Left  = _Move(_Right);   <<<<<<<<< calls
this `operator=`
    //	    _Right = _Move(_Tmp);
    //	  }
    // However, `_Self`, does not implement a full assignment operator
    // in favor of defense against misuse.
#ifdef PROBLEM    
    assert(Term == I.Term &&"Cannot assign iterators to two
different
blocks!");
    idx = I.idx;
#endif // PROBLEM    
    return *this;
  }
////////////////////////////////////////////////////////////////////////////
/////////////

-----Original Message-----
From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On
Behalf Of Duncan Sands
Sent: Wednesday, October 20, 2010 5:00 AM
To: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] MS VS2010 std implementation: "Cannot assign
iterators to two different blocks!"

Hi Bob, was this issue resolved?

Ciao,

Duncan.
> When using MS VS2010 there is an issue with std:
>
> `SuccIterator` implements a partial assignment operator:
>
> inline const _Self &operator=(const _Self &I) {
>
> assert(Term == I.Term &&"Cannot assign iterators to two
different
blocks!");
>
> idx = I.idx;
>
> return *this;
>
> }
>
> For copy construction, MS VS2010 std reserves the right, and sometimes
calls,
>
> a method to reverse elements which does so by swap:
>
> template<class _Ty> inline
>
> void swap(_Ty& _Left, _Ty& _Right)
>
> { // exchange values stored at _Left and _Right
>
> _Ty _Tmp = _Move(_Left);
>
> _Left = _Move(_Right); <<<<<<<<<<<<<
Needs FULL assignment operator!!!!
>
> _Right = _Move(_Tmp);
>
> }
>
> When `_Ty` is `SuccIterator const` the assert "Cannot assign iterators
to
two
> different blocks!" can fail.
>
> Can this be a workaround:
>
> inline const _Self &operator=(const _Self &I) {
>
> new (this) _Self(I);
>
> return *this;
>
> }
>
> The larger question is can both the assert be preserved and swap
satisfied?
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
_______________________________________________
LLVM Developers mailing list
LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Mon, Oct 4, 2010 at 9:52 PM, Bob Floyd <bobfloyd at comcast.net>
wrote:
> When using MS VS2010 there is an issue with std:
>
>
>
> `SuccIterator` implements a partial assignment operator:
>
>
>
> inline const _Self &operator=(const _Self &I) {
>
>     assert(Term == I.Term &&"Cannot assign iterators to two
different
> blocks!");
>
>     idx = I.idx;
>
>     return *this;
>
>     }
>
>
>
> For copy construction, MS VS2010 std reserves the right, and sometimes
> calls,
>
> a method to reverse elements which does so by swap:
>
>
>
>  template<class _Ty> inline
>
>  void swap(_Ty& _Left, _Ty& _Right)
>
>  {             // exchange values stored at _Left and _Right
>
>    _Ty _Tmp = _Move(_Left);
This uses the compiler generated default copy constructor which does
an element by element copy.
>    _Left = _Move(_Right);
<<<<<<<<<<<<< Needs FULL assignment
operator!!!!
Given that SuccIterator does not implement move semantics, we can
basically remove all the _Move's here, they just turn into
assignments. Thus _Left and _Right are both fully constructed.
_Left.Term == _Right.Term is true if it was true before swap.
>    _Right = _Move(_Tmp);
>
>  }
>
>
>
> When `_Ty` is `SuccIterator const` the assert "Cannot assign iterators
to
> two different blocks!" can fail.
>
>
>
> Can this be a workaround:
>
>
>
> inline const _Self &operator=(const _Self &I) {
>
>   new (this) _Self(I);
>
>   return *this;
>
> }
>
>
>
> The larger question is can both the assert be preserved and swap satisfied?
I believe this is an error in your code. Are you 100% sure that
_Left.Term == _Right.Term before swap is called?

- Michael Spencer

Michael,

Yes, while stepping through this VS2010 `std::swap` code, `_Move` did nothing,
e.g.

////////////////////////////////////////////////////
void swap(_Ty& _Left, _Ty& _Right)
{	// exchange values stored at _Left and _Right
  _Ty _Tmp = _Move(_Left);
  _Left  = _Move(_Right);
  _Right = _Move(_Tmp);
}
////////////////////////////////////////////////////

was equivalent to:

////////////////////////////////////////////////////
void swap(_Ty& _Left, _Ty& _Right)
{	// exchange values stored at _Left and _Right
  _Ty _Tmp = Left;
  _Left  = _Right;   
  _Right = Tmp;
}
////////////////////////////////////////////////////

the deque being reversed had 3 items: `a1`, `a2`, and `a3` (`SuccIterator&`
types)

the 1st call to swap `_Left` was `a1` and `_Right` was `a3`

the assignment: "_Left = _Right" was "a1 = a3" 

`a1` and `a3` had different `SuccIterator::Term`s, causing the assert in
`SuccIterator::operator=` to fail.

This all happened when an llvm stack was being copy constructed (right now I
can't be more specific since I'm in-between running code). Finding the
microsoft std library doing a reverse during a simple stack copy was a surprise.
But on the other hand, since the stack was implemented using deque, it appeared
the ms std code was being clever with it.

As I recall, I was not surprised that the `SuccIterator::Term`s in the llvm
stack were different. Are you saying
the `Term`s in the llvm stack should have been the same, meaning, perhaps, I
have used the API incorrectly? After replacing the `SuccIterator::operator=` I
hand-checked the JIT code generated by assembly stepping through it and it was
Ok. Suggesting I populated Ok, and different `SuccIterator::Term`s were ok.

Could a solution be to implement `_Move` in `SuccIterator`? I am not familiar
with `_Move` semantics in MS std library.

Bob

-----Original Message-----
From: Michael Spencer [mailto:bigcheesegs at gmail.com] 
Sent: Thursday, October 21, 2010 7:53 AM
To: Bob Floyd
Cc: llvmdev at cs.uiuc.edu
Subject: Re: [LLVMdev] MS VS2010 std implementation: "Cannot assign
iterators to two different blocks!"

On Mon, Oct 4, 2010 at 9:52 PM, Bob Floyd <bobfloyd at comcast.net>
wrote:
> When using MS VS2010 there is an issue with std:
>
>
>
> `SuccIterator` implements a partial assignment operator:
>
>
>
> inline const _Self &operator=(const _Self &I) {
>
> ��� assert(Term == I.Term &&"Cannot assign iterators to two
different
> blocks!");
>
> ��� idx = I.idx;
>
> ��� return *this;
>
> ��� }
>
>
>
> For copy construction, MS VS2010 std reserves the right, and sometimes
> calls,
>
> a method to reverse elements which does so by swap:
>
>
>
> �template<class _Ty> inline
>
> �void swap(_Ty& _Left, _Ty& _Right)
>
> �{������������ // exchange values stored at _Left and _Right
>
> �� _Ty _Tmp = _Move(_Left);
This uses the compiler generated default copy constructor which does
an element by element copy.
> �� _Left = _Move(_Right);
<<<<<<<<<<<<< Needs FULL assignment
operator!!!!
Given that SuccIterator does not implement move semantics, we can
basically remove all the _Move's here, they just turn into
assignments. Thus _Left and _Right are both fully constructed.
_Left.Term == _Right.Term is true if it was true before swap.
> �� _Right = _Move(_Tmp);
>
> �}
>
>
>
> When `_Ty` is `SuccIterator const` the assert "Cannot assign iterators
to
> two different blocks!" can fail.
>
>
>
> Can this be a workaround:
>
>
>
> inline const _Self &operator=(const _Self &I) {
>
> � new (this) _Self(I);
>
> � return *this;
>
> }
>
>
>
> The larger question is can both the assert be preserved and swap satisfied?
I believe this is an error in your code. Are you 100% sure that
_Left.Term == _Right.Term before swap is called?

- Michael Spencer