al_destroy_display and cleaning up static memory

no-reply@allegro.cc (Edgar Reynaldo) — Wed, 18 Nov 2015 15:47:43 +0000

I've been spelunking^[1] in allegro internals for a few hours now trying to debug my ManyMouse program. I finally figured out I'm accessing a display that has been destroyed already by means of al_get_current_display returning a destroyed display. Shouldn't the thread local state (tls->current_display) be reset (zeroed) when a display is destroyed? Or set to another display if one is available?

src/tls.c#SelectExpand
 378/* Function: al_get_current_display
 379 */
 380ALLEGRO_DISPLAY *al_get_current_display(void)
 381{
 382   thread_local_state *tls;
 383
 384   if ((tls = tls_get()) == NULL)
 385      return NULL;
 386   return tls->current_display;
 387}

Also, al_destroy_display does nothing to zero the memory of the display in use, so it just contains leftover garbage when you destroy it. Doesn't proper practice dictate zeroing memory when you aren't going to use it anymore? Or setting it to some kind of guard value?

display.c#SelectExpand
 139/* Function: al_destroy_display
 140 */
 141void al_destroy_display(ALLEGRO_DISPLAY *display)
 142{
 143   if (display) {
 144      /* This causes warnings and potential errors on Android because
 145       * it clears the context and Android needs this thread to have
 146       * the context bound in its destroy function and to destroy the
 147       * shader. Just skip this part on Android.
 148       */
 149#ifndef ALLEGRO_ANDROID
 150      ALLEGRO_BITMAP *bmp;
 151
 152      bmp = al_get_target_bitmap();
 153      if (bmp && _al_get_bitmap_display(bmp) == display)
 154         al_set_target_bitmap(NULL);
 155
 156      /* This can happen if we have a current display, but the target bitmap
 157       * was a memory bitmap.
 158       */
 159      if (display == al_get_current_display())
 160         _al_set_current_display_only(NULL);
 161#endif
 162
 163      al_destroy_shader(display->default_shader);
 164      display->default_shader = NULL;
 165
 166      ASSERT(display->vt);
 167      display->vt->destroy_display(display);
 168   }
 169}

I'm using Windows and D3D at the moment, so display->vt->destroy_display is currently set to d3d_destroy_display. However, free doesn't seem to be setting any guard pattern with MinGW 4.8.1 when free(display) is called (free is being called because the ALLEGRO_MEMORY_INTERFACE* mem is zero [because allegro never calls al_set_memory_interface anywhere, it is unused]).

memory.c#SelectExpand
 49/* Function: al_free_with_context
 50 */
 51void al_free_with_context(void *ptr,
 52   int line, const char *file, const char *func)
 53{
 54   if (mem)
 55      mem->mi_free(ptr, line, file, func);
 56   else
 57      free(ptr);
 58}

memory.h#SelectExpand
 43/* Function: al_free
 44 */
 45#define al_free(p) \
 46   (al_free_with_context((p), __LINE__, __FILE__, __func__)) 

d3d_disp.cpp#SelectExpand
 893static void d3d_destroy_display(ALLEGRO_DISPLAY *display)
 894{
 895   ALLEGRO_SYSTEM_WIN *system = (ALLEGRO_SYSTEM_WIN *)al_get_system_driver();
 896   ALLEGRO_DISPLAY_D3D *d3d_display = (ALLEGRO_DISPLAY_D3D *)display;
 897   ALLEGRO_DISPLAY *old_disp = al_get_current_display();
 898
 899   ALLEGRO_INFO("destroying display %p (current %p)\n", display, old_disp);
 900
 901   if (old_disp != display)
 902      _al_set_current_display_only(display);
 903
 904   if (system->mouse_grab_display == display)
 905      al_ungrab_mouse();
 906
 907   _al_win_destroy_display_icons(display);
 908
 909   d3d_destroy_display_internals(d3d_display);
 910
 911   _al_vector_free(&display->display_invalidated_callbacks);
 912   _al_vector_free(&display->display_validated_callbacks);
 913
 914   _al_vector_find_and_delete(&system->system.displays, &display);
 915
 916   if (system->system.displays._size <= 0) {
 917      ffw_set = false;
 918      already_fullscreen = false;
 919   }
 920
 921   if (d3d_display->es_inited) {
 922      _al_event_source_free(&display->es);
 923      d3d_display->es_inited = false;
 924   }
 925
 926   _al_vector_free(&display->bitmaps);
 927   _al_vector_free(&((ALLEGRO_DISPLAY_WIN*) display)->msg_callbacks);
 928
 929   if (old_disp != display)
 930      _al_set_current_display_only(old_disp);
 931
 932   al_free(display->vertex_cache);
 933   al_free(display);
 934}

