Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDL2] Run tests with --trackmem #10637

Merged
merged 7 commits into from
Aug 31, 2024

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Aug 30, 2024

This backports c7a1876 and b68ac01 to enable --trackmem on ci.

Description

Existing Issue(s)

SDL_Mutex or SDL_SpinLock cannot be used as these use SDL_malloc internally.

Backport of c7a1876
@madebr madebr force-pushed the sdl2-ctest-with-trackmem branch 7 times, most recently from 9411ae0 to aefa0ce Compare August 31, 2024 15:01
@madebr
Copy link
Contributor Author

madebr commented Aug 31, 2024

Is running SDL2 with --trackmem enabled something we want?

On Windows, I see these leaks
1: INFO: Memory allocations:
1: Expect 2 allocations from within SDL_GetErrBuf()
1: Allocation 0: 6 bytes
1: 	0x7ff73b42e559: SDL_realloc_REAL+0x29 D:\a\SDL\SDL\src\stdlib\SDL_malloc.c:5326
1: 	0x7ff73b431708: SDL_getenv_REAL+0x48 D:\a\SDL\SDL\src\stdlib\SDL_getenv.c:202
1: 	0x7ff73b409305: SDL_GetHint_REAL+0x25 D:\a\SDL\SDL\src\SDL_hints.c:169
1: 	0x7ff73b418aac: SDL_VideoInit_REAL+0x8c D:\a\SDL\SDL\src\video\SDL_video.c:488
1: 	0x7ff73b3c3b98: SDLTest_CommonInit+0xc8 D:\a\SDL\SDL\src\test\SDL_test_common.c:1157
1: 	0x7ff73b398063: SDL_main+0x1b3 D:\a\SDL\SDL\test\testautomation.c:93
1: 	0x7ff73b527d30: main_getcmdline+0x160 D:\a\SDL\SDL\src\main\windows\SDL_windows_main.c:80
1: 	0x7ff73b5267b0: __scrt_common_main_seh+0x10c D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
1: 	0x7ffae0d14cb0: BaseThreadInitThunk+0x10 :0
1: 	0x7ffae2e3eceb: RtlUserThreadStart+0x2b :0
1: Allocation 1: 9 bytes
1: 	0x7ff73b42e4fb: SDL_malloc_REAL+0x1b D:\a\SDL\SDL\src\stdlib\SDL_malloc.c:5295
1: 	0x7ff73b3e1f13: SDL_strdup_REAL+0x23 D:\a\SDL\SDL\src\stdlib\SDL_string.c:642
1: 	0x7ff73b39cc96: hints_testHintChanged+0x16 D:\a\SDL\SDL\test\testautomation_hints.c:93
1: 	0x7ff73b409474: SDL_ResetHint_REAL+0xa4 D:\a\SDL\SDL\src\SDL_hints.c:118
1: 	0x7ff73b39ca11: hints_setHint+0x471 D:\a\SDL\SDL\test\testautomation_hints.c:218
1: 	0x7ff73b3c7d6b: SDLTest_RunTest+0xfb D:\a\SDL\SDL\src\test\SDL_test_harness.c:261
1: 	0x7ff73b3c76bb: SDLTest_RunSuites+0x68b D:\a\SDL\SDL\src\test\SDL_test_harness.c:582
1: 	0x7ff73b3980d8: SDL_main+0x228 D:\a\SDL\SDL\test\testautomation.c:105
1: 	0x7ff73b527d30: main_getcmdline+0x160 D:\a\SDL\SDL\src\main\windows\SDL_windows_main.c:80
1: 	0x7ff73b5267b0: __scrt_common_main_seh+0x10c D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
1: 	0x7ffae0d14cb0: BaseThreadInitThunk+0x10 :0
1: 	0x7ffae2e3eceb: RtlUserThreadStart+0x2b :0
1: Allocation 2: 116 bytes
1: 	0x7ff73b42e4fb: SDL_malloc_REAL+0x1b D:\a\SDL\SDL\src\stdlib\SDL_malloc.c:5295
1: 	0x7ff73b3e1f13: SDL_strdup_REAL+0x23 D:\a\SDL\SDL\src\stdlib\SDL_string.c:642
1: 	0x7ff73b3fcf3b: SDL_SetPrimarySelectionText_REAL+0x7b D:\a\SDL\SDL\src\video\SDL_clipboard.c:61
1: 	0x7ff73b39baab: clipboard_testPrimarySelectionTextFunctions+0x11b D:\a\SDL\SDL\test\testautomation_clipboard.c:258
1: 	0x7ff73b3c7d6b: SDLTest_RunTest+0xfb D:\a\SDL\SDL\src\test\SDL_test_harness.c:261
1: 	0x7ff73b3c76bb: SDLTest_RunSuites+0x68b D:\a\SDL\SDL\src\test\SDL_test_harness.c:582
1: 	0x7ff73b3980d8: SDL_main+0x228 D:\a\SDL\SDL\test\testautomation.c:105
1: 	0x7ff73b527d30: main_getcmdline+0x160 D:\a\SDL\SDL\src\main\windows\SDL_windows_main.c:80
1: 	0x7ff73b5267b0: __scrt_common_main_seh+0x10c D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
1: 	0x7ffae0d14cb0: BaseThreadInitThunk+0x10 :0
1: 	0x7ffae2e3eceb: RtlUserThreadStart+0x2b :0
1: Allocation 3: 16 bytes
1: 	0x7ff73b42e46d: SDL_calloc_REAL+0x1d D:\a\SDL\SDL\src\stdlib\SDL_malloc.c:5311
1: 	0x7ff73b41bfc5: SDL_CreateMutex_srw+0x15 D:\a\SDL\SDL\src\thread\windows\SDL_sysmutex.c:64
1: 	0x7ff73b3e03af: SDL_LogMessageV_REAL+0x4f D:\a\SDL\SDL\src\SDL_log.c:410
1: 	0x7ff73b3da6a9: SDL_LogMessage+0x19 D:\a\SDL\SDL\src\dynapi\SDL_dynapi.c:219
1: 	0x7ff73b3c8aff: SDLTest_Log+0x7f D:\a\SDL\SDL\src\test\SDL_test_log.c:102
1: 	0x7ff73b3c815a: SDLTest_AssertPass+0x6a D:\a\SDL\SDL\src\test\SDL_test_assert.c:105
1: 	0x7ff73b3b8635: subsystemsSetUp+0x15 D:\a\SDL\SDL\test\testautomation_subsystems.c:23
1: 	0x7ff73b3c7d20: SDLTest_RunTest+0xb0 D:\a\SDL\SDL\src\test\SDL_test_harness.c:254
1: 	0x7ff73b3c76bb: SDLTest_RunSuites+0x68b D:\a\SDL\SDL\src\test\SDL_test_harness.c:582
1: 	0x7ff73b3980d8: SDL_main+0x228 D:\a\SDL\SDL\test\testautomation.c:105
1: 	0x7ff73b527d30: main_getcmdline+0x160 D:\a\SDL\SDL\src\main\windows\SDL_windows_main.c:80
1: 	0x7ff73b5267b0: __scrt_common_main_seh+0x10c D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
1: 	0x7ffae0d14cb0: BaseThreadInitThunk+0x10 :0
1: 	0x7ffae2e3eceb: RtlUserThreadStart+0x2b :0
1: Allocation 4: 88 bytes
1: 	0x7ff73b42e559: SDL_realloc_REAL+0x29 D:\a\SDL\SDL\src\stdlib\SDL_malloc.c:5326
1: 	0x7ff73b3e4740: SDL_TLSSet_REAL+0x80 D:\a\SDL\SDL\src\thread\SDL_thread.c:78
1: 	0x7ff73b3e43a4: SDL_GetErrBuf+0xb4 D:\a\SDL\SDL\src\thread\SDL_thread.c:305
1: 	0x7ff73b3dfc39: SDL_ClearError_REAL+0x9 D:\a\SDL\SDL\src\SDL_error.c:72
1: 	0x7ff73b3ef311: SDL_InitSubSystem_REAL+0x31 D:\a\SDL\SDL\src\SDL.c:229
1: 	0x7ff73b3b8b7f: subsystems_dependRefCountWithExtraInit+0x2f D:\a\SDL\SDL\test\testautomation_subsystems.c:175
1: 	0x7ff73b3c7d6b: SDLTest_RunTest+0xfb D:\a\SDL\SDL\src\test\SDL_test_harness.c:261
1: 	0x7ff73b3c76bb: SDLTest_RunSuites+0x68b D:\a\SDL\SDL\src\test\SDL_test_harness.c:582
1: 	0x7ff73b3980d8: SDL_main+0x228 D:\a\SDL\SDL\test\testautomation.c:105
1: 	0x7ff73b527d30: main_getcmdline+0x160 D:\a\SDL\SDL\src\main\windows\SDL_windows_main.c:80
1: 	0x7ff73b5267b0: __scrt_common_main_seh+0x10c D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
1: 	0x7ffae0d14cb0: BaseThreadInitThunk+0x10 :0
1: 	0x7ffae2e3eceb: RtlUserThreadStart+0x2b :0
1: Total: 0.22 Kb in 5 allocations
On Linux, I see these leaks
1: INFO: Memory allocations:
1: Expect 2 allocations from within SDL_GetErrBuf()
1: Allocation 0: 88 bytes
1: 	0x559d55ddb986: SDL_realloc_REAL+0x26
1: 	0x559d55ddcd34: SDL_TLSSet_REAL+0x64
1: 	0x559d55ddd1b4: SDL_GetErrBuf+0xe4
1: 	0x559d55e24bbd: SDL_ClearError_REAL+0xd
1: 	0x559d55e23d51: SDL_InitSubSystem_REAL+0x21
1: 	0x559d55d81d12: subsystems_dependRefCountWithExtraInit+0x32
1: 	0x559d55d90de8: SDLTest_RunSuites+0x598
1: 	0x559d55d66d42: main+0x182
1: 	0x7f1f4502d083: __libc_start_main+0xf3
1: 	0x559d55d66ede: _start+0x2e
1: Allocation 1: 109 bytes
1: 	0x559d55ddb8e8: SDL_malloc_REAL+0x18
1: 	0x559d55ddc52d: SDL_strdup_REAL+0x1d
1: 	0x559d55ddf38d: SDL_SetPrimarySelectionText_REAL+0x5d
1: 	0x559d55d69ca6: clipboard_testPrimarySelectionTextFunctions+0x96
1: 	0x559d55d90de8: SDLTest_RunSuites+0x598
1: 	0x559d55d66d42: main+0x182
1: 	0x7f1f4502d083: __libc_start_main+0xf3
1: 	0x559d55d66ede: _start+0x2e
1: Allocation 2: 9 bytes
1: 	0x559d55ddb8e8: SDL_malloc_REAL+0x18
1: 	0x559d55ddc52d: SDL_strdup_REAL+0x1d
1: 	0x559d55d6a705: hints_testHintChanged+0x15
1: 	0x559d55e250cb: SDL_ResetHint_REAL+0x9b
1: 	0x559d55d6abac: hints_setHint+0x49c
1: 	0x559d55d90de8: SDLTest_RunSuites+0x598
1: 	0x559d55d66d42: main+0x182
1: 	0x7f1f4502d083: __libc_start_main+0xf3
1: 	0x559d55d66ede: _start+0x2e
1: Allocation 3: 40 bytes
1: 	0x559d55ddb926: SDL_calloc_REAL+0x16
1: 	0x559d55e213da: SDL_CreateMutex_REAL+0x2a
1: 	0x559d55e25d55: SDL_LogMessageV_REAL+0x1d5
1: 	0x559d55da3754: SDL_LogMessage+0x94
1: 	0x559d55d91b60: SDLTest_Log+0x110
1: 	0x559d55d897e2: SDLTest_AssertPass+0x102
1: 	0x559d55d81cbb: subsystemsSetUp+0x1b
1: 	0x559d55d90dcf: SDLTest_RunSuites+0x57f
1: 	0x559d55d66d42: main+0x182
1: 	0x7f1f4502d083: __libc_start_main+0xf3
1: 	0x559d55d66ede: _start+0x2e
1: Total: 0.24 Kb in 4 allocations
Macos also has some leaks (no stacktrace because libunwind is not available)
 1: Expect 2 allocations from within SDL_GetErrBuf()
1: Allocation 0: 64 bytes
1: Allocation 1: 9 bytes
1: Allocation 2: 88 bytes
1: Allocation 3: 55 bytes
1: Total: 0.21 Kb in 4 allocations

I'd like to port these changes to sdl2-compat, and enable it on its ci.

@slouken
Copy link
Collaborator

slouken commented Aug 31, 2024

Is running SDL2 with --trackmem enabled something we want?

On Windows, I see these leaks
On Linux, I see these leaks
Macos also has some leaks (no stacktrace because libunwind is not available)
I'd like to port these changes to sdl2-compat, and enable it on its ci.

We've done a bunch of work on SDL3 to clean up all leaks in SDL_Quit(), which we aren't planning to backport to SDL2. I'm fine if someone wants to do that work, but some of it involved API changes and significant rework, so I don't think it's worth it for now.

@madebr
Copy link
Contributor Author

madebr commented Aug 31, 2024

We've done a bunch of work on SDL3 to clean up all leaks in SDL_Quit(), which we aren't planning to backport to SDL2. I'm fine if someone wants to do that work, but some of it involved API changes and significant rework, so I don't think it's worth it for now.

Understood. I'll just leave the ci --trackmem commit out.

@madebr madebr force-pushed the sdl2-ctest-with-trackmem branch 3 times, most recently from 2cf9bfc to f2f0807 Compare August 31, 2024 17:23
@madebr madebr force-pushed the sdl2-ctest-with-trackmem branch from f2f0807 to 8ae28ec Compare August 31, 2024 20:49
@madebr madebr merged commit 2ae8b4c into libsdl-org:SDL2 Aug 31, 2024
40 checks passed
@madebr madebr deleted the sdl2-ctest-with-trackmem branch August 31, 2024 21:49
@sezero
Copy link
Contributor

sezero commented Sep 4, 2024

After sync'ing sdl2-compat test programs with latest sdl2, I'm getting
the following warnings:

/tmp/sdl2-compat/test/testlocale.c: In function 'main':
/tmp/sdl2-compat/test/testlocale.c:66:13: warning: implicit declaration of function 'exit' [-Wimplicit-function-declaration]
             exit(1);
             ^
/tmp/sdl2-compat/test/testlocale.c:66:13: warning: incompatible implicit declaration of built-in function 'exit'
/tmp/sdl2-compat/test/testplatform.c: In function 'main':
/tmp/sdl2-compat/test/testplatform.c:475:13: warning: implicit declaration of function 'exit' [-Wimplicit-function-declaration]
             exit(1);
             ^
/tmp/sdl2-compat/test/testplatform.c:475:13: warning: incompatible implicit declaration of built-in function 'exit'
/tmp/sdl2-compat/test/testqsort.c: In function 'main':
/tmp/sdl2-compat/test/testqsort.c:94:13: warning: implicit declaration of function 'exit' [-Wimplicit-function-declaration]
             exit(1);
             ^
/tmp/sdl2-compat/test/testqsort.c:94:13: warning: incompatible implicit declaration of built-in function 'exit'

All of those exit()s are following SDLTest_CommonLogUsage(), btw.

