Bug 38630 – VS2010 asserts a null iterator passed to std::copy in Vector::operator=

• Status: CLOSED
• Resolution: FIXED
• Component: Web Template Framework

[Jocelyn Turcotte]

This happens when copying an empty Vector over another empty Vector.

Release builds works correctly.

[Jocelyn Turcotte]

Created attachment 55211 [details]
Patch

Fixes the assert.

[Alexey Proskuryakov]

It's not good for performance to add another branch. Perhaps this should only be done when building with VS2010? And a bug should probably be filed with Microsoft.

[Kent Tamura]

AFAIK, passing 0 to an Iterator parameter is invalid in C++. So this behavior of VS2010 seems reasonable.
If we worry about the performance, we should use "#if COMPILER(MSVC)" here.

[Kent Tamura]

I think we should add just

} else if (!other.size())
    return *this;

(No removing "if (!begin()) return *this;")

This is an optimization for all compilers and solve the VS2010 issue.

[Alexey Proskuryakov]

I do not see any restrictions on input or output iterators in copy() algorithm
description in C++ standard. Do you have a reference for this requirement?

> This is an optimization for all compilers and solve the VS2010 issue.

That also sounds like an extra branch in common case. Or did I misunderstand your proposal?

[Kent Tamura]

(In reply to comment #5)
> I do not see any restrictions on input or output iterators in copy() algorithm
> description in C++ standard. Do you have a reference for this requirement?

I checked the C++0x draft, and I think i was wrong.  0 can be a valid iterator in general.

The standard says neither "The OutputIterator parameter must be dereferenceable" nor "It may be not dereferenceable if [first, last) is empty."  So we can't assume the VS2010 behavior is a bug.

> > This is an optimization for all compilers and solve the VS2010 issue.
> 
> That also sounds like an extra branch in common case. Or did I misunderstand your proposal?

Yes, it will make an extra branch.  But it will skip the following 3-lines of the code in a case of empty vectors.
I have no strong preference on it.  I think either of it or "#if COMPILER(MSVC)" are ok.

[Jocelyn Turcotte]

Created attachment 55543 [details]
Kent's proposal patch

Here is a patch for the proposed fix.

This adds a branch when other.size() is non-zero and fits in capacity(), but exit earlier in other cases.

I would also be ok with wrapping the previous patch in a MSVC/MSVC2010 specific macro piece if you think this one has issues.

Anyhow the msvc behavior, being a bug or not, has been reported there:
https://connect.microsoft.com/VisualStudio/feedback/details/558044/std-copy-should-not-check-dest-when-first-last

[WebKit Review Bot]

Attachment 55543 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/Vector.h:680:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
JavaScriptCore/wtf/Vector.h:705:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jocelyn Turcotte]

Created attachment 55545 [details]
Kent's proposal patch v2

Fix style issues

[Kent Tamura]

Comment on attachment 55545 [details]
Kent's proposal patch v2

> @@ -677,14 +677,17 @@ namespace WTF {
>          if (&other == this)
>              return *this;
>          
> -        if (size() > other.size())
> +        if (size() > other.size()) {
>              shrink(other.size());
> -        else if (other.size() > capacity()) {
> +            return *this;
> +        }

This branch just resizes 'this' and doesn't copy the content of 'other'.

[Alexey Proskuryakov]

Comment on attachment 55545 [details]
Kent's proposal patch v2

> The standard says neither "The OutputIterator parameter must be dereferenceable"
> nor "It may be not dereferenceable if [first, last) is empty."  So we can't
> assume the VS2010 behavior is a bug.

The standard doesn't need to explicitly mention everything it allows. If there is no restriction, then it's allowed.

Please make this fix only for MSVC2010, file a bug with them, and reference this bug in a comment in code, so that we could know when to remove the workaround.

-        if (size() > other.size())
+        if (size() > other.size()) {
             shrink(other.size());
-        else if (other.size() > capacity()) {
+            return *this;
+        }

This doesn't look like it could possibly work. Did you run regression tests with this patch? Or am I just confused?

[Alexey Proskuryakov]

> This branch just resizes 'this' and doesn't copy the content of 'other'.

Sorry, didn't see your comment before reviewing. So, I'm not just confused :-)

[Jocelyn Turcotte]

Created attachment 55682 [details]
Patch v2

(In reply to comment #11)
> This doesn't look like it could possibly work. Did you run regression tests with this patch? Or am I just confused?

No, there was a lack of testing from my part on this patch, sorry about that.

Here is the first patch enclosed to apply only when the assert would pop.
Adding only "else if (!other.size()) return *this;" would not prevent the assert when size() > other.size() and the buffer gets shrunk to 0.

See Comment #7 for a link to the reported visual studio bug.

[Alexey Proskuryakov]

Comment on attachment 55682 [details]
Patch v2

+// Workarounds an assert in VS2010. See https://bugs.webkit.org/show_bug.cgi?id=38630

There is no verb "workarounds", it's either a noun or "works around". I still think that it would be better to have a direct link to MS issue. The use case for this comment is: can I remove this code now? - click link - read. With a Bugzilla link, there is an extra step involved. On the other hand, the Bugzilla link may be more stable.

r=me, but please fix the grammar issue.

[Jocelyn Turcotte]

Committed r59463: <http://trac.webkit.org/changeset/59463>

Changes since the review:
- Fixed the grammar issue
- The link in the code points to the VS issue instead
- Changed the preprocessor expression to also match when _ITERATOR_DEBUG_LEVEL == 1

thanks!

[Simon Hausmann]

Revision r59463 cherry-picked into qtwebkit-2.0 with commit a1a9dda8011dbd6ffc40bad8b4e62d2d3bf95916

[Jocelyn Turcotte]

Update:
The bug has been fixed in Visual Studio and the fix will be available in VS2011.

http://connect.microsoft.com/VisualStudio/feedback/details/558044/std-copy-should-not-check-dest-when-first-last

[Jocelyn Turcotte]

(In reply to comment #17)
> The bug has been fixed in Visual Studio and the fix will be available in VS2011.
err.. I mean VC11