Allegro.cc - Online Community

Allegro.cc Forums » Allegro Development » Crash in example(s?) ; ex_threads,

This thread is locked; no one can reply to it. rss feed Print
Crash in example(s?) ; ex_threads,
Edgar Reynaldo
Member #8,592
May 2007
avatar

Hey guys,

I recently compiled A5.2 from GIT and when I run ex_threads.exe it crashes. Here's the log and backtrace :

#SelectExpand
1c:\mingw\LIBS\A5GITdistro\bin>gdb ex_threads.exe 2GNU gdb (GDB) 7.6.1 3Copyright (C) 2013 Free Software Foundation, Inc. 4License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> 5This is free software: you are free to change and redistribute it. 6There is NO WARRANTY, to the extent permitted by law. Type "show copying" 7and "show warranty" for details. 8This GDB was configured as "mingw32". 9For bug reporting instructions, please see: 10<http://www.gnu.org/software/gdb/bugs/>... 11Reading symbols from c:\mingw\LIBS\A5GITdistro\bin\ex_threads.exe...done. 12(gdb) run 13Starting program: c:\mingw\LIBS\A5GITdistro\bin/ex_threads.exe 14[New Thread 10184.0x208c] 15[New Thread 10184.0x25a8] 16[New Thread 10184.0x11e4] 17[New Thread 10184.0x2014] 18[New Thread 10184.0xe60] 19[New Thread 10184.0x254c] 20[New Thread 10184.0x22e0] 21[New Thread 10184.0x1aac] 22warning: HEAP[ex_threads.exe]: 23warning: Heap block at 00696D60 modified at 00696D78 past requested size of 10 24 25 26Program received signal SIGTRAP, Trace/breakpoint trap. 27[Switching to Thread 10184.0x254c] 280x77a8ee8b in ?? () 29(gdb) bt 30#0 0x77a8ee8b in ?? () 31#1 0x77a3cc8f in ?? () 32#2 0x77a8e598 in ?? () 33#3 0x779ea563 in ?? () 34#4 0x779ea119 in ?? () 35#5 0x779ea043 in ?? () 36#6 0x75047d39 in realloc () from C:\WINDOWS\SysWOW64\msvcrt.dll 37#7 0x00690000 in ?? () 38#8 0x675af435 in al_realloc_with_context (ptr=0x696d68, n=48, line=188, file=0x677621ac <__func__.5627+28> "C:\\mingw\\LIBS\\A5GIT\\allegro5\\src\\misc\\vector.c", 39 func=0x677622af <__func__.5372> "_al_vector_alloc_back") at C:\mingw\LIBS\A5GIT\allegro5\src\memory.c:70 40#9 0x67617e69 in _al_vector_alloc_back (vec=0x677ded00 <eds_list>) at C:\mingw\LIBS\A5GIT\allegro5\src\misc\vector.c:188 41#10 0x67633910 in _al_d3d_generate_display_format_list () at C:\mingw\LIBS\A5GIT\allegro5\src\win\d3d_display_formats.cpp:97 42#11 0x6762f22b in d3d_create_display_internals (d3d_display=0x6970a0, free_on_error=true) at C:\mingw\LIBS\A5GIT\allegro5\src\win\d3d_disp.cpp:1640 43#12 0x6762fb08 in d3d_create_display_locked (w=300, h=300) at C:\mingw\LIBS\A5GIT\allegro5\src\win\d3d_disp.cpp:1774 44#13 0x6762fe1e in d3d_create_display (w=300, h=300) at C:\mingw\LIBS\A5GIT\allegro5\src\win\d3d_disp.cpp:1836 45#14 0x6759f870 in al_create_display (w=300, h=300) at C:\mingw\LIBS\A5GIT\allegro5\src\display.c:53 46#15 0x00401c97 in thread_func (thr=0x695480, arg=0x63fc60) at C:\mingw\LIBS\A5GIT\allegro5\examples\ex_threads.c:116 47#16 0x675b4480 in thread_func_trampoline (inner=0x695480, _outer=0x695480) at C:\mingw\LIBS\A5GIT\allegro5\src\threads.c:80 48#17 0x67627618 in thread_proc_trampoline (data=0x695480) at C:\mingw\LIBS\A5GIT\allegro5\src\win\wxthread.c:33 49#18 0x750671e6 in msvcrt!_beginthreadex () from C:\WINDOWS\SysWOW64\msvcrt.dll 50#19 0x750672b1 in msvcrt!_endthreadex () from C:\WINDOWS\SysWOW64\msvcrt.dll 51#20 0x748438f4 in KERNEL32!BaseThreadInitThunk () from C:\WINDOWS\SysWOW64\kernel32.dll 52#21 0x77a15de3 in ?? () 53#22 0x77a15dae in ?? () 54#23 0x00000000 in ?? () 55(gdb) frame 10 56#10 0x67633910 in _al_d3d_generate_display_format_list () at C:\mingw\LIBS\A5GIT\allegro5\src\win\d3d_display_formats.cpp:97 5797 peds = (ALLEGRO_EXTRA_DISPLAY_SETTINGS **)_al_vector_alloc_back(&eds_list); 58(gdb)

