std::move and vc10: bug or misunderstanding?

D

Daniel

Consider the following boost test case:

#define BOOST_TEST_MAIN
#include <boost/test/unit_test.hpp>
#include <utility>
#include <string>

class A
{
public:
void value(std::string s)
{
s_ = s;
}
std::string s_;
};

class B
{
public:
void f()
{
for (size_t i = 0; i < 10; ++i)
{
buffer_ = "Hello world";
a.value(std::move(buffer_));
}
}
A a;
std::string buffer_;
};


BOOST_AUTO_TEST_CASE( test1 )
{
B b;
b.f();
}

When run, boost reports 9 occurrences of memory leaks. These leaks vanish when the signature of the value method is changed to value(std:;string&& s); But the original program shouldn't leak, should it? Or am I misunderstanding something?

Thanks,
Daniel
 
S

SG

#define BOOST_TEST_MAIN
#include <boost/test/unit_test.hpp>
#include <utility>
#include <string>

class A
{
public:
    void value(std::string s)
    {
        s_ = s;
    }
    std::string s_;
};

Since you are in C++11 mode, you could replace

s_ = s;

with

s_ = move(s);
class B
{
public:
    void f()
    {
        for (size_t i = 0; i < 10; ++i)
        {
            buffer_ = "Hello world";
            a.value(std::move(buffer_));
        }
    }
    A a;
    std::string buffer_;
};

BOOST_AUTO_TEST_CASE( test1 )
{
    B b;
    b.f();
}

When run, boost reports 9 occurrences of memory leaks.

That's weird. I don't know enough about Boost.Test to explain this but
your program definitely does not contain any memory leaks -- assuming
the std::string implementation is free of bugs. Maybe your std::string
implementation has its own heap management that does not immediately
deallocate memory of a shirt-lived temporary.
These leaks vanish when the signature of the value method is changed
to value(std:;string&& s);

By doing that you just avoid the creation of a lot of temporary string
objects.
But the original program shouldn't leak,
should it? Or am I misunderstanding something?

No, it should not.

I just tried your test using g++ (tdm64-1) 4.6.1 and Boost 1.53 with
the following result:

Running 1 test case...

*** No errors detected

Cheers!
SG
 
D

Daniel

Daniel wrote in


As there is not a single 'new' in your code, there shouldn't be any leak.

So it seems either the std::string implementation or the Boost memory

leak detector seems to be buggy, or something else. On my machine (MSVC

2010 64-bit, old Boost (1.44)) your test does not report any leaks.
Thanks Paavo, I really appreciate your feedback, and that you tried this yourself. As your env is similar to mine (except boost 1.48), I ran the testsagain, and observed that the reported leaks only occurred in Debug, not Release. It's nice to know that this isn't a C++ issue.

Thanks,
Daniel
 
D

Daniel

On May 24, 3:29 am, Daniel wrote:


I just tried your test using g++ (tdm64-1) 4.6.1 and Boost 1.53 with

the following result:



Running 1 test case...



*** No errors detected
Thanks SG, really appreciate that you tried this, after Paavo's feedback and your's, I reran the test, and observed that the leaks, e.g.

Detected memory leaks!
Dumping objects ->
{713} normal block at 0x007BCB98, 16 bytes long.
Data: < > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD

are only reported in Debug, not Release. It's nice to know that this is nota C++ issue.

Thanks,
Daniel
 
V

Victor Bazarov

Thanks SG, really appreciate that you tried this, after Paavo's feedback and your's, I reran the test, and observed that the leaks, e.g.

Detected memory leaks!
Dumping objects ->
{713} normal block at 0x007BCB98, 16 bytes long.
Data: < > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD

are only reported in Debug, not Release. It's nice to know that this is not a C++ issue.

In order for them to be reported in Release you need to use special
tools. In Debug those tools are incorporated into your executable by
the compiler/linker. IOW, if you don't see any leaks in Release, it's
not because there aren't any leaks, it's because you don't look with
proper tools.

If you want to learn more about available tools from Microsoft, read up
on _CrtMemCheckpoint (and related functions), start here:
http://msdn.microsoft.com/en-us/library/vstudio/wc28wkas(v=vs.100).aspx
, or just search the web for "debugging memory leak windows". Plenty of
info available.

V
 
G

Geoff

Thanks Paavo, I really appreciate your feedback, and that you tried this yourself. As your env is similar to mine (except boost 1.48), I ran the tests again, and observed that the reported leaks only occurred in Debug, not Release. It's nice to know that this isn't a C++ issue.

You don't see the leaks in release mode because the release mode code
doesn't check and report them.
 
D

Daniel

In order for them to be reported in Release you need to use special

tools. In Debug those tools are incorporated into your executable by

the compiler/linker. IOW, if you don't see any leaks in Release, it's

not because there aren't any leaks, it's because you don't look with

proper tools.
Thanks for pointing that out!

Best regards,
Daniel
 
G

Gert-Jan de Vos

Thanks Paavo, I really appreciate your feedback, and that you tried this
yourself. As your env is similar to mine (except boost 1.48), I ran the
tests again, and observed that the reported leaks only occurred in Debug,
not Release. It's nice to know that this isn't a C++ issue.

I ran your test with MSVC10 SP1, 32 bit, boost-1.51.0, Debug: no leaks. If
I add this leak: "new int;" I see just this single 4 byte leak reported.

Did you install Service Pack 1?

Gert-Jan
 
D

Daniel

Did you install Service Pack 1?

Yes, Microsoft Visual Studio 2010 Version 10.0.40219.1 SP1Rel, however, given what others are reporting, I'm starting to feel like a minority of one! I better head over to an msdn group, a big thanks to everyone here for your help.

Daniel
 
P

pasa

class A
{
public:
void value(std::string s)
{
s_ = s;
}
std::string s_;
};

class B
{
public:
void f()
{
for (size_t i = 0; i < 10; ++i)
{
buffer_ = "Hello world";
a.value(std::move(buffer_));
}
}
A a;
std::string buffer_;

};

BOOST_AUTO_TEST_CASE( test1 )
{
B b;
b.f();
}

When run, boost reports 9 occurrences of memory leaks. These leaks vanish when the signature of the value method is changed to value(std:;string&& s); But the original program shouldn't leak, should it? Or am I misunderstanding something?

This looks a bug in MS' implementation of std::string.

The leaked thing is allocated in this function:
file xstring line 1981
void _Copy(size_type _Newsize, size_type _Oldlen)
{ // copy _Oldlen elements to newly allocated buffer
size_type _Newres = _Newsize | this->_ALLOC_MASK;
if (max_size() < _Newres)
_Newres = _Newsize; // undo roundup if too big
else if (this->_Myres / 2 <= _Newres / 3)
;
else if (this->_Myres <= max_size() - this->_Myres / 2)
_Newres = this->_Myres
+ this->_Myres / 2; // grow exponentially if possible
else
_Newres = max_size(); // settle for max_size()

_Elem *_Ptr;
_TRY_BEGIN
_Ptr = this->_Alval.allocate(_Newres + 1);
_CATCH_ALL
_Newres = _Newsize; // allocation failed, undo roundup and retry
_TRY_BEGIN
_Ptr = this->_Alval.allocate(_Newres + 1);
_CATCH_ALL
_Tidy(true); // failed again, discard storage and reraise
_RERAISE;
_CATCH_END
_CATCH_END

if (0 < _Oldlen)
_Traits::copy(_Ptr, _Myptr(), _Oldlen); // copy existing elements
_Tidy(true);
this->_Bx._Ptr = _Ptr;
this->_Myres = _Newres;
_Eos(_Oldlen);
}

By the allocate() call in the middle. The pointer is stored in the
union that holds the sting state -- the 16 byte directly or the
pointer to the allocated external storage. It is immediately trampled
over by the last line, calling _Eos, that looks at the length of 11
and treats the union as directly holding the material, applying the 0
terminator at front. The bug is that we should not be here in the
first place.

The caller is
xstring:1957
bool _Grow(size_type _Newsize,
bool _Trim = false)
{ // ensure buffer is big enough, trim to size if _Trim is true
if (max_size() < _Newsize)
_Xlen(); // result too long
if (this->_Myres < _Newsize)
_Copy(_Newsize, this->_Mysize); // reallocate to grow
else if (_Trim && _Newsize < this->_BUF_SIZE)
_Tidy(true, // copy and deallocate if trimming to small string
_Newsize < this->_Mysize ? _Newsize : this->_Mysize);
else if (_Newsize == 0)
_Eos(0); // new size is zero, just null terminate
return (0 < _Newsize); // return true only if more work to do
}
the second if.
I'm lazy to look further, my best guess is that a previous move-from
breaks the internal state, setting the "capacity" to zero instead of
16, that triggers this Grow.