Do we prefer changing those exit(1) calls to return 1, or do we keep
them as they are but include stdlib.h?

@madebr
Copy link
Contributor Author

madebr commented Sep 4, 2024

Calls to exit are a smell imho. If a return 1 suffices, let's do that.
I noticed the examples use exit to do cleanup in case of an error condition.
It's heresy, but I kinda like to use goto to handle error conditions, and avoid an indentation hell.

For SDL2, I'd say to add #include <stdlib.h>, but for SDL3 make an effort to avoid it.

@slouken
Copy link
Collaborator

slouken commented Sep 4, 2024

Calls to exit are a smell imho. If a return 1 suffices, let's do that. I noticed the examples use exit to do cleanup in case of an error condition. It's heresy, but I kinda like to use goto to handle error conditions, and avoid an indentation hell.

For SDL2, I'd say to add #include <stdlib.h>, but for SDL3 make an effort to avoid it.

It looks like these came from your adding command line parsing. I'm switching them to returns.

@sezero
Copy link
Contributor

sezero commented Sep 4, 2024

It's heresy, but I kinda like to use goto to handle error conditions,

I too am one of those heretics, so, you're not alone

@sezero
Copy link
Contributor

sezero commented Sep 4, 2024

It looks like these came from your adding command line parsing. I'm switching them to returns.

OK, leaving to you. Will sync sdl2-compat when it's done.

@slouken
Copy link
Collaborator

slouken commented Sep 4, 2024

It looks like these came from your adding command line parsing. I'm switching them to returns.

I started into the rabbit hole of fixing SDL2 test program return values and quickly backed right out. Feel free to make whatever change you want for SDL2. Changing them to return or adding #include <stdlib.h> /* exit() */ is fine.

@sezero
Copy link
Contributor

sezero commented Sep 4, 2024

I chose to changing those three to use returns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants