diff --git a/include/SDL3/SDL_events.h b/include/SDL3/SDL_events.h index 8460609e92..63958e7e44 100644 --- a/include/SDL3/SDL_events.h +++ b/include/SDL3/SDL_events.h @@ -1431,7 +1431,7 @@ extern SDL_DECLSPEC void * SDLCALL SDL_AllocateEventMemory(size_t size); * This function changes ownership of temporary event memory allocated for events and APIs that * follow the SDL_GetStringRule. If this function succeeds, the memory will no longer be automatically freed by SDL, it must be freed using SDL_free() by the application. * - * If the memory isn't temporary, or it was allocated on a different thread, this will return NULL, and the application does not have ownership of the memory. + * If the memory isn't temporary, or it was allocated on a different thread, or if it is associated with an event currently in the event queue, this will return NULL, and the application does not have ownership of the memory. * * Note that even if a function follows the SDL_GetStringRule it may not be using temporary event memory, and this function will return NULL in that case. * @@ -1443,6 +1443,7 @@ extern SDL_DECLSPEC void * SDLCALL SDL_AllocateEventMemory(size_t size); * \since This function is available since SDL 3.0.0. * * \sa SDL_AllocateEventMemory + * \sa SDL_free */ extern SDL_DECLSPEC void * SDLCALL SDL_ClaimEventMemory(const void *mem); @@ -1451,15 +1452,11 @@ extern SDL_DECLSPEC void * SDLCALL SDL_ClaimEventMemory(const void *mem); * * This function frees temporary memory allocated for events and APIs that * follow the SDL_GetStringRule. This memory is local to the thread that creates - * it and is automatically freed for the main thread when pumping the event - * loop. For other threads you may call this function periodically to + * it and is automatically freed for the main thread when processing events. + * For other threads you may call this function periodically to * free any temporary memory created by that thread. * - * You can free a specific pointer, to provide more fine grained control over memory management, or you can pass NULL to free all pending temporary allocations. You should *NOT* pass NULL on your main event handling thread, as there may be temporary memory being used by events in-flight. For that thread SDL will call this internally when it's safe to do so. - * - * Note that if you call SDL_AllocateEventMemory() on one thread and pass it - * to another thread, e.g. via a user event, then you should be sure the other - * thread has finished processing it before calling this function with NULL. + * You can free a specific pointer, to provide more fine grained control over memory management, or you can pass NULL to free all pending temporary allocations. * * All temporary memory is freed on the main thread in SDL_Quit() and for other threads when they call SDL_CleanupTLS(), which is automatically called at cleanup time for threads created using SDL_CreateThread(). * diff --git a/src/events/SDL_events.c b/src/events/SDL_events.c index 8b5923ad27..bc7f7950de 100644 --- a/src/events/SDL_events.c +++ b/src/events/SDL_events.c @@ -71,10 +71,25 @@ typedef struct static SDL_DisabledEventBlock *SDL_disabled_events[256]; static Uint32 SDL_userevents = SDL_EVENT_USER; -/* Private data -- event queue */ +typedef struct SDL_EventMemory +{ + void *memory; + struct SDL_EventMemory *prev; + struct SDL_EventMemory *next; +} SDL_EventMemory; + +typedef struct SDL_EventMemoryState +{ + SDL_EventMemory *head; + SDL_EventMemory *tail; +} SDL_EventMemoryState; + +static SDL_TLSID SDL_event_memory; + typedef struct SDL_EventEntry { SDL_Event event; + SDL_EventMemory *memory; struct SDL_EventEntry *prev; struct SDL_EventEntry *next; } SDL_EventEntry; @@ -90,22 +105,6 @@ static struct SDL_EventEntry *free; } SDL_EventQ = { NULL, SDL_FALSE, { 0 }, 0, NULL, NULL, NULL }; -typedef struct SDL_EventMemory -{ - Uint32 eventID; - void *memory; - struct SDL_EventMemory *next; -} SDL_EventMemory; - -typedef struct SDL_EventMemoryState -{ - SDL_EventMemory *head; - SDL_EventMemory *tail; -} SDL_EventMemoryState; - -static SDL_TLSID SDL_event_memory; - - static void SDL_CleanupEventMemory(void *data) { SDL_EventMemoryState *state = (SDL_EventMemoryState *)data; @@ -137,6 +136,130 @@ static SDL_EventMemoryState *SDL_GetEventMemoryState(SDL_bool create) return state; } +static SDL_EventMemory *SDL_GetEventMemoryEntry(SDL_EventMemoryState *state, const void *mem) +{ + SDL_EventMemory *entry; + + // Start from the end, it's likely to have been recently allocated + for (entry = state->tail; entry; entry = entry->prev) { + if (mem == entry->memory) { + return entry; + } + } + return NULL; +} + +static void SDL_LinkEventMemoryEntry(SDL_EventMemoryState *state, SDL_EventMemory *entry) +{ + entry->prev = state->tail; + entry->next = NULL; + + if (state->tail) { + state->tail->next = entry; + } else { + state->head = entry; + } + state->tail = entry; +} + +static void SDL_UnlinkEventMemoryEntry(SDL_EventMemoryState *state, SDL_EventMemory *entry) +{ + if (state->head == entry) { + state->head = entry->next; + } + if (state->tail == entry) { + state->tail = entry->prev; + } + + if (entry->prev) { + entry->prev->next = entry->next; + } + if (entry->next) { + entry->next->prev = entry->prev; + } + + entry->prev = NULL; + entry->next = NULL; +} + +static void SDL_FreeEventMemoryEntry(SDL_EventMemoryState *state, SDL_EventMemory *entry, SDL_bool free_data) +{ + if (free_data) { + SDL_free(entry->memory); + } + SDL_free(entry); +} + +static void SDL_LinkEventMemoryToEvent(SDL_EventEntry *event, const void *mem) +{ + SDL_EventMemoryState *state; + SDL_EventMemory *entry; + + state = SDL_GetEventMemoryState(SDL_FALSE); + if (!state) { + return; + } + + entry = SDL_GetEventMemoryEntry(state, mem); + if (entry) { + SDL_UnlinkEventMemoryEntry(state, entry); + entry->next = event->memory; + event->memory = entry; + } +} + +// Transfer the event memory from the thread-local event memory list to the event +static void SDL_TransferEventMemoryToEvent(SDL_EventEntry *event) +{ + switch (event->event.type) { + case SDL_EVENT_TEXT_EDITING: + SDL_LinkEventMemoryToEvent(event, event->event.edit.text); + break; + case SDL_EVENT_TEXT_EDITING_CANDIDATES: + SDL_LinkEventMemoryToEvent(event, event->event.edit_candidates.candidates); + break; + case SDL_EVENT_TEXT_INPUT: + SDL_LinkEventMemoryToEvent(event, event->event.text.text); + break; + case SDL_EVENT_DROP_BEGIN: + case SDL_EVENT_DROP_FILE: + case SDL_EVENT_DROP_TEXT: + case SDL_EVENT_DROP_COMPLETE: + case SDL_EVENT_DROP_POSITION: + SDL_LinkEventMemoryToEvent(event, event->event.drop.source); + SDL_LinkEventMemoryToEvent(event, event->event.drop.data); + break; + default: + if (event->event.type >= SDL_EVENT_USER && event->event.type <= SDL_EVENT_LAST-1) { + SDL_LinkEventMemoryToEvent(event, event->event.user.data1); + SDL_LinkEventMemoryToEvent(event, event->event.user.data2); + } + break; + } +} + +// Transfer the event memory from the event to the thread-local event memory list +static void SDL_TransferEventMemoryFromEvent(SDL_EventEntry *event) +{ + SDL_EventMemoryState *state; + SDL_EventMemory *entry, *next; + + if (!event->memory) { + return; + } + + state = SDL_GetEventMemoryState(SDL_TRUE); + if (!state) { + return; // this is now a leak, but you probably have bigger problems if malloc failed. + } + + for (entry = event->memory; entry; entry = next) { + next = entry->next; + SDL_LinkEventMemoryEntry(state, entry); + } + event->memory = NULL; +} + void *SDL_FreeLater(void *memory) { SDL_EventMemoryState *state; @@ -155,16 +278,9 @@ void *SDL_FreeLater(void *memory) return memory; // this is now a leak, but you probably have bigger problems if malloc failed. We could probably pool up and reuse entries, though. } - entry->eventID = SDL_last_event_id; entry->memory = memory; - entry->next = NULL; - if (state->tail) { - state->tail->next = entry; - } else { - state->head = entry; - } - state->tail = entry; + SDL_LinkEventMemoryEntry(state, entry); return memory; } @@ -182,58 +298,17 @@ const char *SDL_AllocateEventString(const char *string) return NULL; } -static void SDL_FlushEventMemory(Uint32 eventID) -{ - SDL_EventMemoryState *state; - - state = SDL_GetEventMemoryState(SDL_FALSE); - if (!state) { - return; - } - - if (state->head) { - while (state->head) { - SDL_EventMemory *entry = state->head; - - if (eventID && (Sint32)(eventID - entry->eventID) < 0) { - break; - } - - /* If you crash here, your application has memory corruption - * or freed memory in an event, which is no longer necessary. - */ - state->head = entry->next; - SDL_free(entry->memory); - SDL_free(entry); - } - if (!state->head) { - state->tail = NULL; - } - } -} - void *SDL_ClaimEventMemory(const void *mem) { SDL_EventMemoryState *state; state = SDL_GetEventMemoryState(SDL_FALSE); if (state && mem) { - SDL_EventMemory *prev = NULL, *entry; - - for (entry = state->head; entry; prev = entry, entry = entry->next) { - if (mem == entry->memory) { - if (prev) { - prev->next = entry->next; - } - if (entry == state->head) { - state->head = entry->next; - } - if (entry == state->tail) { - state->tail = prev; - } - SDL_free(entry); - return (void *)mem; - } + SDL_EventMemory *entry = SDL_GetEventMemoryEntry(state, mem); + if (entry) { + SDL_UnlinkEventMemoryEntry(state, entry); + SDL_FreeEventMemoryEntry(state, entry, SDL_FALSE); + return (void *)mem; } } return NULL; @@ -249,34 +324,17 @@ void SDL_FreeEventMemory(const void *mem) } if (mem) { - SDL_EventMemory *prev = NULL, *entry; - - for (entry = state->head; entry; prev = entry, entry = entry->next) { - if (mem == entry->memory) { - if (prev) { - prev->next = entry->next; - } - if (entry == state->head) { - state->head = entry->next; - } - if (entry == state->tail) { - state->tail = prev; - } - SDL_free(entry->memory); - SDL_free(entry); - break; - } + SDL_EventMemory *entry = SDL_GetEventMemoryEntry(state, mem); + if (entry) { + SDL_UnlinkEventMemoryEntry(state, entry); + SDL_FreeEventMemoryEntry(state, entry, SDL_TRUE); } } else { - if (state->head) { - while (state->head) { - SDL_EventMemory *entry = state->head; + while (state->head) { + SDL_EventMemory *entry = state->head; - state->head = entry->next; - SDL_free(entry->memory); - SDL_free(entry); - } - state->tail = NULL; + SDL_UnlinkEventMemoryEntry(state, entry); + SDL_FreeEventMemoryEntry(state, entry, SDL_TRUE); } } } @@ -784,6 +842,7 @@ void SDL_StopEventLoop(void) /* Clean out EventQ */ for (entry = SDL_EventQ.head; entry;) { SDL_EventEntry *next = entry->next; + SDL_TransferEventMemoryFromEvent(entry); SDL_free(entry); entry = next; } @@ -890,6 +949,8 @@ static int SDL_AddEvent(SDL_Event *event) if (event->type == SDL_EVENT_POLL_SENTINEL) { SDL_AtomicAdd(&SDL_sentinel_pending, 1); } + entry->memory = NULL; + SDL_TransferEventMemoryToEvent(entry); if (SDL_EventQ.tail) { SDL_EventQ.tail->next = entry; @@ -917,6 +978,8 @@ static int SDL_AddEvent(SDL_Event *event) /* Remove an event from the queue -- called with the queue locked */ static void SDL_CutEvent(SDL_EventEntry *entry) { + SDL_TransferEventMemoryFromEvent(entry); + if (entry->prev) { entry->prev->next = entry->next; } @@ -1093,10 +1156,7 @@ static void SDL_PumpEventsInternal(SDL_bool push_sentinel) SDL_VideoDevice *_this = SDL_GetVideoDevice(); /* Free old event memory */ - /*SDL_FlushEventMemory(SDL_last_event_id - SDL_MAX_QUEUED_EVENTS);*/ - if (SDL_AtomicGet(&SDL_EventQ.count) == 0) { - SDL_FlushEventMemory(SDL_last_event_id); - } + SDL_FreeEventMemory(NULL); /* Release any keys held down from last frame */ SDL_ReleaseAutoReleaseKeys(); diff --git a/test/testautomation_events.c b/test/testautomation_events.c index 14418f76cc..77d1770455 100644 --- a/test/testautomation_events.c +++ b/test/testautomation_events.c @@ -175,6 +175,108 @@ static int events_addDelEventWatchWithUserdata(void *arg) return TEST_COMPLETED; } +/** + * Creates and validates temporary event memory + */ +static int events_eventMemory(void *arg) +{ + SDL_Event event; + void *mem, *claimed, *tmp; + SDL_bool found; + + { + /* Create and claim event memory */ + mem = SDL_AllocateEventMemory(1); + SDLTest_AssertCheck(mem != NULL, "SDL_AllocateEventMemory()"); + *(char *)mem = '1'; + + claimed = SDL_ClaimEventMemory(mem); + SDLTest_AssertCheck(claimed != NULL, "SDL_ClaimEventMemory() returned a valid pointer"); + + /* Verify that we can't claim it again */ + tmp = SDL_ClaimEventMemory(mem); + SDLTest_AssertCheck(tmp == NULL, "SDL_ClaimEventMemory() can't claim memory twice"); + + /* Verify that freeing the original pointer does nothing */ + SDL_FreeEventMemory(mem); + SDLTest_AssertCheck(*(char *)mem == '1', "SDL_FreeEventMemory() on claimed memory has no effect"); + + /* Clean up */ + SDL_free(claimed); + } + + { + /* Create and free event memory */ + mem = SDL_AllocateEventMemory(1); + SDLTest_AssertCheck(mem != NULL, "SDL_AllocateEventMemory()"); + *(char *)mem = '1'; + + SDL_FreeEventMemory(mem); + claimed = SDL_ClaimEventMemory(mem); + SDLTest_AssertCheck(claimed == NULL, "SDL_ClaimEventMemory() can't claim memory after it's been freed"); + } + + { + /* Create event memory and queue it */ + mem = SDL_AllocateEventMemory(1); + SDLTest_AssertCheck(mem != NULL, "SDL_AllocateEventMemory()"); + *(char *)mem = '2'; + + SDL_zero(event); + event.type = SDL_EVENT_USER; + event.user.data1 = mem; + SDL_PushEvent(&event); + + /* Verify that we can't claim the memory once it's on the queue */ + claimed = SDL_ClaimEventMemory(mem); + SDLTest_AssertCheck(claimed == NULL, "SDL_ClaimEventMemory() can't claim memory on the event queue"); + + /* Get the event and verify the memory is valid */ + found = SDL_FALSE; + while (SDL_PollEvent(&event)) { + if (event.type == SDL_EVENT_USER && event.user.data1 == mem) { + found = SDL_TRUE; + } + } + SDLTest_AssertCheck(found, "SDL_PollEvent() returned queued event"); + + /* Verify that we can claim the memory now that we've dequeued it */ + claimed = SDL_ClaimEventMemory(mem); + SDLTest_AssertCheck(claimed != NULL, "SDL_ClaimEventMemory() can claim memory after dequeuing event"); + + /* Clean up */ + SDL_free(claimed); + } + + { + /* Create event memory and queue it */ + mem = SDL_AllocateEventMemory(1); + SDLTest_AssertCheck(mem != NULL, "SDL_AllocateEventMemory()"); + *(char *)mem = '3'; + + SDL_zero(event); + event.type = SDL_EVENT_USER; + event.user.data1 = mem; + SDL_PushEvent(&event); + + /* Get the event and verify the memory is valid */ + found = SDL_FALSE; + while (SDL_PollEvent(&event)) { + if (event.type == SDL_EVENT_USER && event.user.data1 == mem) { + found = SDL_TRUE; + } + } + SDLTest_AssertCheck(found, "SDL_PollEvent() returned queued event"); + + /* Verify that pumping the event loop frees event memory */ + SDL_PumpEvents(); + claimed = SDL_ClaimEventMemory(mem); + SDLTest_AssertCheck(claimed == NULL, "SDL_ClaimEventMemory() can't claim memory after pumping the event loop"); + } + + return TEST_COMPLETED; +} + /* ================= Test References ================== */ /* Events test cases */ @@ -190,9 +292,13 @@ static const SDLTest_TestCaseReference eventsTest3 = { (SDLTest_TestCaseFp)events_addDelEventWatchWithUserdata, "events_addDelEventWatchWithUserdata", "Adds and deletes an event watch function with userdata", TEST_ENABLED }; +static const SDLTest_TestCaseReference eventsTestEventMemory = { + (SDLTest_TestCaseFp)events_eventMemory, "events_eventMemory", "Creates and validates temporary event memory", TEST_ENABLED +}; + /* Sequence of Events test cases */ static const SDLTest_TestCaseReference *eventsTests[] = { - &eventsTest1, &eventsTest2, &eventsTest3, NULL + &eventsTest1, &eventsTest2, &eventsTest3, &eventsTestEventMemory, NULL }; /* Events test suite (global) */