[6/6] tests: Remove memory leak checking infrastructure

Submitted by Daniel Stone on Aug. 29, 2018, 6:17 a.m.

Details

Message ID 20180829061715.27776-7-daniels@collabora.com
State Accepted
Commit 01095a9ce4d8457ee2f221848b9805c4b5cffc95
Headers show
Series "Minor test/scanner fixes" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Daniel Stone Aug. 29, 2018, 6:17 a.m.
There are far better ways to detect memory leaks, such as either
valgrind or ASan. Having Meson makes it really easy to use these tools
in our tests, and we can do that in CI as well.

Having these local wrappers actually completely broke ASan usage, so
remove them in favour of using the more powerful options.

Signed-off-by: Daniel Stone <daniels@collabora.com>
---
 tests/sanity-test.c     | 66 ++--------------------------------
 tests/test-compositor.c |  5 ++-
 tests/test-runner.c     | 79 ++++++-----------------------------------
 tests/test-runner.h     | 13 +++----
 4 files changed, 20 insertions(+), 143 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/sanity-test.c b/tests/sanity-test.c
index 2495a115..98beca8d 100644
--- a/tests/sanity-test.c
+++ b/tests/sanity-test.c
@@ -35,7 +35,7 @@ 
 
 #include "test-compositor.h"
 
-extern int leak_check_enabled;
+extern int fd_leak_check_enabled;
 
 TEST(empty)
 {
@@ -83,71 +83,11 @@  FAIL_TEST(sanity_assert)
 	assert(0);
 }
 
