Fix #110971: Replace our own barely-working backtrace on memleaks feature with ASAN utils. #111006

Bastien Montagne · 2023-08-10T15:02:03+02:00

Bastien Montagne commented

2023-08-10 15:02:03 +02:00

Instead of storing the backtrace in all memory blocks, and trying to get
meaningful info out of this list of pointers when printing leaked ones,
just use __asan_describe_address when ASAN is enabled.

This also work on Windows, in addition to linux and (presumably) OSX,
but does require to build with ASAN enabled.

The previous code was not working very well anymore, for some reason the
call to backtrace_symbols seems to fail to give any meaningful
information nowadays on most of Blender code. And it was only
implemented for linux and OSX.

Based on an idea from @LazyDodo, many thanks!

Instead of storing the backtrace in all memory blocks, and trying to get meaningful info out of this list of pointers when printing leaked ones, just use `__asan_describe_address` when ASAN is enabled. This also work on Windows, in addition to linux and (presumably) OSX, but does require to build with ASAN enabled. The previous code was not working very well anymore, for some reason the call to `backtrace_symbols` seems to fail to give any meaningful information nowadays on most of Blender code. And it was only implemented for linux and OSX. Based on an idea from @LazyDodo, many thanks!

👍 1 ❤️ 1

Bastien Montagne commented

2023-08-10 15:02:26 +02:00

@blender-bot build

Bastien Montagne requested review from Sergey Sharybin 2023-08-10 15:02:45 +02:00

Bastien Montagne requested review from Ray molenkamp 2023-08-10 15:02:45 +02:00

Bastien Montagne added the

labels 2023-08-10 15:03:09 +02:00

Bastien Montagne force-pushed guardedalloc-backtrace-fix from 73361879dc to 32c7ace6f0

2023-08-10 15:11:19 +02:00

Compare

Bastien Montagne commented

2023-08-10 15:12:28 +02:00

@blender-bot build

Sergey Sharybin approved these changes 2023-08-10 15:24:39 +02:00

Sergey Sharybin left a comment

From the description it seems all good. Some minor inlined comment.
Assume you tested it and such :)

From the description it seems all good. Some minor inlined comment. Assume you tested it and such :)

intern/guardedalloc/intern/mallocn_guarded_impl.c

						
				@ -104,9 +99,6 @@ typedef MemHead MemHeadAligned;

				#ifdef DEBUG_BACKTRACE

				#  if defined(__linux__) || defined(__APPLE__)

				#    include <execinfo.h>

Sergey Sharybin commented

2023-08-10 15:23:51 +02:00

Do we still need this include?

👍 1

mont29 marked this conversation as resolved

Bastien Montagne force-pushed guardedalloc-backtrace-fix from 32c7ace6f0 to d1e28d3aea

2023-08-10 15:49:44 +02:00

Compare

Ray molenkamp commented

2023-08-10 16:22:04 +02:00

minor changes to suggest, i enabled it by default on windows, since there's no LSAN there, improved the comment, and cleaned up some left over defines

diff --git a/intern/guardedalloc/intern/mallocn_guarded_impl.c b/intern/guardedalloc/intern/mallocn_guarded_impl.c
index 26a919984ae..ae1d0fc3385 100644
--- a/intern/guardedalloc/intern/mallocn_guarded_impl.c
+++ b/intern/guardedalloc/intern/mallocn_guarded_impl.c
@@ -40,14 +40,16 @@
 //#define DEBUG_MEMCOUNTER
 
 /* Only for debugging:
- * Defining DEBUG_BACKTRACE will store a backtrace from where
- * memory block was allocated and print this trace for all
- * unfreed blocks.
+ * Defining DEBUG_BACKTRACE will display a backtrace from where memory block was allocated and
+ * print this trace for all unfreed blocks. This will only work for asan enabled builds. This
+ * option will be on by default for MSVC as it currently does not have LSAN which would normally
+ * report these leaks, off by default on all other platforms because it would report the leaks
+ * twice, once here, and once by LSAN.
  */
+#if defined(_MSC_VER)
+#  define DEBUG_BACKTRACE
+#else
 //#define DEBUG_BACKTRACE
-
-#ifdef DEBUG_BACKTRACE
-#  define BACKTRACE_SIZE 100
 #endif
 
 #ifdef DEBUG_MEMCOUNTER

minor changes to suggest, i enabled it by default on windows, since there's no LSAN there, improved the comment, and cleaned up some left over defines ```diff diff --git a/intern/guardedalloc/intern/mallocn_guarded_impl.c b/intern/guardedalloc/intern/mallocn_guarded_impl.c index 26a919984ae..ae1d0fc3385 100644 --- a/intern/guardedalloc/intern/mallocn_guarded_impl.c +++ b/intern/guardedalloc/intern/mallocn_guarded_impl.c @@ -40,14 +40,16 @@ //#define DEBUG_MEMCOUNTER /* Only for debugging: - * Defining DEBUG_BACKTRACE will store a backtrace from where - * memory block was allocated and print this trace for all - * unfreed blocks. + * Defining DEBUG_BACKTRACE will display a backtrace from where memory block was allocated and + * print this trace for all unfreed blocks. This will only work for asan enabled builds. This + * option will be on by default for MSVC as it currently does not have LSAN which would normally + * report these leaks, off by default on all other platforms because it would report the leaks + * twice, once here, and once by LSAN. */ +#if defined(_MSC_VER) +# define DEBUG_BACKTRACE +#else //#define DEBUG_BACKTRACE - -#ifdef DEBUG_BACKTRACE -# define BACKTRACE_SIZE 100 #endif #ifdef DEBUG_MEMCOUNTER ```

👍 1

Ray molenkamp approved these changes 2023-08-10 16:24:29 +02:00

Ray molenkamp left a comment

accepted, given the minor changes suggested are done, not worth having another review round over unless there are objections.

Bastien Montagne force-pushed guardedalloc-backtrace-fix from d1e28d3aea to 95bc45b635

2023-08-10 17:54:34 +02:00

Compare

Bastien Montagne merged commit 2e79ca3205 into main

2023-08-10 17:55:03 +02:00

Bastien Montagne deleted branch guardedalloc-backtrace-fix

2023-08-10 17:55:05 +02:00

Bastien Montagne referenced this issue from a commit