I'm investigating this but I haven't found the cause yet. I looked at _al_d3d_generate_display_format_list and _al_vector_alloc_back but so far I haven't really found anything obvious.

From the addresses and the warnings, it looks like it is trying to write past the allocated memory for eds_list->_items in frame #10.

What's weird is that it is crashing in realloc. Can't explain that one.

EDIT
Okay, so I've been doing some debugging, and I've been looking at _al_d3d_generate_display_format_list and I'm getting some odd values that I can't explain. On line 90 of d3d_display_formats.cpp, a triply nested loop appears. When it crashes, i, j, and k are all zero, which should indicate it is the first time through each loop. The weird thing is though, that eds_list._size is already 3 and eds_list._unused is already 1, which would indicate al_vector_alloc_back has been called three times, as well as realloc. How can a single line of code in the third level of the loop have been called 3 times when each loop index is zero? It makes no sense. That's as far as I've gotten debugging this.

Elias
Member #358
May 2000

ex_threads creates several windows, so that loop is executed several times in parallel - but I didn't actually look at the code.

--
"Either help out or stop whining" - Evert

Edgar Reynaldo
Member #8,592
May 2007
avatar

I think then that the problem is this variable :

d3d_display_formats.cpp#SelectExpand
35static _AL_VECTOR eds_list;

Since eds_list is not part of TLS it gets shared among the threads in ex_threads.cpp, which are creating displays in parallel. So eds_list's contents get reallocated in one thread while another thread has an old memory address for the vector's contents and then it accesses memory that is no longer valid.

In addition, there are two other static variables in the _al_d3d_generate_display_format_list function. I'm not sure what the comment "stop [the] warning" means or what it is for. It looks like they are there to prevent the function from re-generating the display format list if it has already been created and the adapter and fullscreen variables match the new display options.