References

http://dictionary.reference.com/browse/spelunking

no-reply@allegro.cc (Elias) — Wed, 18 Nov 2015 21:23:23 +0000

Edgar Reynaldo said:

Shouldn't the thread local state (tls->current_display) be reset (zeroed) when a display is destroyed? Or set to another display if one is available?

I think setting it to zero is what we should do.

But I already see this, so somehow you must manage to bypass that...

https://github.com/liballeg/allegro5/blob/5.1/src/win/d3d_disp.cpp#L2416

Quote:

Doesn't proper practice dictate zeroing memory when you aren't going to use it anymore?

No - the memory may be reclaimed by the next memory allocation in fact and so zeroing out would not do anything. MSVC I believe has a debug setting by which it writes a pattern into free memory and does not re-use it. valgrind does the same in Linux and it's very useful.

And you can always use al_set_memory_interface to do exactly that on your own, it's why that function exists.

no-reply@allegro.cc (Edgar Reynaldo) — Thu, 19 Nov 2015 01:26:05 +0000

Elias said:

I think setting it to zero is what we should do.

But I already see this, so somehow you must manage to bypass that...

https://github.com/liballeg/allegro5/blob/5.1/src/win/d3d_disp.cpp#L2416

The link you gave references _al_d3d_destroy_bitmap. Perhaps you meant something else?

Elias said:

I said:

Doesn't proper practice dictate zeroing memory when you aren't going to use it anymore?

And you can always use al_set_memory_interface to do exactly that on your own, it's why that function exists.

Oh, okay!! That's what that is for! I tried looking up bit patterns used by MinGW but I couldn't find anything on Google. I thought MinGW would fill memory freed by free with some special bit pattern. I see 0xffffffffbaadfood in places (uninitialized allocated memory). How would I 'not re-use' memory? Would I have to make my own memory pool and divvy that up when my malloc is called?

no-reply@allegro.cc (Elias) — Thu, 19 Nov 2015 02:02:21 +0000

Edgar Reynaldo said:

The link you gave references _al_d3d_destroy_bitmap. Perhaps you meant something else?

Oh, yes, I thought the target bitmap is invalid - didn't realize we're talking about the display itself getting destroyed. So how can the current display end up pointing to a destroyed one? Only way I can see is if two threads access it at the same time - which is just not supported.

no-reply@allegro.cc (Edgar Reynaldo) — Wed, 02 Dec 2015 09:39:31 +0000

You may be right, but I'm not sure. I have two displays created on the main thread, and then all the mouse displays are created on the main display's window process thread that allegro creates in the window callback. Each mouse handles its own drawing on its window process thread that allegro created for the display in its own window process callback function. Mice are told to draw from the main displays callback process through PostMessage, which sends a message to the mouse's HWND, and drawing is taken care of by drawing from a DIB to the HWND of the mouse.

I really don't know what is causing the crash, and like I said I can only debug it on Vista. I'll try to get more info tonight, and see whether it is the backbuffer that is the bitmap that has been destroyed like you asked.