Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old March 28, 2016, 12:53   #1
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Using mtrace - z-virt is considered harmful

So I decided to run mtrace on Angband to see if any of my sound module code leaked memory.

Turns out Angband leaks like a sieve, but because of z-virt, it is impossible to figure out where the leaks are. mtrace tells me all the unfreed memory was allocated in z-virt.c. Even inlining the mem_*() functions didn't help (now there all allocated in z-virt.h)

So I macro'd mem_alloc to malloc etc, etc. Angband crashed immediately

So z-virt is harmfull for two reasons:
  1. It makes it impossible to use mtrace, and;
  2. It's obviously providing cover for some poor coding practices

I know checking for NULL after malloc/zalloc is a PITA (there are ~400 instances in the source), but it would allow better memory debugging using standard tools.
calris is offline   Reply With Quote
Old March 28, 2016, 13:28   #2
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Speaking of bad programming practices, I came across this gem in cave.c:

Code:
c->feat_count = mem_zalloc((z_info->f_max + 1) * sizeof(int));
Sure, c->feat_count is currently in int and is unlikely to ever not be, but if there is a lot more of this in the code, it might cause a few headaches
calris is offline   Reply With Quote
Old March 28, 2016, 14:46   #3
Therem Harth
Knight
 
Therem Harth's Avatar
 
Join Date: Jan 2008
Location: https://gitlab.com/miramor
Posts: 894
Therem Harth is on a distinguished road
Hmm. A thought, maybe off target...

Perhaps Angband could eventually offload its memory allocation and data structure stuff to a common third-party library, such as APR or GLib?

I know that extra dependencies are not desirable, but both of the above are fairly small.
Therem Harth is offline   Reply With Quote
Old March 28, 2016, 15:09   #4
redlumf
Scout
 
Join Date: Aug 2015
Posts: 25
redlumf is on a distinguished road
I've tried valgrind (on 4.0.1)

Code:
valgrind --tool=memcheck --leak-check=full ./angband -mgcu
Found two errors.
Code:
=3920== 40 bytes in 1 blocks are definitely lost in loss record 27 of 95
==3920==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3920==    by 0x4E07F8: mem_alloc (z-virt.c:41)
==3920==    by 0x4E0861: mem_zalloc (z-virt.c:54)
==3920==    by 0x4DA58D: dice_new (z-dice.c:197)
==3920==    by 0x438F3D: parse_class_dice (init.c:3527)
==3920==    by 0x47AF54: parser_parse (parser.c:335)
==3920==    by 0x47B8D6: parse_file (parser.c:671)
==3920==    by 0x43947B: run_parse_class (init.c:3636)
==3920==    by 0x47B72E: run_parser (parser.c:629)
==3920==    by 0x43ACDD: init_arrays (init.c:4365)
==3920==    by 0x43AD9D: init_angband (init.c:4441)
==3920==    by 0x4E1635: main (main.c:546)
==3920== 
==3920== 48 bytes in 3 blocks are definitely lost in loss record 44 of 95
==3920==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3920==    by 0x4E07F8: mem_alloc (z-virt.c:41)
==3920==    by 0x4E0988: string_make (z-virt.c:98)
==3920==    by 0x49611F: parse_buy_flag (store.c:261)
==3920==    by 0x47AF54: parser_parse (parser.c:335)
==3920==    by 0x47B8D6: parse_file (parser.c:671)
==3920==    by 0x49628D: run_parse_stores (store.c:287)
==3920==    by 0x47B72E: run_parser (parser.c:629)
==3920==    by 0x49637F: store_init (store.c:334)
==3920==    by 0x43AD9D: init_angband (init.c:4441)
==3920==    by 0x4E1635: main (main.c:546)
==3920== 
==3920== LEAK SUMMARY:
==3920==    definitely lost: 88 bytes in 4 blocks
==3920==    indirectly lost: 0 bytes in 0 blocks
==3920==      possibly lost: 0 bytes in 0 blocks
==3920==    still reachable: 296,656 bytes in 392 blocks
==3920==         suppressed: 0 bytes in 0 blocks
redlumf is offline   Reply With Quote
Old March 28, 2016, 15:12   #5
takkaria
Veteran
 