The bug happens only if the content fits the small-string buffer (15+1
chars), with bigger one it is okay.

Good addition to my list of why we should avoid this crapbag called
std::string. :-(

I guess MS will not release any corrections to VS2010 (I recall a deal
of serious bugs closed with nofix), so if you want to actually use
something like this, find the source problem in the move assignment,
then install the modified xstring file to all your machines. Or use
CString or some other sensible string implementation.
 
B

Balog Pal

I ran your test with MSVC10 SP1, 32 bit, boost-1.51.0, Debug: no leaks. If
I add this leak: "new int;" I see just this single 4 byte leak reported.

Did you install Service Pack 1?

I cut it into my app and produced the same leaks, see my other post
confirming the bug.
 
Ö

Öö Tiib

Good addition to my list of why we should avoid this crapbag called
std::string. :-(

Good addition to my list of why we should avoid this crapbag called
VS2010. It is broken in too many ways to count. Either use VS2008 (that
is the repaired version of VS2002) or VS2012 that at least contains
some C++11 support.
 
P

pasa

I'm lazy to look further, my best guess is that a previous move-from
breaks the internal state, setting the "capacity" to zero instead of
16, that triggers this Grow.

Exactly as predicted:

_Myt& assign(_Myt&& _Right)
{ // assign by moving _Right
if (this == &_Right)
;
else if (get_allocator() != _Right.get_allocator()
&& this->_BUF_SIZE <= _Right._Myres)
*this = _Right;
else
{ // not same, clear this and steal from _Right
_Tidy(true);
if (_Right._Myres < this->_BUF_SIZE)
_Traits::move(this->_Bx._Buf, _Right._Bx._Buf,
_Right._Mysize + 1);
else
{ // copy pointer
this->_Bx._Ptr = _Right._Bx._Ptr;
_Right._Bx._Ptr = 0;
}
this->_Mysize = _Right._Mysize;
this->_Myres = _Right._Myres;

_Right._Mysize = 0;
_Right._Myres = 0;
}
return (*this);
}

Note the last
_Right._Myres = 0;
that should happen only under the last condition, for the short case
_Right should better be left alone. With size changing I suspect it
may even have 0-termination at inconsistent place.
 
G

Gert-Jan de Vos

Exactly as predicted:

_Myt& assign(_Myt&& _Right)
{ // assign by moving _Right
if (this == &_Right)
;
else if (get_allocator() != _Right.get_allocator()
&& this->_BUF_SIZE <= _Right._Myres)
*this = _Right;
else
{ // not same, clear this and steal from _Right
_Tidy(true);
if (_Right._Myres < this->_BUF_SIZE)
_Traits::move(this->_Bx._Buf, _Right._Bx._Buf,
_Right._Mysize + 1);
else
{ // copy pointer
this->_Bx._Ptr = _Right._Bx._Ptr;
_Right._Bx._Ptr = 0;
}
this->_Mysize = _Right._Mysize;
this->_Myres = _Right._Myres;

_Right._Mysize = 0;
_Right._Myres = 0;
}
return (*this);
}

Note the last
_Right._Myres = 0;
that should happen only under the last condition, for the short case
_Right should better be left alone. With size changing I suspect it
may even have 0-termination at inconsistent place.

My version of xstring (18/01/2011) has in assign(_Myt&&) instead of:

_Right._Mysize = 0;
_Right._Myres = 0;

This line:

_Right._Tidy();

Tidy sets _Myres to _BUF_SIZE - 1 in this case. This test does not leak
on my system.

These are the version numbers in crtversion.h:

_VC_CRT_MAJOR_VERSION 10
_VC_CRT_MINOR_VERSION 0
_VC_CRT_BUILD_VERSION 40219
_VC_CRT_RBUILD_VERSION 325

Gert-Jan
 
B

Balog Pal

Still no leaks detected with "Microsoft Visual Studio 2010 Version
10.0.40219.1 SP1Rel", tried both 32- and 64-bit Debug modes.

Also, the line 1981 in my copy of xstring is in the _Pdif function, not
_Copy as reported by pasa. Seems like this is some kind of bug in MSVC-s
string implementation which has been fixed at some point (SP1?).

That's interesting, as I also have SP1 installed, and updates enabled --
I recall to see some VS related updates pop up time to time, and I never
reject or hide such a thing.

I'll check the files on our other machines and see whether only mine is
messed up or all, and why.
 
B

Balog Pal

CString is not standard

As majority of code we work with.
not portable,

I did port it to several systems in the past without problems or a
workload worth mentioning -- and majority of code we write is not
intended for porting in the first place. for code written in VS it
applies even more.
decays automatically into const char* pointer,

What is a great feature that makes the std:: version crappy to use with
real-life environment. Like WIN32, someone using VS is pretty likely to
target. And if you deal with code that originated in C, you can replace
char[] buffers without messing up the client code. With std:: version it
requires much work and result in code infested with all those c_str()
noise.

I understand why the standard body did not want that implicit
conversion, but I honestly do not understand most actual users to speak
against. I work with all kind of implicit converting things for more
than two decades and related accidents are still on the theroetic field.
And I keep asking evidence to the contrary and getting silence or
more theory. Certainly I can construct a broken example but it just
fails to happen in practice, and easy to spot on a review. (Or maybe
accident needs some pretty louse policy on pointer usage in general --
for that case I'd guess the implicit conversion will not be a
considerable problem compared to that ;-)
cannot really hold utf-8, etc.

Err, what? Please explain this one.
What's so sensible about it?

It doesn't have hundreds of unneeded member functions, while lacking the
few that are actually needed and used. Like:

- Format !!!
- the trim family
- make upper and lowercase
- buffer interface with Getbuffer/releasebuffer, so I can set the
content from file, resource, WIN32 API call without using a vector<char>
and then construct a string in the next step

Also it is does COW fine in the MT environment as the interface got not
overlooked. It's convenient in general and is a life-saver for some
special cases with massive sharing.

CString was created by actual engineering, thinking use cases for a
string class, not some monster to demonstrate that you can use it with
generic algorithms in theory.

For the opposite question, why could in make sense to use std::string I
have a single bullet, "it is standard". What makes it a widespread
liability. :-o
 
Ö

Öö Tiib

And if you deal with code that originated in C, you can replace
char[] buffers without messing up the client code.

And relying on UB whenever they happen to be passed to printf() or any
other varargs function. I understand MS is doing their best to make this
UB work, but it is still UB and makes the code yet more unportable.

Also, MS static analysis tool prefast goes pretty verbose without
(LPCTSTR) noise in such places.

The same trick (sizeof(char*) == sizeof(std::string)) is used by
some open source std::string implementations as well. Just, mentioning
I'm not advocating such extensions nor their usage.
And if I am using _UNICODE like all Windows code should do nowadays,
such kind of usage compiles fine and silently misbehaves at run-time.

Really? With _UNICODE that CString is wchar-t*. By docs it looks
like one should use '%ls' format specifiers with 'wchar-t*' arguments.
Not sure about MS C RTL however ... isn't MS C still C89?
 
P

pasa

That's interesting, as I also have SP1 installed, and updates enabled --
I recall to see some VS related updates pop up time to time, and I never
reject or hide such a thing.

I'll check the files on our other machines and see whether only mine is
messed up or all, and why.

Other machines appear to have the '11 version of xstring. While they
list the same service packs applied in the about box.
I'm re-applying the SP1 to see what happens...

I have a vague recollection to use 'Repair' on the install once in the
past, possibly it created a state that has older files, but keeps the
installed update entries intact? Not very fun. :-(
 
P

pasa

Other machines appear to have the '11 version of xstring. While they
list the same service packs applied in the about box.
I'm re-applying the SP1 to see what happens...

I have a vague recollection to use 'Repair' on the install once in the
past, possibly it created a state that has older files, but keeps the
installed update entries intact?  Not very fun. :-(

Using 'Reapply' of the SP1 did not change a thing after using up
insane amount of time.
I had to remove the SP. (The remove action produced a couple of '11
files that belong to some hotfix.)

Then install it again and the fixes from win update -- not really the
fixed files are there and the memory leak disappeared.

Sorry for this last OT, but some other users may get hit be the same
problem, the repair functionality of the installers are FUBAR.
 
B

Balog Pal

You mean, copying (parts of) MFC over to another platform and hacking it
to work there? Is this even legal?

Something like that. Not sure on the legal fine print, probably it's in
the gray area as I sell the end product didn't see restrictions on where
I compile my source. Even if i couldn't use the original code it is no
problem to re-implement the interface that is pretty lean. But who
cares when YAGNI applies to this case.
decays automatically into const char* pointer,

What is a great feature that makes the std:: version crappy to use with
real-life environment. Like WIN32, someone using VS is pretty likely to
target. And if you deal with code that originated in C, you can replace
char[] buffers without messing up the client code.

And relying on UB whenever they happen to be passed to printf() or any
other varargs function.

It is not UB, as the implementation defines the behavior for the case.
You can find it somewhere on msdn.
I understand MS is doing their best to make this
UB work, but it is still UB and makes the code yet more unportable.

Undefined by the standard, defined by implementation -- sum is well
defined, and working fine. Certainly on the a different implementation
things could change. Until the theoretic shit hits you're way ahead as
the alternative does not work on any platform to start with.
And
if I am using _UNICODE like all Windows code should do nowadays, such
kind of usage compiles fine and silently misbehaves at run-time.

Dunno, I don't use _UNICODE. But for that realm you shall use all kind
of 'T' stuff consistently and if you do everything works fine.

Also IMO the printf typesafety issue is quite last-century, we have a
ton of static analyzers and this check is the most basic one. and those
who refuse to use static anal have way bigger trouble to start with --
just look the pages of companies creating those tools, most run their
program against open codebases and show example problems and counts. ;-)

This means you have done the conversion only halfways. And I dislike more
the (LPCTSTR) noise advocated by MS.

I lost you here.
Just because in an Unicode-aware Windows program CString would be UTF-16.

You're welcome to use CStringA and CStringW instead if config-dependent
changes bother you. But if one wants to have common code that can be
compiled for either 8 or 16 bit chars the MS way works fine even if with
some noise.

In a real setup I'd push for a global decision on one type of config and
just not support the other and enjoy noiseless coding.
OK, I gather one could use some specialization of CStringT instead, but I
have never bothered to do this.
?


In my mind, it is easier to ignore part of std::string interface than
ignore the whole of it and learn something else instead.

So you ignore all the 100+ functions and are left with just three usable
ones: regular ctor, range ctor and op+. Is that really worth a standard
class? bah.
And besides,
according the MSDN documentation CStringT has more member functions (35)
than basic_string (32). Maybe you indeed get hundreds if you count
overloads, but overloads are all doing basically the same jobs so
learning or ignoring them is easier.

IMO overloads are still members, but we can drop it and switch to count
useful things only.
Plus std::string does have some member functions which I use all the time
and which are lacking in CString:

- find_first_not_of, find_last_not_of
- compare with offset and length arguments
- append with offset and length arguments

The last one sounds like Mid(), the others I never needed.
While these are handy functions sometimes, they have the common problem
that they are locale-dependent and thus the results are basically
unpredictable (and there are no variants taking explicit locale
arguments).

That is true, but poor excuse to discard the most common use.

In my work the locale-specific strings are almost a different family,
while I have heavy use that is tied to ASCII set. "Key"s in the program
used in all kind of technical files and such.

The really localized things are firewalled away from the rest of the
program.
Even the wide version CString MakeUpper() seems to depend on
the narrow codepage locale (by reasons beyond my comprehension) and
produces some crap instead of working properly.

Probably so, but it's a different story for a different day. Messing the
core use cases of the string is a source of problems. One violent case
to recall: we had problems with xml and especially xslt conversions on
linux. it took hours literally. We profiled the problem to operator ==
of Glib::ustring. Which we replaced (in experimental setup) with simple
strcmp, to gain some 100x speed with the same behavior. It was
normalizing the content before doing actual compare -- as unicode is
messed up with precomposed characters and other such stuff. Certainly
nothing like was needed with our data that had ascii keys only, and even
if we wanted arbitrary things it would be presented in a consistent,
prenormalized way. So simple strcmp would serve.

IMNSHO imperfect generic locale support should never prevent
implementation of the most frequent uses of strings.
I think this is a single point for CString so far.

And a really big one really -- especially as I mentioned that in real
life you will have many extensions, where string will be a basic LEGO
piece. The C++98 version of std::string is just hostile for them with
its forced copy and lack of linear storage. While CString has all the
pieces out of the box.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
473,744
Messages
2,569,483
Members
44,902
Latest member
Elena68X5

Latest Threads

Top