d3d_display_formats.cpp#SelectExpand
47void _al_d3d_generate_display_format_list(void) 48{
49 static bool fullscreen = !(al_get_new_display_flags() & ALLEGRO_FULLSCREEN); /* stop warning */
50 static int adapter = ~al_get_new_display_adapter(); /* stop warning */
51 int i; 52 53 if (!_al_vector_is_empty(&eds_list) && (fullscreen == (bool)(al_get_new_display_flags() & ALLEGRO_FULLSCREEN)) 54 && (adapter == al_get_new_display_adapter())) { 55 return; 56 } 57 else if (!_al_vector_is_empty(&eds_list)) { 58 _al_d3d_destroy_display_format_list(); 59 } 60 61 fullscreen = (al_get_new_display_flags() & ALLEGRO_FULLSCREEN) != 0; 62 adapter = al_get_new_display_adapter(); 63 if (adapter < 0) 64 adapter = 0; 65 66 _al_vector_init(&eds_list, sizeof(ALLEGRO_EXTRA_DISPLAY_SETTINGS *)); 67 68 /* Loop through each bit combination of: 69 * bit 0: 16/32 bit 70 * bit 1: single-buffer 71 * bit 2: vsync 72 */ 73 for (i = 0; i < 8; i++) { 74 int format_num = !!(i & 1); 75 int single_buffer = !!(i & 2); 76 int vsync = !!(i & 4); 77 int allegro_format = ALLEGRO_PIXEL_FORMAT_XRGB_8888; 78 if (format_num == 1) allegro_format = ALLEGRO_PIXEL_FORMAT_RGB_565; 79 D3DFORMAT d3d_format = (D3DFORMAT)_al_pixel_format_to_d3d(allegro_format); 80 81 /* Count available multisample quality levels. */ 82 DWORD quality_levels = 0; 83 84 if (_al_d3d->CheckDeviceMultiSampleType(adapter, D3DDEVTYPE_HAL, d3d_format, 85 !fullscreen, D3DMULTISAMPLE_NONMASKABLE, &quality_levels) != D3D_OK) { 86 _al_d3d->CheckDeviceMultiSampleType(adapter, D3DDEVTYPE_REF, d3d_format, 87 !fullscreen, D3DMULTISAMPLE_NONMASKABLE, &quality_levels); 88 } 89 90 /* Loop through available depth/stencil formats. */ 91 for (int j = 0; j < D3D_DEPTH_FORMATS; j++) { 92 if (j == 0 || IsDepthFormatExisting( 93 depth_stencil_formats[j].format, d3d_format)) { 94 DEPTH_STENCIL_DESC *ds = depth_stencil_formats + j; 95 for (int k = 0; k < (int)quality_levels + 1; k++) { 96 ALLEGRO_EXTRA_DISPLAY_SETTINGS *eds, **peds; 97 peds = (ALLEGRO_EXTRA_DISPLAY_SETTINGS **)_al_vector_alloc_back(&eds_list); 98 eds = *peds = (ALLEGRO_EXTRA_DISPLAY_SETTINGS *)al_malloc(sizeof *eds); 99 memset(eds->settings, 0, sizeof(int) * ALLEGRO_DISPLAY_OPTIONS_COUNT); 100 101 eds->settings[ALLEGRO_COMPATIBLE_DISPLAY] = 1; 102 103 if (format_num == 0) { 104 eds->settings[ALLEGRO_RED_SIZE] = 8; 105 eds->settings[ALLEGRO_GREEN_SIZE] = 8; 106 eds->settings[ALLEGRO_BLUE_SIZE] = 8; 107 eds->settings[ALLEGRO_RED_SHIFT] = 16; 108 eds->settings[ALLEGRO_GREEN_SHIFT] = 8; 109 eds->settings[ALLEGRO_BLUE_SHIFT] = 0; 110 eds->settings[ALLEGRO_COLOR_SIZE] = 32; 111 } 112 else if (format_num == 1) { 113 eds->settings[ALLEGRO_RED_SIZE] = 5; 114 eds->settings[ALLEGRO_GREEN_SIZE] = 6; 115 eds->settings[ALLEGRO_BLUE_SIZE] = 5; 116 eds->settings[ALLEGRO_RED_SHIFT] = 11; 117 eds->settings[ALLEGRO_GREEN_SHIFT] = 5; 118 eds->settings[ALLEGRO_BLUE_SHIFT] = 0; 119 eds->settings[ALLEGRO_COLOR_SIZE] = 16; 120 } 121 122 if (single_buffer) { 123 eds->settings[ALLEGRO_SINGLE_BUFFER] = 1; 124 eds->settings[ALLEGRO_UPDATE_DISPLAY_REGION] = 1; 125 } 126 127 if (vsync) { 128 eds->settings[ALLEGRO_VSYNC] = 1; 129 } 130 131 eds->settings[ALLEGRO_DEPTH_SIZE] = ds->d; 132 eds->settings[ALLEGRO_STENCIL_SIZE] = ds->s; 133 134 if (k > 1) { 135 eds->settings[ALLEGRO_SAMPLE_BUFFERS] = 1; 136 // TODO: Is it ok to use the quality level here? 137 eds->settings[ALLEGRO_SAMPLES] = k; 138 } 139 } 140 } 141 } 142 143 } 144 ALLEGRO_INFO("found %d format combinations\n", _al_vector_size(&eds_list)); 145}

So I think that if eds_list, fullscreen, and adapter are moved into TLS then ex_threads will be able to run safely, since then each thread will have its own display settings list.

Elias
Member #358
May 2000

Or simply not make them global, thread local or not?

I haven't looked at the code though so maybe there is a reason those are global - but my first try at fixing it would be to simply not make those variables global.

--
"Either help out or stop whining" - Evert

Edgar Reynaldo
Member #8,592
May 2007
avatar

fullscreen and adapter need to persist between function calls, as well as the eds_list. There should be one instance per thread, because each time you create a display they change, and a single thread can have multiple displays.

Edit
Well I guess technically that's not true. You could recreate the list each time. However, I think the point was to re-use it.

Edit2
Actually, I don't know. Multiple functions work on the same data, but that could be solved by passing a reference. It would force you to fully re-create the display list each time you create a display though. I don't know how expensive that is. The list only needs to be re-created when fullscreen or adapter change.

Edit3 04/28/2016 1:00AM
Do you guys want me to work on a patch for this? Right now I'm leaning towards moving eds_list, fullscreen, and adapter into TLS. Or else we could just recreate the list every time _al_d3d_generate_display_format_list is called. Thoughts? I'm willing to work on this if you want. Otherwise, ex_threads is broken on Direct3D.

Elias
Member #358
May 2000

I would say re-create the list every time - but I haven't looked at the code. My gut feeling just is that it's always bad to keep stuff around in some global variable - and this is not something that should take a long time nor something that will happen very often, so I'd say re-create it when needed.

If you make the patch to just move it to TLS that probably also is acceptable though :)

--
"Either help out or stop whining" - Evert

Edgar Reynaldo
Member #8,592
May 2007
avatar

I think I will add it to TLS. I'll do it here in the next few days or so.

Edit
Okay, I'm in the middle of adding it to TLS. How should I interface with the thread local storage though? tls_get is not accessible outside of tls.c. Should I make accessor functions? Or can I export tls_get somehow? It seems silly to have three accessor functions just to access three variables in tls.

Go to: