|
From: Stephen T. <st...@to...> - 2005-11-28 09:05:19
|
Am I reading the following message correct when it says that I am having
an error in a destructor of a class I have? I don't see how there can be
an invalid read of size 4 which the code sure sure looks fine.
The error message goes like:
=3D=3D19262=3D=3D Invalid read of size 4
=3D=3D19262=3D=3D at 0x804D29E: guitar::Chord::~Chord() (chord.cpp:32)
=3D=3D19262=3D=3D by 0x804E9A8: guitar::ChordMap::~ChordMap() (chordmap.=
cpp:45)
=3D=3D19262=3D=3D by 0x804BCEB: test_insert() (test_chordmap.cpp:51)
=3D=3D19262=3D=3D by 0x804C051: main (test_chordmap.cpp:86)
The destructor is claiming that is wrong is:
Chord::~Chord ()
{
delete m_name;
delete m_arrangement;
}
The variables m_name and m_arrangement are given in the constructors like s=
o:
Chord::Chord ( ChordName* name, Fingering* arrange )
: m_name ( new ChordName( *name ) ),
m_arrangement ( new Fingering( *arrange ) ),
m_written( false )
{}
I am doing a deep copy to show that the class will claim
ownership/responsibility to destroy them when the destructor is called
since they don't mean anything outside this class. So I am not sure why
valgrind is complaining.
I am using valgrind 3.0.1.
Stephen
|
|
From: Tom H. <to...@co...> - 2005-11-28 09:50:38
|
In message <113...@ba...>
Stephen Torri <st...@to...> wrote:
> Am I reading the following message correct when it says that I am having
> an error in a destructor of a class I have? I don't see how there can be
> an invalid read of size 4 which the code sure sure looks fine.
>
> The error message goes like:
>
> ==19262== Invalid read of size 4
> ==19262== at 0x804D29E: guitar::Chord::~Chord() (chord.cpp:32)
> ==19262== by 0x804E9A8: guitar::ChordMap::~ChordMap() (chordmap.cpp:45)
> ==19262== by 0x804BCEB: test_insert() (test_chordmap.cpp:51)
> ==19262== by 0x804C051: main (test_chordmap.cpp:86)
>
>
> The destructor is claiming that is wrong is:
>
> Chord::~Chord ()
> {
> delete m_name;
> delete m_arrangement;
> }
Well you haven't said what line that error is coming from, but as it
is an invalid read rather than a complaint about freeing an already
freed block the problem is likely to be when it loads m_name from the
object.
In other words the this pointer is probably bogus - ie you are calling
delete on a pointer that is not a pointer to a valid Chord object - in
fact it is not a pointer to allocated memory at all.
Tom
--
Tom Hughes (to...@co...)
http://www.compton.nu/
|
|
From: Stephen T. <st...@to...> - 2005-11-28 15:54:14
|
On Mon, 2005-11-28 at 09:50 +0000, Tom Hughes wrote:
> In message <113...@ba...>
> Stephen Torri <st...@to...> wrote:
>=20
> > Am I reading the following message correct when it says that I am havin=
g
> > an error in a destructor of a class I have? I don't see how there can b=
e
> > an invalid read of size 4 which the code sure sure looks fine.
> >
> > The error message goes like:
> >
> > =3D=3D19262=3D=3D Invalid read of size 4
> > =3D=3D19262=3D=3D at 0x804D29E: guitar::Chord::~Chord() (chord.cpp:3=
2)
> > =3D=3D19262=3D=3D by 0x804E9A8: guitar::ChordMap::~ChordMap() (chord=
map.cpp:45)
> > =3D=3D19262=3D=3D by 0x804BCEB: test_insert() (test_chordmap.cpp:51)
> > =3D=3D19262=3D=3D by 0x804C051: main (test_chordmap.cpp:86)
> >
> >
> > The destructor is claiming that is wrong is:
> >
> > Chord::~Chord ()
> > {
> > delete m_name;
> > delete m_arrangement;
> > }
>=20
> Well you haven't said what line that error is coming from, but as it
> is an invalid read rather than a complaint about freeing an already
> freed block the problem is likely to be when it loads m_name from the
> object.
Your right. Sorry. The error is coming from the 'delete m_name' line.
> In other words the this pointer is probably bogus - ie you are calling
> delete on a pointer that is not a pointer to a valid Chord object - in
> fact it is not a pointer to allocated memory at all.
Well the container ChordMap does a deep copy of all items being placed
in it. So when the destructor is called I loop through the internal data
structure and call delete on all pointer in it.=20
I think the problem might be further up the list. In the test program
which is suppose to exercise this class, test_chordmap, I think might
see the problem. The address of the Chord pointer stored in the ChordMap
is an address from the stack.
ChordMap a;
ChordName name;
name.setName("A","Major","7");
Chord cd (&name);
a.insert(&cd);
What I thought was that since the function 'insert' in ChordMap does a
deep copy that I could use a address of a Chord object from the stack
and not need to call delete on the Chord object in this example. So one
less call to delete is what I was trying to save here.
When I changed this example from using stack addresses to pointers and
the message was removed. Yet now I must use delete on these pointers.
ChordMap a;
ChordName* name =3D new ChordName();
name->setName("A","Major","7");
Chord* cd =3D new Chord (name);
a.insert(cd);
Even when I do this I get an "Invalid read of size 4" at the same line
as before 'delete m_name' in destructor of class Chord. In stead of the
delete being called on the chord pointer from in the destructor of class
ChordMap its being called at the end of the function test_insert in the
test_chordmap program.
So regardless of how I seem to want to delete this pointer to a Chord
object I get an invalid read.
I believe a fully working test case would be helpful but that will have
to wait until later tonight. So in the meantime I appreciate your help
and discussion.
Stephen
|
|
From: Josef W. <Jos...@gm...> - 2005-11-28 16:47:18
|
On Monday 28 November 2005 16:53, Stephen Torri wrote: > Chord cd (&name); > a.insert(&cd); > What I thought was that since the function 'insert' in ChordMap does a > deep copy Do you have a copy constructor for Chord? The deep copy should also generate a new ChordName object. Otherwise you get a dangling pointer after going out of this scope. Josef |
|
From: Dennis L. <pla...@tz...> - 2005-11-28 16:10:35
|
Am Montag, den 28.11.2005, 03:05 -0600 schrieb Stephen Torri: > Am I reading the following message correct when it says that I am having > an error in a destructor of a class I have? I don't see how there can be > an invalid read of size 4 which the code sure sure looks fine. > > The error message goes like: > > ==19262== Invalid read of size 4 > ==19262== at 0x804D29E: guitar::Chord::~Chord() (chord.cpp:32) > ==19262== by 0x804E9A8: guitar::ChordMap::~ChordMap() (chordmap.cpp:45) > ==19262== by 0x804BCEB: test_insert() (test_chordmap.cpp:51) > ==19262== by 0x804C051: main (test_chordmap.cpp:86) Is this all of the error message ? Usually valgrind tells us what it knows about the memory, e.g.: Address 0x3FA50239 is not stack'd, malloc'd or (recently) free'd or that it has been freed already or similar, it should come right after the stacktrace of the invalid read. greets Dennis |
|
From: Stephen T. <st...@to...> - 2005-11-29 04:18:52
Attachments:
chord.cpp
chordmap.cpp
|
On Mon, 2005-11-28 at 16:46 +0100, Dennis Lubert wrote:
> Is this all of the error message ? Usually valgrind tells us what it
> knows about the memory, e.g.:
>
> Address 0x3FA50239 is not stack'd, malloc'd or (recently) free'd
>
> or that it has been freed already or similar, it should come right after
> the stacktrace of the invalid read.
>
> greets
>
> Dennis
No it is not all there is to the message. I am sorry I did not include
it all. What I send seemed to be sufficient but I did not know what is
expected.
==11257== Invalid read of size 4
==11257== at 0x804D2F6: guitar::Chord::~Chord() (chord.cpp:32)
==11257== by 0x804EA00: guitar::ChordMap::~ChordMap() (chordmap.cpp:45)
==11257== by 0x804BD44: test_insert() (test_chordmap.cpp:54)
==11257== by 0x804C0A9: main (test_chordmap.cpp:88)
==11257== Address 0x1C878270 is 0 bytes inside a block of size 32 free'd
==11257== at 0x1B900807: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck.so)
==11257== by 0x8057E14: guitar::ChordName::~ChordName() (chordname.cpp:40)
==11257== by 0x804D307: guitar::Chord::~Chord() (chord.cpp:32)
==11257== by 0x804BD0C: test_insert() (test_chordmap.cpp:54)
==11257== by 0x804C0A9: main (test_chordmap.cpp:88)
The message after the "Address ... " line does not make sense because
line 40 of chordname.map is the end of the destructor.
ChordName::~ChordName ()
{
for ( std::vector<ChordName*>::iterator pos = m_aliases.begin();
pos != m_aliases.end();
++pos )
{
delete *pos;
}
}
Stephen
|
|
From: Dennis L. <pla...@gm...> - 2005-11-29 05:34:52
|
At 05:18 29.11.2005, Stephen Torri wrote:
>No it is not all there is to the message. I am sorry I did not include
>it all. What I send seemed to be sufficient but I did not know what is
>expected.
>
> ==11257== Invalid read of size 4
> ==11257== at 0x804D2F6: guitar::Chord::~Chord() (chord.cpp:32)
> ==11257== by 0x804EA00: guitar::ChordMap::~ChordMap()
> (chordmap.cpp:45)
> ==11257== by 0x804BD44: test_insert() (test_chordmap.cpp:54)
> ==11257== by 0x804C0A9: main (test_chordmap.cpp:88)
> ==11257== Address 0x1C878270 is 0 bytes inside a block of
> size 32 free'd
> ==11257== at 0x1B900807: operator delete(void*) (in
> /usr/lib/valgrind/vgpreload_memcheck.so)
> ==11257== by 0x8057E14: guitar::ChordName::~ChordName()
> (chordname.cpp:40)
> ==11257== by 0x804D307: guitar::Chord::~Chord() (chord.cpp:32)
> ==11257== by 0x804BD0C: test_insert() (test_chordmap.cpp:54)
> ==11257== by 0x804C0A9: main (test_chordmap.cpp:88)
>
>The message after the "Address ... " line does not make sense because
>line 40 of chordname.map is the end of the destructor.
>
> ChordName::~ChordName ()
> {
> for ( std::vector<ChordName*>::iterator pos = m_aliases.begin();
> pos != m_aliases.end();
> ++pos )
> {
> delete *pos;
> }
> }
What you did looks pretty much like using some object after it has
been deleted. Let me try to explain what happended, how to interpret
the valgrind messages, and why it is important (well, basically
because it helps explaining what happened)
==11257== Address 0x1C878270 is 0 bytes inside a block of size 32 free'd
This tells us what valgrind knows bout the memory that was accessed.
So it know, this memory was once a valid object from the heap, that
was deleted using the delete operator. This we know because of:
==11257== at 0x1B900807: operator delete(void*)
==11257== by 0x8057E14: guitar::ChordName::~ChordName() (chordname.cpp:40)
This is basically how things work in C++. The dtor is run until its
end (so line 40) and then the code for the real deallocation is done
(so operator delete).
Asuming that
==11257== at 0x804D2F6: guitar::Chord::~Chord() (chord.cpp:32)
is delete *pos;
this means that you are deleting some Chord Object, which has a
pointer in its list which points to some other Chord object, which
has already been deleted. Why this is the case is somewhere in your
program. Let me give you some hints on your way through C++. Never
try to store pointers to objects which you get over functions, unless
you can ensure they are always created with new. If you get
references/pointers, copy the object into one you internally created
with new. Secondly then make sure you never put one of those pointers
twice into your container, or into some other objects container,
since of course it will try to delete it multiple times.
greets
Dennis
Carpe quod tibi datum est
|
|
From: Dennis L. <pla...@gm...> - 2005-11-29 13:18:51
|
At 08:21 29.11.2005, Stephen Torri wrote: >I think I see this where it can be in my code. The data structure allows >for multiple chord names for the same chord. So a Chord X can have a >name of "E Major" or "F flat". Well I thought that calling delete on a >already deleted pointer was fine. No, its a no-no. You may not access memory you have deleted, thats why you delete it. (and calling delete invokes the dtor which accesses the memory) >My thinking wonders does the Container c own the pointer so that its >responsible to delete it or is it the responsibility of the caller to >delete it? > >Is a delete required in this code at the end of main if the Container >class would provide a destructor? Sure the data type stored in class A >is simple. > >Unfortunately this project requires me not to add any new dependencies >otherwise I would use boost shared_ptr class and not have to worry about >ownership. Once the object was out of scope it would automatically be >deleted. Can I do that here with them by adding a reference counting to >each class that I use as a pointer? If it is ok for you that putting the pointer into the container is giving up ownership on the object, and let the container own the object, then do it, document it and look that this principle is always obeyed. Its not bad to do so, only bad thing about enforcing certain ownerships is when you break the rules and/or have unspecified cases where you one time choose this and the other that. Weeks of coding can save hours of planning... greets Dennis Carpe quod tibi datum est |