-FAIL_TEST(sanity_malloc_direct)
-{
-	void *p;
-
-	assert(leak_check_enabled);
-
-	p = malloc(10);	/* memory leak */
-	assert(p);	/* assert that we got memory, also prevents
-			 * the malloc from getting optimized away. */
-	free(NULL);	/* NULL must not be counted */
-	test_disable_coredumps();
-}
-
-TEST(disable_leak_checks)
-{
-	volatile void *mem;
-	assert(leak_check_enabled);
-	/* normally this should be on the beginning of the test.
-	 * Here we need to be sure, that the leak checks are
-	 * turned on */
-	DISABLE_LEAK_CHECKS;
-
-	mem = malloc(16);
-	assert(mem);
-}
-
-FAIL_TEST(sanity_malloc_indirect)
-{
-	struct wl_array array;
-
-	assert(leak_check_enabled);
-
-	wl_array_init(&array);
-
-	/* call into library that calls malloc */
-	wl_array_add(&array, 14);
-
-	/* not freeing array, must leak */
-
-	test_disable_coredumps();
-}
-
-FAIL_TEST(tc_client_memory_leaks)
-{
-	struct display *d = display_create();
-	client_create_noarg(d, sanity_malloc_direct);
-	display_run(d);
-	test_disable_coredumps();
-	display_destroy(d);
-}
-
-FAIL_TEST(tc_client_memory_leaks2)
-{
-	struct display *d = display_create();
-	client_create_noarg(d, sanity_malloc_indirect);
-	display_run(d);
-	test_disable_coredumps();
-	display_destroy(d);
-}
-
 FAIL_TEST(sanity_fd_leak)
 {
 	int fd[2];
 
-	assert(leak_check_enabled);
+	assert(fd_leak_check_enabled);
 
 	/* leak 2 file descriptors */
 	if (pipe(fd) < 0)
@@ -185,7 +125,7 @@  sanity_fd_no_leak(void)
 {
 	int fd[2];
 
-	assert(leak_check_enabled);
+	assert(fd_leak_check_enabled);
 
 	/* leak 2 file descriptors */
 	if (pipe(fd) < 0)
diff --git a/tests/test-compositor.c b/tests/test-compositor.c
index 0631f614..72f63515 100644
--- a/tests/test-compositor.c
+++ b/tests/test-compositor.c
@@ -156,7 +156,7 @@  run_client(void (*client_main)(void *data), void *data,
 	   int wayland_sock, int client_pipe)
 {
 	char s[8];
-	int cur_alloc, cur_fds;
+	int cur_fds;
 	int can_continue = 0;
 
 	/* Wait until display signals that client can continue */
@@ -169,7 +169,6 @@  run_client(void (*client_main)(void *data), void *data,
 	snprintf(s, sizeof s, "%d", wayland_sock);
 	setenv("WAYLAND_SOCKET", s, 0);
 
-	cur_alloc = get_current_alloc_num();
 	cur_fds = count_open_fds();
 
 	client_main(data);
@@ -182,7 +181,7 @@  run_client(void (*client_main)(void *data), void *data,
 	if (!getenv("WAYLAND_SOCKET"))
 		cur_fds--;
 
-	check_leaks(cur_alloc, cur_fds);
+	check_fd_leaks(cur_fds);
 }
 
 static struct client_info *
diff --git a/tests/test-runner.c b/tests/test-runner.c
index 82a0a7b8..1487dc48 100644
--- a/tests/test-runner.c
+++ b/tests/test-runner.c
@@ -44,16 +44,10 @@ 
 
 #include "test-runner.h"
 
-static int num_alloc;
-static void* (*sys_malloc)(size_t);
-static void (*sys_free)(void*);
-static void* (*sys_realloc)(void*, size_t);
-static void* (*sys_calloc)(size_t, size_t);
-
-/* when set to 1, check if tests are not leaking memory and opened files.
+/* when set to 1, check if tests are not leaking opened files.
  * It is turned on by default. It can be turned off by
  * WAYLAND_TEST_NO_LEAK_CHECK environment variable. */
-int leak_check_enabled;
+int fd_leak_check_enabled;
 
 /* when this var is set to 0, every call to test_set_timeout() is
  * suppressed - handy when debugging the test. Can be set by
@@ -65,40 +59,6 @@  static int is_atty = 0;
 
 extern const struct test __start_test_section, __stop_test_section;
 
-__attribute__ ((visibility("default"))) void *
-malloc(size_t size)
-{
-	num_alloc++;
-	return sys_malloc(size);
-}
-
-__attribute__ ((visibility("default"))) void
-free(void* mem)
-{
-	if (mem != NULL)
-		num_alloc--;
-	sys_free(mem);
-}
-
-__attribute__ ((visibility("default"))) void *
-realloc(void* mem, size_t size)
-{
-	if (mem == NULL)
-		num_alloc++;
-	return sys_realloc(mem, size);
-}
-
-__attribute__ ((visibility("default"))) void *
-calloc(size_t nmemb, size_t size)
-{
-	if (sys_calloc == NULL)
-		return NULL;
-
-	num_alloc++;
-
-	return sys_calloc(nmemb, size);
-}
-
 static const struct test *
 find_test(const char *name)
 {
@@ -156,25 +116,12 @@  sigalrm_handler(int signum)
 	abort();
 }
 
-int
-get_current_alloc_num(void)
-{
-	return num_alloc;
-}
-
 void
-check_leaks(int supposed_alloc, int supposed_fds)
+check_fd_leaks(int supposed_fds)
 {
 	int num_fds;
 
-	if (leak_check_enabled) {
-		if (supposed_alloc != num_alloc) {
-			fprintf(stderr, "Memory leak detected in test. "
-				"Allocated %d blocks, unfreed %d\n", num_alloc,
-				num_alloc - supposed_alloc);
-			abort();
-		}
-
+	if (fd_leak_check_enabled) {
 		num_fds = count_open_fds();
 		if (supposed_fds != num_fds) {
 			fprintf(stderr, "fd leak detected in test. "
@@ -183,14 +130,14 @@  check_leaks(int supposed_alloc, int supposed_fds)
 			abort();
 		}
 	} else {
-		fprintf(stderr, "Leak checks disabled\n");
+		fprintf(stderr, "FD leak checks disabled\n");
 	}
 }
 
 static void
 run_test(const struct test *t)
 {
-	int cur_alloc, cur_fds;
+	int cur_fds;
 	struct sigaction sa;
 
 	if (timeouts_enabled) {
@@ -200,7 +147,7 @@  run_test(const struct test *t)
 		assert(sigaction(SIGALRM, &sa, NULL) == 0);
 	}
 
-	cur_alloc = get_current_alloc_num();
+	//cur_alloc = get_current_alloc_num();
 	cur_fds = count_open_fds();
 
 	t->run();
@@ -209,7 +156,7 @@  run_test(const struct test *t)
 	if (timeouts_enabled)
 		alarm(0);
 
-	check_leaks(cur_alloc, cur_fds);
+	check_fd_leaks(cur_fds);
 
 	exit(EXIT_SUCCESS);
 }
@@ -348,20 +295,14 @@  int main(int argc, char *argv[])
 	int total, pass;
 	siginfo_t info;
 
-	/* Load system malloc, free, and realloc */
-	sys_calloc = dlsym(RTLD_NEXT, "calloc");
-	sys_realloc = dlsym(RTLD_NEXT, "realloc");
-	sys_malloc = dlsym(RTLD_NEXT, "malloc");
-	sys_free = dlsym(RTLD_NEXT, "free");
-
 	if (isatty(fileno(stderr)))
 		is_atty = 1;
 
 	if (is_debugger_attached()) {
-		leak_check_enabled = 0;
+		fd_leak_check_enabled = 0;
 		timeouts_enabled = 0;
 	} else {
-		leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
+		fd_leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
 		timeouts_enabled = !getenv("WAYLAND_TEST_NO_TIMEOUTS");
 	}
 
diff --git a/tests/test-runner.h b/tests/test-runner.h
index 9c47a2b0..d0734009 100644
--- a/tests/test-runner.h
+++ b/tests/test-runner.h
@@ -63,11 +63,8 @@  count_open_fds(void);
 void
 exec_fd_leak_check(int nr_expected_fds); /* never returns */
 
-int
-get_current_alloc_num(void);
-
 void
-check_leaks(int supposed_allocs, int supposed_fds);
+check_fd_leaks(int supposed_fds);
 
 /*
  * set/reset the timeout in seconds. The timeout starts
@@ -89,10 +86,10 @@  test_sleep(unsigned int);
 void
 test_disable_coredumps(void);
 
-#define DISABLE_LEAK_CHECKS			\
-	do {					\
-		extern int leak_check_enabled;	\
-		leak_check_enabled = 0;		\
+#define DISABLE_LEAK_CHECKS				\
+	do {						\
+		extern int fd_leak_check_enabled;	\
+		fd_leak_check_enabled = 0;		\
 	} while (0);
 
 #endif

Comments

> On 29 Aug 2018, at 9.17, Daniel Stone <daniels@collabora.com> wrote:
> 
> There are far better ways to detect memory leaks, such as either
> valgrind or ASan. Having Meson makes it really easy to use these tools
> in our tests, and we can do that in CI as well.
> 
> Having these local wrappers actually completely broke ASan usage, so
> remove them in favour of using the more powerful options.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>
> ---
> tests/sanity-test.c     | 66 ++--------------------------------
> tests/test-compositor.c |  5 ++-
> tests/test-runner.c     | 79 ++++++-----------------------------------
> tests/test-runner.h     | 13 +++----
> 4 files changed, 20 insertions(+), 143 deletions(-)
> 
> diff --git a/tests/sanity-test.c b/tests/sanity-test.c
> index 2495a115..98beca8d 100644
> --- a/tests/sanity-test.c
> +++ b/tests/sanity-test.c
> @@ -35,7 +35,7 @@
> 
> #include "test-compositor.h"
> 
> -extern int leak_check_enabled;
> +extern int fd_leak_check_enabled;
> 
> TEST(empty)
> {
> @@ -83,71 +83,11 @@ FAIL_TEST(sanity_assert)
> 	assert(0);
> }
> 
> -FAIL_TEST(sanity_malloc_direct)
> -{
> -	void *p;
> -
> -	assert(leak_check_enabled);
> -
> -	p = malloc(10);	/* memory leak */
> -	assert(p);	/* assert that we got memory, also prevents
> -			 * the malloc from getting optimized away. */
> -	free(NULL);	/* NULL must not be counted */
> -	test_disable_coredumps();
> -}
> -
> -TEST(disable_leak_checks)
> -{
> -	volatile void *mem;
> -	assert(leak_check_enabled);
> -	/* normally this should be on the beginning of the test.
> -	 * Here we need to be sure, that the leak checks are
> -	 * turned on */
> -	DISABLE_LEAK_CHECKS;
> -
> -	mem = malloc(16);
> -	assert(mem);
> -}
> -
> -FAIL_TEST(sanity_malloc_indirect)
> -{
> -	struct wl_array array;
> -
> -	assert(leak_check_enabled);
> -
> -	wl_array_init(&array);
> -
> -	/* call into library that calls malloc */
> -	wl_array_add(&array, 14);
> -
> -	/* not freeing array, must leak */
> -
> -	test_disable_coredumps();
> -}
> -
> -FAIL_TEST(tc_client_memory_leaks)
> -{
> -	struct display *d = display_create();
> -	client_create_noarg(d, sanity_malloc_direct);
> -	display_run(d);
> -	test_disable_coredumps();
> -	display_destroy(d);
> -}
> -
> -FAIL_TEST(tc_client_memory_leaks2)
> -{
> -	struct display *d = display_create();
> -	client_create_noarg(d, sanity_malloc_indirect);
> -	display_run(d);
> -	test_disable_coredumps();
> -	display_destroy(d);
> -}
> -
> FAIL_TEST(sanity_fd_leak)
> {
> 	int fd[2];
> 
> -	assert(leak_check_enabled);
> +	assert(fd_leak_check_enabled);
> 
> 	/* leak 2 file descriptors */
> 	if (pipe(fd) < 0)
> @@ -185,7 +125,7 @@ sanity_fd_no_leak(void)
> {
> 	int fd[2];
> 
> -	assert(leak_check_enabled);
> +	assert(fd_leak_check_enabled);
> 
> 	/* leak 2 file descriptors */
> 	if (pipe(fd) < 0)
> diff --git a/tests/test-compositor.c b/tests/test-compositor.c
> index 0631f614..72f63515 100644
> --- a/tests/test-compositor.c
> +++ b/tests/test-compositor.c
> @@ -156,7 +156,7 @@ run_client(void (*client_main)(void *data), void *data,
> 	   int wayland_sock, int client_pipe)
> {
> 	char s[8];
> -	int cur_alloc, cur_fds;
> +	int cur_fds;
> 	int can_continue = 0;
> 
> 	/* Wait until display signals that client can continue */
> @@ -169,7 +169,6 @@ run_client(void (*client_main)(void *data), void *data,
> 	snprintf(s, sizeof s, "%d", wayland_sock);
> 	setenv("WAYLAND_SOCKET", s, 0);
> 
> -	cur_alloc = get_current_alloc_num();
> 	cur_fds = count_open_fds();
> 
> 	client_main(data);
> @@ -182,7 +181,7 @@ run_client(void (*client_main)(void *data), void *data,
> 	if (!getenv("WAYLAND_SOCKET"))
> 		cur_fds--;
> 
> -	check_leaks(cur_alloc, cur_fds);
> +	check_fd_leaks(cur_fds);
> }
> 
> static struct client_info *
> diff --git a/tests/test-runner.c b/tests/test-runner.c
> index 82a0a7b8..1487dc48 100644
> --- a/tests/test-runner.c
> +++ b/tests/test-runner.c
> @@ -44,16 +44,10 @@
> 
> #include "test-runner.h"
> 
> -static int num_alloc;
> -static void* (*sys_malloc)(size_t);
> -static void (*sys_free)(void*);
> -static void* (*sys_realloc)(void*, size_t);
> -static void* (*sys_calloc)(size_t, size_t);
> -
> -/* when set to 1, check if tests are not leaking memory and opened files.
> +/* when set to 1, check if tests are not leaking opened files.
>  * It is turned on by default. It can be turned off by
>  * WAYLAND_TEST_NO_LEAK_CHECK environment variable. */
> -int leak_check_enabled;
> +int fd_leak_check_enabled;
> 
> /* when this var is set to 0, every call to test_set_timeout() is
>  * suppressed - handy when debugging the test. Can be set by
> @@ -65,40 +59,6 @@ static int is_atty = 0;
> 
> extern const struct test __start_test_section, __stop_test_section;
> 
> -__attribute__ ((visibility("default"))) void *
> -malloc(size_t size)
> -{
> -	num_alloc++;
> -	return sys_malloc(size);
> -}
> -
> -__attribute__ ((visibility("default"))) void
> -free(void* mem)
> -{
> -	if (mem != NULL)
> -		num_alloc--;
> -	sys_free(mem);
> -}
> -
> -__attribute__ ((visibility("default"))) void *
> -realloc(void* mem, size_t size)
> -{
> -	if (mem == NULL)
> -		num_alloc++;
> -	return sys_realloc(mem, size);
> -}
> -
> -__attribute__ ((visibility("default"))) void *
> -calloc(size_t nmemb, size_t size)
> -{
> -	if (sys_calloc == NULL)
> -		return NULL;
> -
> -	num_alloc++;
> -
> -	return sys_calloc(nmemb, size);
> -}
> -
> static const struct test *
> find_test(const char *name)
> {
> @@ -156,25 +116,12 @@ sigalrm_handler(int signum)
> 	abort();
> }
> 
> -int
> -get_current_alloc_num(void)
> -{
> -	return num_alloc;
> -}
> -
> void
> -check_leaks(int supposed_alloc, int supposed_fds)
> +check_fd_leaks(int supposed_fds)
> {
> 	int num_fds;
> 
> -	if (leak_check_enabled) {
> -		if (supposed_alloc != num_alloc) {
> -			fprintf(stderr, "Memory leak detected in test. "
> -				"Allocated %d blocks, unfreed %d\n", num_alloc,
> -				num_alloc - supposed_alloc);
> -			abort();
> -		}
> -
> +	if (fd_leak_check_enabled) {
> 		num_fds = count_open_fds();
> 		if (supposed_fds != num_fds) {
> 			fprintf(stderr, "fd leak detected in test. "
> @@ -183,14 +130,14 @@ check_leaks(int supposed_alloc, int supposed_fds)
> 			abort();
> 		}
> 	} else {
> -		fprintf(stderr, "Leak checks disabled\n");
> +		fprintf(stderr, "FD leak checks disabled\n");
> 	}
> }
> 
> static void
> run_test(const struct test *t)
> {
> -	int cur_alloc, cur_fds;
> +	int cur_fds;
> 	struct sigaction sa;
> 
> 	if (timeouts_enabled) {
> @@ -200,7 +147,7 @@ run_test(const struct test *t)
> 		assert(sigaction(SIGALRM, &sa, NULL) == 0);
> 	}
> 
> -	cur_alloc = get_current_alloc_num();
> +	//cur_alloc = get_current_alloc_num();

Nit: the above line could/should have been deleted.

Reviewed-by: Ian Ray <ian.ray@ge.com>


> 	cur_fds = count_open_fds();
> 
> 	t->run();
> @@ -209,7 +156,7 @@ run_test(const struct test *t)
> 	if (timeouts_enabled)
> 		alarm(0);
> 
> -	check_leaks(cur_alloc, cur_fds);
> +	check_fd_leaks(cur_fds);
> 
> 	exit(EXIT_SUCCESS);
> }
> @@ -348,20 +295,14 @@ int main(int argc, char *argv[])
> 	int total, pass;
> 	siginfo_t info;
> 
> -	/* Load system malloc, free, and realloc */
> -	sys_calloc = dlsym(RTLD_NEXT, "calloc");
> -	sys_realloc = dlsym(RTLD_NEXT, "realloc");
> -	sys_malloc = dlsym(RTLD_NEXT, "malloc");
> -	sys_free = dlsym(RTLD_NEXT, "free");
> -
> 	if (isatty(fileno(stderr)))
> 		is_atty = 1;
> 
> 	if (is_debugger_attached()) {
> -		leak_check_enabled = 0;
> +		fd_leak_check_enabled = 0;
> 		timeouts_enabled = 0;
> 	} else {
> -		leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
> +		fd_leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
> 		timeouts_enabled = !getenv("WAYLAND_TEST_NO_TIMEOUTS");
> 	}
> 
> diff --git a/tests/test-runner.h b/tests/test-runner.h
> index 9c47a2b0..d0734009 100644
> --- a/tests/test-runner.h
> +++ b/tests/test-runner.h
> @@ -63,11 +63,8 @@ count_open_fds(void);
> void
> exec_fd_leak_check(int nr_expected_fds); /* never returns */
> 
> -int
> -get_current_alloc_num(void);
> -
> void
> -check_leaks(int supposed_allocs, int supposed_fds);
> +check_fd_leaks(int supposed_fds);
> 
> /*
>  * set/reset the timeout in seconds. The timeout starts
> @@ -89,10 +86,10 @@ test_sleep(unsigned int);
> void
> test_disable_coredumps(void);
> 
> -#define DISABLE_LEAK_CHECKS			\
> -	do {					\
> -		extern int leak_check_enabled;	\
> -		leak_check_enabled = 0;		\
> +#define DISABLE_LEAK_CHECKS				\
> +	do {						\
> +		extern int fd_leak_check_enabled;	\
> +		fd_leak_check_enabled = 0;		\
> 	} while (0);
> 
> #endif
> -- 
> 2.17.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Hi Ian,

On Wed, 29 Aug 2018 at 07:45, Ray, Ian (GE Healthcare) <ian.ray@ge.com> wrote:
> > On 29 Aug 2018, at 9.17, Daniel Stone <daniels@collabora.com> wrote:
> > @@ -200,7 +147,7 @@ run_test(const struct test *t)
> >               assert(sigaction(SIGALRM, &sa, NULL) == 0);
> >       }
> >
> > -     cur_alloc = get_current_alloc_num();
> > +     //cur_alloc = get_current_alloc_num();
>
> Nit: the above line could/should have been deleted.

Oops, yes, thanks for catching that.

Cheers,
Daniel
On Wed, Aug 29, 2018 at 07:17:15AM +0100, Daniel Stone wrote:
> There are far better ways to detect memory leaks, such as either
> valgrind or ASan. Having Meson makes it really easy to use these tools
> in our tests, and we can do that in CI as well.
> 
> Having these local wrappers actually completely broke ASan usage, so
> remove them in favour of using the more powerful options.
> 
> Signed-off-by: Daniel Stone <daniels@collabora.com>

series Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
   Peter

> ---
>  tests/sanity-test.c     | 66 ++--------------------------------
>  tests/test-compositor.c |  5 ++-
>  tests/test-runner.c     | 79 ++++++-----------------------------------
>  tests/test-runner.h     | 13 +++----
>  4 files changed, 20 insertions(+), 143 deletions(-)
> 
> diff --git a/tests/sanity-test.c b/tests/sanity-test.c
> index 2495a115..98beca8d 100644
> --- a/tests/sanity-test.c
> +++ b/tests/sanity-test.c
> @@ -35,7 +35,7 @@
>  
>  #include "test-compositor.h"
>  
> -extern int leak_check_enabled;
> +extern int fd_leak_check_enabled;
>  
>  TEST(empty)
>  {
> @@ -83,71 +83,11 @@ FAIL_TEST(sanity_assert)
>  	assert(0);
>  }
>  
> -FAIL_TEST(sanity_malloc_direct)
> -{
> -	void *p;
> -
> -	assert(leak_check_enabled);
> -
> -	p = malloc(10);	/* memory leak */
> -	assert(p);	/* assert that we got memory, also prevents
> -			 * the malloc from getting optimized away. */
> -	free(NULL);	/* NULL must not be counted */
> -	test_disable_coredumps();
> -}
> -
> -TEST(disable_leak_checks)
> -{
> -	volatile void *mem;
> -	assert(leak_check_enabled);
> -	/* normally this should be on the beginning of the test.
> -	 * Here we need to be sure, that the leak checks are
> -	 * turned on */
> -	DISABLE_LEAK_CHECKS;
> -
> -	mem = malloc(16);
> -	assert(mem);
> -}
> -
> -FAIL_TEST(sanity_malloc_indirect)
> -{
> -	struct wl_array array;
> -
> -	assert(leak_check_enabled);
> -
> -	wl_array_init(&array);
> -
> -	/* call into library that calls malloc */
> -	wl_array_add(&array, 14);
> -
> -	/* not freeing array, must leak */
> -
> -	test_disable_coredumps();
> -}
> -
> -FAIL_TEST(tc_client_memory_leaks)
> -{
> -	struct display *d = display_create();
> -	client_create_noarg(d, sanity_malloc_direct);
> -	display_run(d);
> -	test_disable_coredumps();
> -	display_destroy(d);
> -}
> -
> -FAIL_TEST(tc_client_memory_leaks2)
> -{
> -	struct display *d = display_create();
> -	client_create_noarg(d, sanity_malloc_indirect);
> -	display_run(d);
> -	test_disable_coredumps();
> -	display_destroy(d);
> -}
> -
>  FAIL_TEST(sanity_fd_leak)
>  {
>  	int fd[2];
>  
> -	assert(leak_check_enabled);
> +	assert(fd_leak_check_enabled);
>  
>  	/* leak 2 file descriptors */
>  	if (pipe(fd) < 0)
> @@ -185,7 +125,7 @@ sanity_fd_no_leak(void)
>  {
>  	int fd[2];
>  
> -	assert(leak_check_enabled);
> +	assert(fd_leak_check_enabled);
>  
>  	/* leak 2 file descriptors */
>  	if (pipe(fd) < 0)
> diff --git a/tests/test-compositor.c b/tests/test-compositor.c
> index 0631f614..72f63515 100644
> --- a/tests/test-compositor.c
> +++ b/tests/test-compositor.c
> @@ -156,7 +156,7 @@ run_client(void (*client_main)(void *data), void *data,
>  	   int wayland_sock, int client_pipe)
>  {
>  	char s[8];
> -	int cur_alloc, cur_fds;
> +	int cur_fds;
>  	int can_continue = 0;
>  
>  	/* Wait until display signals that client can continue */
> @@ -169,7 +169,6 @@ run_client(void (*client_main)(void *data), void *data,
>  	snprintf(s, sizeof s, "%d", wayland_sock);
>  	setenv("WAYLAND_SOCKET", s, 0);
>  
> -	cur_alloc = get_current_alloc_num();
>  	cur_fds = count_open_fds();
>  
>  	client_main(data);
> @@ -182,7 +181,7 @@ run_client(void (*client_main)(void *data), void *data,
>  	if (!getenv("WAYLAND_SOCKET"))
>  		cur_fds--;
>  
> -	check_leaks(cur_alloc, cur_fds);
> +	check_fd_leaks(cur_fds);
>  }
>  
>  static struct client_info *
> diff --git a/tests/test-runner.c b/tests/test-runner.c
> index 82a0a7b8..1487dc48 100644
> --- a/tests/test-runner.c
> +++ b/tests/test-runner.c
> @@ -44,16 +44,10 @@
>  
>  #include "test-runner.h"
>  
> -static int num_alloc;
> -static void* (*sys_malloc)(size_t);
> -static void (*sys_free)(void*);
> -static void* (*sys_realloc)(void*, size_t);
> -static void* (*sys_calloc)(size_t, size_t);
> -
> -/* when set to 1, check if tests are not leaking memory and opened files.
> +/* when set to 1, check if tests are not leaking opened files.
>   * It is turned on by default. It can be turned off by
>   * WAYLAND_TEST_NO_LEAK_CHECK environment variable. */
> -int leak_check_enabled;
> +int fd_leak_check_enabled;
>  
>  /* when this var is set to 0, every call to test_set_timeout() is
>   * suppressed - handy when debugging the test. Can be set by
> @@ -65,40 +59,6 @@ static int is_atty = 0;
>  
>  extern const struct test __start_test_section, __stop_test_section;
>  
> -__attribute__ ((visibility("default"))) void *
> -malloc(size_t size)
> -{
> -	num_alloc++;
> -	return sys_malloc(size);
> -}
> -
> -__attribute__ ((visibility("default"))) void
> -free(void* mem)
> -{
> -	if (mem != NULL)
> -		num_alloc--;
> -	sys_free(mem);
> -}
> -
> -__attribute__ ((visibility("default"))) void *
> -realloc(void* mem, size_t size)
> -{
> -	if (mem == NULL)
> -		num_alloc++;
> -	return sys_realloc(mem, size);
> -}
> -
> -__attribute__ ((visibility("default"))) void *
> -calloc(size_t nmemb, size_t size)
> -{
> -	if (sys_calloc == NULL)
> -		return NULL;
> -
> -	num_alloc++;
> -
> -	return sys_calloc(nmemb, size);
> -}
> -
>  static const struct test *
>  find_test(const char *name)
>  {
> @@ -156,25 +116,12 @@ sigalrm_handler(int signum)
>  	abort();
>  }
>  
> -int
> -get_current_alloc_num(void)
> -{
> -	return num_alloc;
> -}
> -
>  void
> -check_leaks(int supposed_alloc, int supposed_fds)
> +check_fd_leaks(int supposed_fds)
>  {
>  	int num_fds;
>  
> -	if (leak_check_enabled) {
> -		if (supposed_alloc != num_alloc) {
> -			fprintf(stderr, "Memory leak detected in test. "
> -				"Allocated %d blocks, unfreed %d\n", num_alloc,
> -				num_alloc - supposed_alloc);
> -			abort();
> -		}
> -
> +	if (fd_leak_check_enabled) {
>  		num_fds = count_open_fds();
>  		if (supposed_fds != num_fds) {
>  			fprintf(stderr, "fd leak detected in test. "
> @@ -183,14 +130,14 @@ check_leaks(int supposed_alloc, int supposed_fds)
>  			abort();
>  		}
>  	} else {
> -		fprintf(stderr, "Leak checks disabled\n");
> +		fprintf(stderr, "FD leak checks disabled\n");
>  	}
>  }
>  
>  static void
>  run_test(const struct test *t)
>  {
> -	int cur_alloc, cur_fds;
> +	int cur_fds;
>  	struct sigaction sa;
>  
>  	if (timeouts_enabled) {
> @@ -200,7 +147,7 @@ run_test(const struct test *t)
>  		assert(sigaction(SIGALRM, &sa, NULL) == 0);
>  	}
>  
> -	cur_alloc = get_current_alloc_num();
> +	//cur_alloc = get_current_alloc_num();
>  	cur_fds = count_open_fds();
>  
>  	t->run();
> @@ -209,7 +156,7 @@ run_test(const struct test *t)
>  	if (timeouts_enabled)
>  		alarm(0);
>  
> -	check_leaks(cur_alloc, cur_fds);
> +	check_fd_leaks(cur_fds);
>  
>  	exit(EXIT_SUCCESS);
>  }
> @@ -348,20 +295,14 @@ int main(int argc, char *argv[])
>  	int total, pass;
>  	siginfo_t info;
>  
> -	/* Load system malloc, free, and realloc */
> -	sys_calloc = dlsym(RTLD_NEXT, "calloc");
> -	sys_realloc = dlsym(RTLD_NEXT, "realloc");
> -	sys_malloc = dlsym(RTLD_NEXT, "malloc");
> -	sys_free = dlsym(RTLD_NEXT, "free");
> -
>  	if (isatty(fileno(stderr)))
>  		is_atty = 1;
>  
>  	if (is_debugger_attached()) {
> -		leak_check_enabled = 0;
> +		fd_leak_check_enabled = 0;
>  		timeouts_enabled = 0;
>  	} else {
> -		leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
> +		fd_leak_check_enabled = !getenv("WAYLAND_TEST_NO_LEAK_CHECK");
>  		timeouts_enabled = !getenv("WAYLAND_TEST_NO_TIMEOUTS");
>  	}
>  
> diff --git a/tests/test-runner.h b/tests/test-runner.h
> index 9c47a2b0..d0734009 100644
> --- a/tests/test-runner.h
> +++ b/tests/test-runner.h
> @@ -63,11 +63,8 @@ count_open_fds(void);
>  void
>  exec_fd_leak_check(int nr_expected_fds); /* never returns */
>  
> -int
> -get_current_alloc_num(void);
> -
>  void
> -check_leaks(int supposed_allocs, int supposed_fds);
> +check_fd_leaks(int supposed_fds);
>  
>  /*
>   * set/reset the timeout in seconds. The timeout starts
> @@ -89,10 +86,10 @@ test_sleep(unsigned int);
>  void
>  test_disable_coredumps(void);
>  
> -#define DISABLE_LEAK_CHECKS			\
> -	do {					\
> -		extern int leak_check_enabled;	\
> -		leak_check_enabled = 0;		\
> +#define DISABLE_LEAK_CHECKS				\
> +	do {						\
> +		extern int fd_leak_check_enabled;	\
> +		fd_leak_check_enabled = 0;		\
>  	} while (0);
>  
>  #endif
> -- 
> 2.17.1
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On 29 August 2018 at 07:17, Daniel Stone <daniels@collabora.com> wrote:
> There are far better ways to detect memory leaks, such as either
> valgrind or ASan. Having Meson makes it really easy to use these tools
> in our tests, and we can do that in CI as well.
>
> Having these local wrappers actually completely broke ASan usage, so
> remove them in favour of using the more powerful options.
>
Fwiw adding valgrind to any build-system is a 2-5 line task ;-)
Sanitizers on the other hand (esp. on shared libraries) is a pain.

-Emil
Hi,

On Wed, 29 Aug 2018 at 17:37, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 29 August 2018 at 07:17, Daniel Stone <daniels@collabora.com> wrote:
> > There are far better ways to detect memory leaks, such as either
> > valgrind or ASan. Having Meson makes it really easy to use these tools
> > in our tests, and we can do that in CI as well.
> >
> > Having these local wrappers actually completely broke ASan usage, so
> > remove them in favour of using the more powerful options.
>
> Fwiw adding valgrind to any build-system is a 2-5 line task ;-)

Sure, I'll happily review such a patch.

> Sanitizers on the other hand (esp. on shared libraries) is a pain.

Yes, I spent at least a couple of hours banging my head against that
particular brick wall last weekend, and decided not to.

Cheers,
Daniel