From e8ea2aebe7af3572802c484ec1361dc1b04c16c1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 26 Aug 2019 09:32:39 +0300 Subject: [PATCH 1/9] gerror: Add a docs paragraph about not displaying errors verbatim in UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s confusing and often doesn’t help the user. Match the error code and come up with a more UI-appropriate error message. Signed-off-by: Philip Withnall --- glib/gerror.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/glib/gerror.c b/glib/gerror.c index b190d6881..cf20bc8c0 100644 --- a/glib/gerror.c +++ b/glib/gerror.c @@ -114,6 +114,13 @@ * function (the file being opened, or whatever - though in the * g_file_get_contents() case, the @message already contains a filename). * + * Note, however, that many error messages are too technical to display to the + * user in an application, so prefer to use g_error_matches() to categorize errors + * from called functions, and build an appropriate error message for the context + * within your application. Error messages from a #GError are more appropriate + * to be printed in system logs or on the command line. They are typically + * translated. + * * When implementing a function that can report errors, the basic * tool is g_set_error(). Typically, if a fatal error occurs you * want to g_set_error(), then return immediately. g_set_error() From 8d19b95bd85ecc82fccbf1248e6f333f0caeb1b4 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 22 Aug 2019 00:48:53 +0300 Subject: [PATCH 2/9] ci: Add valgrind to fedora Docker image It will be used in an upcoming commit. Signed-off-by: Philip Withnall Helps: #487 --- .gitlab-ci.yml | 10 +++++----- .gitlab-ci/fedora.Dockerfile | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b65d80020..4ccd9cb8f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -14,7 +14,7 @@ variables: MESON_COMMON_OPTIONS: "--buildtype debug --fatal-meson-warnings" fedora-x86_64: - image: registry.gitlab.gnome.org/gnome/glib/fedora:v2 + image: registry.gitlab.gnome.org/gnome/glib/fedora:v3 stage: build except: - tags @@ -84,7 +84,7 @@ debian-stable-x86_64: - "_build/${CI_JOB_NAME}-report.xml" G_DISABLE_ASSERT: - image: registry.gitlab.gnome.org/gnome/glib/fedora:v2 + image: registry.gitlab.gnome.org/gnome/glib/fedora:v3 stage: build except: - tags @@ -255,7 +255,7 @@ freebsd-12-x86_64: - "_build/${CI_JOB_NAME}-report.xml" coverage: - image: registry.gitlab.gnome.org/gnome/glib/fedora:v2 + image: registry.gitlab.gnome.org/gnome/glib/fedora:v3 stage: coverage except: - tags @@ -268,7 +268,7 @@ coverage: coverage: '/^\s+lines\.+:\s+([\d.]+\%)\s+/' scan-build: - image: registry.gitlab.gnome.org/gnome/glib/fedora:v2 + image: registry.gitlab.gnome.org/gnome/glib/fedora:v3 stage: analysis except: - tags @@ -301,7 +301,7 @@ pages: - public dist-job: - image: registry.gitlab.gnome.org/gnome/glib/fedora:v2 + image: registry.gitlab.gnome.org/gnome/glib/fedora:v3 stage: build only: - tags diff --git a/.gitlab-ci/fedora.Dockerfile b/.gitlab-ci/fedora.Dockerfile index 1c0a39d8b..9b01ccf00 100644 --- a/.gitlab-ci/fedora.Dockerfile +++ b/.gitlab-ci/fedora.Dockerfile @@ -47,6 +47,7 @@ RUN dnf -y install \ shared-mime-info \ systemtap-sdt-devel \ unzip \ + valgrind \ wget \ xz \ zlib-devel \ From 197eff3fc8c79fae20be353801f79de9a52ce387 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 26 Jun 2018 17:23:09 +0100 Subject: [PATCH 3/9] ci: Add valgrind memcheck support on Fedora MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a separate CI job which runs memcheck on the unit tests. This is done as a separate job from the main build, since we don’t want it to interact with code coverage at all. Currently, failure of this job is ignored. Issue #333 will eventually fix that. Signed-off-by: Philip Withnall Fixes: #487 --- .gitlab-ci.yml | 34 ++++++++++++++++++++++++++++++++++ .gitlab-ci/run-tests.sh | 15 +++++++++++++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4ccd9cb8f..0f091e761 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -111,6 +111,40 @@ G_DISABLE_ASSERT: - "_build/meson-logs" - "_build/${CI_JOB_NAME}-report.xml" +valgrind: + image: registry.gitlab.gnome.org/gnome/glib/fedora:v3 + stage: analysis + except: + - tags + variables: + MESON_TEST_TIMEOUT_MULTIPLIER: 10 + script: + - meson ${MESON_COMMON_OPTIONS} + --werror + -Dsystemtap=true + -Ddtrace=true + -Dfam=true + -Dinstalled_tests=true + _build + - ninja -C _build + - bash -x ./.gitlab-ci/run-tests.sh + --log-file _build/meson-logs/testlog-valgrind.json + --wrap "valgrind --tool=memcheck --error-exitcode=1 --track-origins=yes --leak-check=full --leak-resolution=high --num-callers=50 --show-leak-kinds=definite,possible --show-error-list=yes --suppressions=${CI_PROJECT_DIR}/glib.supp" + --no-suite no-valgrind + --no-suite slow + # FIXME: Remove this when we have zero valgrind leaks. + # https://gitlab.gnome.org/GNOME/glib/issues/333 + allow_failure: true + artifacts: + reports: + junit: "_build/${CI_JOB_NAME}-report.xml" + name: "glib-${CI_JOB_NAME}-${CI_COMMIT_REF_NAME}" + when: always + paths: + - "_build/config.h" + - "_build/glib/glibconfig.h" + - "_build/meson-logs" + .cross-template: &cross-template stage: build except: diff --git a/.gitlab-ci/run-tests.sh b/.gitlab-ci/run-tests.sh index ca02816f9..539726e9a 100755 --- a/.gitlab-ci/run-tests.sh +++ b/.gitlab-ci/run-tests.sh @@ -2,10 +2,21 @@ set +e +case "$1" in + --log-file) + log_file="$2" + shift + shift + ;; + *) + log_file="_build/meson-logs/testlog.json" +esac + meson test \ -C _build \ --timeout-multiplier ${MESON_TEST_TIMEOUT_MULTIPLIER} \ - --no-suite flaky + --no-suite flaky \ + "$@" exit_code=$? @@ -13,6 +24,6 @@ python3 .gitlab-ci/meson-junit-report.py \ --project-name=glib \ --job-id "${CI_JOB_NAME}" \ --output "_build/${CI_JOB_NAME}-report.xml" \ - _build/meson-logs/testlog.json + "${log_file}" exit $exit_code From 0cc745f6cbf14e38ce15862fce979bd292f9111f Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 22 Aug 2019 11:25:49 +0300 Subject: [PATCH 4/9] ci: Include stderr output in JUnit XML report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When running tests under valgrind, the valgrind summary is printed in stderr, and the TAP output is printed in stdout. The valgrind summary is useful to include in the GitLab test report, so append it to the textual failure information for failed tests. I can’t find a better XML element in the [JUnit schema](https://github.com/windyroad/JUnit-Schema/blob/master/JUnit.xsd) for representing it. Signed-off-by: Philip Withnall Helps: #487 --- .gitlab-ci/meson-junit-report.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci/meson-junit-report.py b/.gitlab-ci/meson-junit-report.py index 09b89a499..0df07271b 100755 --- a/.gitlab-ci/meson-junit-report.py +++ b/.gitlab-ci/meson-junit-report.py @@ -52,6 +52,7 @@ for line in args.infile: duration = data['duration'] return_code = data['returncode'] log = data['stdout'] + log_stderr = data.get('stderr', '') unit = { 'suite': suite_name, @@ -59,6 +60,7 @@ for line in args.infile: 'duration': duration, 'returncode': return_code, 'stdout': log, + 'stderr': log_stderr, } units = suites.setdefault(suite_name, []) @@ -103,7 +105,7 @@ for name, units in suites.items(): failure.set('classname', '{}/{}'.format(args.project_name, unit['suite'])) failure.set('name', unit['name']) failure.set('type', 'error') - failure.text = unit['stdout'] + failure.text = unit['stdout'] + '\n' + unit['stderr'] output = ET.tostring(testsuites, encoding='unicode') outfile.write(output) From a6ecfeea4c6c0ee5eec1aba47d969d53a3bf99d9 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 27 Jun 2018 10:00:56 +0100 Subject: [PATCH 5/9] =?UTF-8?q?tests:=20Don=E2=80=99t=20run=20Python=20tes?= =?UTF-8?q?ts=20under=20Valgrind?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Python runtime is not amenable to Valgrind, and leak checking is a lot less relevant in Python compared to C. Signed-off-by: Philip Withnall Helps: #487 --- gobject/tests/meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gobject/tests/meson.build b/gobject/tests/meson.build index 35ae4b033..9c6a6e034 100644 --- a/gobject/tests/meson.build +++ b/gobject/tests/meson.build @@ -117,7 +117,7 @@ foreach test_name : python_tests python, args: ['-B', files(test_name)], env: test_env, - suite: ['gobject'], + suite: ['gobject', 'no-valgrind'], ) if installed_tests_enabled From adb9264d126fdd795ca52ee5b659c33f1b9ca198 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 2 Nov 2018 10:13:01 +0000 Subject: [PATCH 6/9] glib.supp: Add leak types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mark all the memcheck leaks as ‘reachable’, so the suppressions will not apply if that memory is no longer reachable on exit(). This feature was introduced in Valgrind 3.9, and is documented here: http://valgrind.org/docs/manual/mc-manual.html#mc-manual.suppfiles Signed-off-by: Philip Withnall --- glib.supp | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/glib.supp b/glib.supp index 2c20b1238..baec2f33b 100644 --- a/glib.supp +++ b/glib.supp @@ -22,6 +22,7 @@ { gnutls-init-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:gtls_gnutls_init @@ -30,6 +31,7 @@ { gnutls-init-realloc Memcheck:Leak + match-leak-kinds:reachable fun:realloc ... fun:gtls_gnutls_init @@ -38,6 +40,7 @@ { g-tls-backend-gnutls-init Memcheck:Leak + match-leak-kinds:reachable fun:g_once_impl fun:g_tls_backend_gnutls_init } @@ -45,6 +48,7 @@ { p11-tokens-init Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:create_tokens_inlock @@ -55,6 +59,7 @@ { g-local-vfs-getpwnam Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:getpwnam @@ -64,6 +69,7 @@ { glib-init-malloc Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_quark_init @@ -74,6 +80,7 @@ { glib-init-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_quark_init @@ -84,6 +91,7 @@ { gobject-init-malloc Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:gobject_init_ctor @@ -92,6 +100,7 @@ { gobject-init-realloc Memcheck:Leak + match-leak-kinds:reachable fun:realloc ... fun:gobject_init_ctor @@ -100,6 +109,7 @@ { gobject-init-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:gobject_init_ctor @@ -108,6 +118,7 @@ { g-type-register-dynamic Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_type_register_dynamic @@ -116,6 +127,7 @@ { g-type-register-static Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_type_register_static @@ -124,6 +136,7 @@ { g-type-register-static-realloc Memcheck:Leak + match-leak-kinds:possible,reachable fun:realloc ... fun:g_type_register_static @@ -132,6 +145,7 @@ { g-type-register-static-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_type_register_static @@ -140,6 +154,7 @@ { g-type-add-interface-dynamic Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_type_add_interface_dynamic @@ -148,6 +163,7 @@ { g-type-add-interface-static Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_type_add_interface_static @@ -174,6 +190,7 @@ { g-test-rand-init Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_rand_new_with_seed_array @@ -185,6 +202,7 @@ { g-test-rand-init2 Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_rand_new_with_seed_array @@ -197,6 +215,7 @@ { g-quark-table-new Memcheck:Leak + match-leak-kinds:reachable fun:g_hash_table_new ... fun:quark_new @@ -205,6 +224,7 @@ { g-quark-table-resize Memcheck:Leak + match-leak-kinds:reachable ... fun:g_hash_table_resize ... @@ -214,6 +234,7 @@ { g-type-interface-init Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:type_iface_vtable_base_init_Wm @@ -231,6 +252,7 @@ { g-type-class-init Memcheck:Leak + match-leak-kinds:reachable fun:g_type_create_instance ... fun:type_class_init_Wm @@ -325,6 +347,7 @@ { g-io-module-default-singleton-malloc Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_type_create_instance @@ -335,6 +358,7 @@ { g-io-module-default-singleton-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_type_create_instance @@ -347,6 +371,7 @@ { g-io-module-default-singleton Memcheck:Leak + match-leak-kinds:reachable fun:g_type_create_instance ... fun:_g_io_module_get_default @@ -355,6 +380,7 @@ { g-io-module-default-singleton-module Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_module_open @@ -365,6 +391,7 @@ { g-io-module-default-singleton-name Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_strdup @@ -375,6 +402,7 @@ { g-get-language-names-malloc Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_get_language_names @@ -383,6 +411,7 @@ { g-get-language-names-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_get_language_names @@ -391,6 +420,7 @@ { g-static-mutex Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_static_mutex_get_mutex_impl @@ -399,6 +429,7 @@ { g-system-thread-init Memcheck:Leak + match-leak-kinds:possible,reachable fun:calloc ... fun:g_system_thread_new @@ -407,6 +438,7 @@ { g-io-module-default-proxy-resolver-gnome Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_proxy_resolver_gnome_init @@ -418,6 +450,7 @@ { g-threaded-resolver-getaddrinfo-config Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:__resolv_conf_allocate @@ -697,9 +730,11 @@ } # g_set_user_dirs() deliberately leaks the previous cached g_get_user_*() values. +# These will not all be reachable on exit. { g_set_user_dirs_str Memcheck:Leak + match-leak-kinds:definite,reachable fun:malloc ... fun:set_str_if_different @@ -707,9 +742,11 @@ } # g_set_user_dirs() deliberately leaks the previous cached g_get_user_*() values. +# These will not all be reachable on exit. { g_set_user_dirs_strv Memcheck:Leak + match-leak-kinds:definite,reachable fun:malloc ... fun:set_strv_if_different @@ -720,6 +757,7 @@ { g_get_system_data_dirs Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_build_system_data_dirs @@ -730,6 +768,7 @@ { g_get_user_data_dir Memcheck:Leak + match-leak-kinds:reachable fun:realloc ... fun:g_build_user_data_dir @@ -740,6 +779,7 @@ { desktop_file_dirs_malloc Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:desktop_file_dirs_lock @@ -749,6 +789,7 @@ { desktop_file_dirs_realloc Memcheck:Leak + match-leak-kinds:reachable fun:realloc ... fun:desktop_file_dirs_lock @@ -758,6 +799,7 @@ { desktop_file_dir_unindexed_setup_search Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:desktop_file_dir_unindexed_setup_search @@ -768,6 +810,7 @@ { g_io_extension_point_register Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_io_extension_point_register @@ -777,6 +820,7 @@ { g_strerror Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_locale_to_utf8 @@ -787,6 +831,7 @@ { g_socket_connection_factory_register_type Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_socket_connection_factory_register_type @@ -796,6 +841,7 @@ { g_dbus_error_quark Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_dbus_error_register_error_domain @@ -806,6 +852,7 @@ { g_private_set_alloc0 Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_private_set_alloc0 @@ -813,6 +860,7 @@ { g_private_set_alloc0-calloc Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:g_private_set_alloc0 @@ -822,6 +870,7 @@ { g_main_context_push_thread_default Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:g_queue_new @@ -832,6 +881,7 @@ { g_file_info_attribute_cache Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:ensure_attribute_hash @@ -841,6 +891,7 @@ { g_file_info_attribute_cache2 Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:ensure_attribute_hash @@ -850,6 +901,7 @@ { g_file_info_attribute_cache3 Memcheck:Leak + match-leak-kinds:reachable fun:malloc ... fun:lookup_namespace @@ -859,9 +911,10 @@ { g_file_info_attribute_cache4 Memcheck:Leak + match-leak-kinds:reachable fun:calloc ... fun:lookup_namespace ... fun:g_file_* -} \ No newline at end of file +} From c8c75dc7adab15a29287055e8ec92285f0c2aca8 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 2 Nov 2018 10:17:10 +0000 Subject: [PATCH 7/9] glib.supp: Fix some indentation Signed-off-by: Philip Withnall --- glib.supp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/glib.supp b/glib.supp index baec2f33b..d70ffb9be 100644 --- a/glib.supp +++ b/glib.supp @@ -462,11 +462,11 @@ # memcheck checks that the third argument to ioctl() is a valid pointer, but # some ioctls use that argument as an integer { - ioctl-with-non-pointer-param - Memcheck:Param - ioctl(generic) - fun:ioctl - fun:btrfs_reflink_with_progress + ioctl-with-non-pointer-param + Memcheck:Param + ioctl(generic) + fun:ioctl + fun:btrfs_reflink_with_progress } { From 8ae07a727abcc157f3ff5a5185903f737a1c3a72 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 22 Aug 2019 11:28:16 +0300 Subject: [PATCH 8/9] glib.supp: Add some fundamental type suppressions Signed-off-by: Philip Withnall --- glib.supp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/glib.supp b/glib.supp index d70ffb9be..220ec461a 100644 --- a/glib.supp +++ b/glib.supp @@ -151,6 +151,24 @@ fun:g_type_register_static } +{ + g-type-register-fundamental + Memcheck:Leak + match-leak-kinds:possible,reachable + fun:malloc + ... + fun:g_type_register_fundamental +} + +{ + g-type-register-fundamental-calloc + Memcheck:Leak + match-leak-kinds:possible,reachable + fun:calloc + ... + fun:g_type_register_fundamental +} + { g-type-add-interface-dynamic Memcheck:Leak From 39052a1cfc76d8d3a1a1888d85d3a359a6631932 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Thu, 22 Aug 2019 11:27:43 +0300 Subject: [PATCH 9/9] tests: Fix some minor memory leaks in tests Signed-off-by: Philip Withnall --- gio/tests/glistmodel.c | 1 + glib/tests/autoptr.c | 10 +++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/gio/tests/glistmodel.c b/gio/tests/glistmodel.c index b867bd54a..562037f62 100644 --- a/gio/tests/glistmodel.c +++ b/gio/tests/glistmodel.c @@ -798,6 +798,7 @@ test_store_past_end (void) g_assert_cmpint (g_list_model_get_n_items (model), ==, 1); item = g_list_model_get_item (model, 0); g_assert_nonnull (item); + g_object_unref (item); item = g_list_model_get_item (model, G_MAXUINT); g_assert_null (item); diff --git a/glib/tests/autoptr.c b/glib/tests/autoptr.c index d24ad1ed8..fccdfe55e 100644 --- a/glib/tests/autoptr.c +++ b/glib/tests/autoptr.c @@ -409,9 +409,11 @@ test_g_rec_mutex_locker (void) g_thread_join (thread); } - /* Verify that the mutex is unlocked again */ - thread = g_thread_new ("rec mutex unlocked", rec_mutex_unlocked_thread, &rec_mutex); - g_thread_join (thread); + /* Verify that the mutex is unlocked again */ + thread = g_thread_new ("rec mutex unlocked", rec_mutex_unlocked_thread, &rec_mutex); + g_thread_join (thread); + + g_rec_mutex_clear (&rec_mutex); } /* Thread function to check that an rw lock given in @data cannot be writer locked */ @@ -478,6 +480,8 @@ test_g_rw_lock_lockers (void) * the locks taken above have been correctly released. */ g_assert_true (g_rw_lock_writer_trylock (&lock)); g_rw_lock_writer_unlock (&lock); + + g_rw_lock_clear (&lock); } static void