FreeOrion

Forums for the FreeOrion project
It is currently Tue Oct 24, 2017 11:18 am

All times are UTC




Post new topic Reply to topic  [ 5 posts ] 
Author Message
PostPosted: Fri Jan 02, 2009 10:32 pm 
Offline
Space Dragon
User avatar

Joined: Thu Oct 02, 2008 11:00 pm
Posts: 339
Location: Slovakia
I did source code analysis to FreeOrion with cppcheck and found this bugs:
Code:
[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 67 times
Top
 Profile  
 
PostPosted: Sat Jan 03, 2009 2:00 am 
Offline
Programming, Design, Admin
User avatar

Joined: Wed Oct 08, 2003 1:33 am
Posts: 12007
Location: Munich
OndrejR wrote:
I did source code analysis to FreeOrion with cppcheck and found this bugs:
Code:
[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.

Quote:
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:
==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:
        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...


Top
 Profile  
 
PostPosted: Sat Jan 03, 2009 9:50 am 
Offline
Space Dragon
User avatar

Joined: Thu Oct 02, 2008 11:00 pm
Posts: 339
Location: Slovakia
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.


Top
 Profile  
 
PostPosted: Sat Jan 10, 2009 1:27 am 
Offline
Space Floater

Joined: Tue Jan 29, 2008 6:25 am
Posts: 22
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.

_________________
CWF Freeware
Programmer's Diary
C - features in Finnish


Top
 Profile  
 
PostPosted: Sat Jan 10, 2009 3:36 am 
Offline
Programming Lead Emeritus
User avatar

Joined: Thu Jun 26, 2003 1:33 pm
Posts: 1092
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.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 5 posts ] 

All times are UTC


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
Powered by phpBB® Forum Software © phpBB Group