FreeOrion bug hunt 2009 #1

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
User avatar
OndrejR
Space Dragon
Posts: 339
Joined: Thu Oct 02, 2008 11:00 pm
Location: Slovakia

FreeOrion bug hunt 2009 #1

#1 Post by OndrejR »

I did source code analysis to FreeOrion with cppcheck and found this bugs:

Code: Select all

[network/Message.cpp:121] Uninitialized member variable 'Message::m_synchronous_response'
[server/ServerApp.cpp:69] Uninitialized member variable 'ServerApp::m_single_player_game'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_design_id'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_object_id'
I also did code analysis with Valgrind and found a lot of bugs(attachment valgrind.zip)
Attachments
valgrind.zip
(30.24 KiB) Downloaded 133 times

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: FreeOrion bug hunt 2009 #1

#2 Post by Geoff the Medio »

OndrejR wrote:I did source code analysis to FreeOrion with cppcheck and found this bugs:

Code: Select all

[network/Message.cpp:121] Uninitialized member variable 'Message::m_synchronous_response'
[server/ServerApp.cpp:69] Uninitialized member variable 'ServerApp::m_single_player_game'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_design_id'
[universe/Universe.cpp:348] Uninitialized member variable 'Universe::m_last_allocated_object_id'
None of those are really "bugs" since all those variables (should) get initialized later before they're used. For example, the Universe last allocated ids are set to -1 in Universe::CreateUniverse or are set when deserializing the universe that's sent from the server or loaded from disk. I added appropriate initializations for them anyway, though.
I also did code analysis with Valgrind and found a lot of bugs(attachment valgrind.zip)
There are a bunch of font bugs that might interest tzlaine, but a lot of it is boost internal stuff or openal internal stuff that I doubt we can do anything about. I'm also skeptical of the accuracy... For example, there is this bug listing...

Code: Select all

==10554== 15,768 (792 direct, 14,976 indirect) bytes in 9 blocks are definitely lost in loss record 752 of 1,257
==10554==    at 0x4C21FCC: operator new(unsigned long) (vg_replace_malloc.c:230)
==10554==    by 0x520BCF: __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::allocate(unsigned long, void const*) (new_allocator.h:92)
==10554==    by 0x520BF3: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_get_node() (stl_tree.h:357)
==10554==    by 0x525C9F: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_create_node(std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:366)
==10554==    by 0x52D8C4: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_insert_(std::_Rb_tree_node_base const*, std::_Rb_tree_node_base const*, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:852)
==10554==    by 0x52DA29: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_insert_unique(std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:1148)
==10554==    by 0x52DC51: std::_Rb_tree<std::set<int, std::less<int>, std::allocator<int> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double>, std::_Select1st<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::_M_insert_unique_(std::_Rb_tree_const_iterator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_tree.h:1188)
==10554==    by 0x52DFF7: std::map<std::set<int, std::less<int>, std::allocator<int> >, double, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::insert(std::_Rb_tree_iterator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> >, std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> const&) (stl_map.h:496)
==10554==    by 0x52E0AA: std::map<std::set<int, std::less<int>, std::allocator<int> >, double, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::pair<std::set<int, std::less<int>, std::allocator<int> > const, double> > >::operator[](std::set<int, std::less<int>, std::allocator<int> > const&) (stl_map.h:419)
==10554==    by 0x53916D: ResourcePool::SetSystemSupplyGroups(std::set<std::set<int, std::less<int>, std::allocator<int> >, std::less<std::set<int, std::less<int>, std::allocator<int> > >, std::allocator<std::set<int, std::less<int>, std::allocator<int> > > > const&) (ResourcePool.cpp:177)
==10554==    by 0x4F79F6: Empire::InitResourcePools() (Empire.cpp:2376)
==10554==    by 0x9ABDE1: MapWnd::InitTurn(int) (MapWnd.cpp:713)
Which refers to this line of ResourcePool::SetSystemSupplyGroups:

Code: Select all

        m_supply_system_groups_resource_production[*it] = 0.0;
were it is a std::set<std::set<int> >::const_iterator. So apparently assigning a value to an possibly creating a new entry in a std::map<std::set<int>, double> causes a loss of memory? m_supply_system_groups_resource_production is cleared earlier in the function, so I don't see what is wrong with this code...

User avatar
OndrejR
Space Dragon
Posts: 339
Joined: Thu Oct 02, 2008 11:00 pm
Location: Slovakia

Re: FreeOrion bug hunt 2009 #1

#3 Post by OndrejR »

Geoff the Medio wrote:but a lot of it is boost internal stuff or openal internal stuff that I doubt we can do anything about
I can at least found that this bug exists, install debug boost and openal libraries and report this bug to boost and openal forum.
Geoff the Medio wrote:I'm also skeptical of the accuracy... For example, there is this bug listing...
Valgrind errors are not always really errors. But error rate should be high and if it is really bug we need to solve.

Maz
Space Floater
Posts: 22
Joined: Tue Jan 29, 2008 6:25 am

Re: FreeOrion bug hunt 2009 #1

#4 Post by Maz »

Valgrind indeed is great tool. But there are certain cases when it does not detect memory issues correctly (or at least there was ... some (longish) time ago). One such situation is when shared memory is reserved / initialized by one process, and another process then uses / frees it (As far as I remember).

Also I do not know what kind of cleanup procedures you do when one exits the game, but often this part is overlooked (well, OS releases the memory when process ends anyways). However valgrind thinks the memory has leaked (as it technically speaking has - although with no other real meaning but showing up as an error in valgrind.

However valgrind reports can (and should) be used (in my humble opinion). Especially invalid writes and reads should all be analyzed carefully.

tzlaine
Programming Lead Emeritus
Posts: 1092
Joined: Thu Jun 26, 2003 1:33 pm

Re: FreeOrion bug hunt 2009 #1

#5 Post by tzlaine »

I regularly use Valgrind, and the recent work reported against the "Memory Leaks?" SourceForge bug is directly attributable to Valgrind reports. However, it irritatingly reports many std::string as "possibly lost", due to the COW implementation. It makes looking for legit leaks pretty difficult.

Post Reply