takkaria's Avatar
 
Join Date: Apr 2007
Posts: 1,858
Donated: $40
takkaria is on a distinguished road
Quote:
Originally Posted by calris View Post
So I decided to run mtrace on Angband to see if any of my sound module code leaked memory.

Turns out Angband leaks like a sieve, but because of z-virt, it is impossible to figure out where the leaks are. mtrace tells me all the unfreed memory was allocated in z-virt.c. Even inlining the mem_*() functions didn't help (now there all allocated in z-virt.h)

So I macro'd mem_alloc to malloc etc, etc. Angband crashed immediately
That's interesting - I can't reproduce this. It would be good to know what's causing these crashes. I used the following macros: (on OS X)

Code:
#define mem_alloc malloc
#define mem_free free
#define mem_realloc realloc
#define mem_zalloc(x) calloc(1, x)
__________________
"Physician, heal thyself."

Last edited by takkaria; March 28, 2016 at 15:18.
takkaria is offline   Reply With Quote
Old March 28, 2016, 15:42   #6
AnonymousHero
Veteran
 
AnonymousHero's Avatar
 
Join Date: Jun 2007
Posts: 1,365
AnonymousHero is on a distinguished road
Quote:
Originally Posted by calris View Post
I know checking for NULL after malloc/zalloc is a PITA (there are ~400 instances in the source), but it would allow better memory debugging using standard tools.
It should be possible to define wrapper macros for some of this. (Macros aren't great either, but sometimes they're better than tons of boilerplate... not that I'd necessarily consider 400 instances of null checks disastrous. Angband is... what, 100k+ lines?)

Btw, you might want to try compiling with ASAN (see "-fsantize=address" in the GCC/Clang options) instead. Perhaps that works better. (I know that it works pretty damn well on ToME 2.x. If you have debug information I think it even gives a full stack trace for any leaks.). IIRC you'll have to fiddle a bit with the makefiles to have them use "$CC" to link rather than "$LD", but other than that it should be straightforward.

EDIT: ASAN is also way ahead of valgrind, at the very least in terms of performance. It's actually feasible to just always compile with ASAN. (Not just in debug mode.)
AnonymousHero is offline   Reply With Quote
Old March 29, 2016, 00:41   #7
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by takkaria View Post
That's interesting - I can't reproduce this. It would be good to know what's causing these crashes. I used the following macros: (on OS X)

Code:
#define mem_alloc malloc
#define mem_free free
#define mem_realloc realloc
#define mem_zalloc(x) calloc(1, x)
I just gave it another go and it works now. Not sure what I did before
calris is offline   Reply With Quote
Old March 29, 2016, 02:12   #8
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
I added -fsanitize=address and -g3 (for good measure) to CFLAGS and -lasan into LIBS (hacked buildsys.mk - need to find a permanent place to put it)

I've reverted back to the mem_*() functions

Angband is VERY playable - I couldn't notice a speed penalty

I think mtrace() is a bit too excitable - muntrace() is called before the end of main() so it picks up everything the C library has internally allocated. ASAN is much fairer - Angband doesn't leak nearly as much as I thought.

Here's what I've found so far:

- There are some resource leaks in x11 - I can fix most of them, but a couple elude me.

- SDL Sound leaks 64 bytes, but it looks like this is buried in the SDL library (SDL_AllocRW)

- Existing character, starting in town, quit immediately:
  • parse_buy_flag() - store.c:262
  • parse_class_dice() - z-dice.c:197

- Existing character, starting in town, descend, search dungeon, learn a new spell, return to town:
  • parse_buy_flag() - store.c:262
  • parse_class_dice() - z-dice.c:197
  • spell_collect_from_book() - player-spell.c:199
  • object_new() - obj-pile.c:200
  • cmd_set_arg_string() - cmd-core.c:492

- Starting a new character
  • parse_buy_flag() - store.c:262
  • parse_class_dice() - z-dice.c:197
  • object_new() - obj-pile.c:200
  • rd_player() - load.c:676
  • player_quests_reset() - player-quest.c:163
  • player_spells_init() - player-spell.c:135
  • rd_item() - load.c:101
calris is offline   Reply With Quote
Old March 29, 2016, 03:26   #9
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by Therem Harth View Post
Hmm. A thought, maybe off target...

Perhaps Angband could eventually offload its memory allocation and data structure stuff to a common third-party library, such as APR or GLib?

I know that extra dependencies are not desirable, but both of the above are fairly small.
Abstracting memory allocation and deallocation makes debugging harder, not easier. Most debugging tools (like ASAN for example), rely on overloading malloc and free. If these get buried in a library, they can become impossible to trace.

For example, here's what ASAN reports about dice_new() leakage:
Code:
Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0x7f43b19bb96a in malloc (/lib64/libasan.so.2+0x9896a)
    #1 0x62c3d8 in mem_alloc /home/gruss/Angband_devel/angband/src/z-virt.c:41
    #2 0x62c461 in mem_zalloc /home/gruss/Angband_devel/angband/src/z-virt.c:54
    #3 0x61d50c in dice_new /home/gruss/Angband_devel/angband/src/z-dice.c:197
    #4 0x48c550 in parse_class_dice /home/gruss/Angband_devel/angband/src/init.c:3573
    #5 0x52bf9e in parser_parse /home/gruss/Angband_devel/angband/src/parser.c:335
    #6 0x52d77d in parse_file /home/gruss/Angband_devel/angband/src/parser.c:671
    #7 0x48cd31 in run_parse_class /home/gruss/Angband_devel/angband/src/init.c:3682
    #8 0x52d508 in run_parser /home/gruss/Angband_devel/angband/src/parser.c:629
    #9 0x48fcc2 in init_arrays /home/gruss/Angband_devel/angband/src/init.c:4412
    #10 0x48fe50 in init_angband /home/gruss/Angband_devel/angband/src/init.c:4488
    #11 0x62dde3 in main /home/gruss/Angband_devel/angband/src/main.c:531
    #12 0x7f43affb557f in __libc_start_main (/lib64/libc.so.6+0x2057f)
And here's what it tells me about the SDL leak:

Code:
Direct leak of 64 byte(s) in 1 object(s) allocated from:
    #0 0x7f43b19bb96a in malloc (/lib64/libasan.so.2+0x9896a)
    #1 0x7f43b0ad443d in SDL_AllocRW (/lib64/libSDL-1.2.so.0+0x1043d)
calris is offline   Reply With Quote
Old March 29, 2016, 12:21   #10
Therem Harth
Knight
 
Therem Harth's Avatar
 
Join Date: Jan 2008
Location: https://gitlab.com/miramor
Posts: 894
Therem Harth is on a distinguished road
Quote:
Originally Posted by calris View Post
Abstracting memory allocation and deallocation makes debugging harder, not easier. Most debugging tools (like ASAN for example), rely on overloading malloc and free. If these get buried in a library, they can become impossible to trace.
Huh. Thanks, that is useful to know.

Although, I thought that LD_PRELOAD libraries took precedence over other shared libs, and would therefore override in those too?

For instance, with sudo's NOEXEC functionality, the library sudo_noexec.so overloads execv(), execl(), and related C library functions. This works even for programs that offload process execution to a shared library.

Is LD_PRELOAD not being used here, or is there something else I'm missing?

Edit: oh N/M, I see what you're saying. Though I'm surprised it doesn't show the full stack trace.
Therem Harth is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
DUNGEON_HGT considered harmful seebs Development 2 May 17, 2013 22:35
what is considered a clean win? pamperedpeterson Vanilla 56 January 29, 2008 08:02


All times are GMT +1. The time now is 22:24.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2018, vBulletin Solutions Inc.