Skip to content

Commit

Permalink
Merge branch 'jk/lsan-race-ignore-false-positive'
Browse files Browse the repository at this point in the history
CI jobs that run threaded programs under LSan has been giving false
positives from time to time, which has been worked around.

This is an alternative to the jk/lsan-race-with-barrier topic with
much smaller change to the production code.

* jk/lsan-race-ignore-false-positive:
  test-lib: ignore leaks in the sanitizer's thread code
  test-lib: check leak logs for presence of DEDUP_TOKEN
  test-lib: simplify leak-log checking
  test-lib: rely on logs to detect leaks
  Revert barrier-based LSan threading race workaround
  • Loading branch information
gitster committed Jan 2, 2025
2 parents d062ccf + b119a68 commit effbef2
Show file tree
Hide file tree
Showing 6 changed files with 10 additions and 52 deletions.
7 changes: 0 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,6 @@ include shared.mak
#
# Define NO_PTHREADS if you do not have or do not want to use Pthreads.
#
# Define THREAD_BARRIER_PTHREAD if your system has pthread_barrier_t. Barrier
# support is optional and is only helpful when building with SANITIZE=leak, as
# it is used to eliminate some races in the leak-checker.
#
# Define NO_PREAD if you have a problem with pread() system call (e.g.
# cygwin1.dll before v1.5.22).
#
Expand Down Expand Up @@ -2083,9 +2079,6 @@ ifdef NO_PTHREADS
else
BASIC_CFLAGS += $(PTHREAD_CFLAGS)
EXTLIBS += $(PTHREAD_LIBS)
ifdef THREAD_BARRIER_PTHREAD
BASIC_CFLAGS += -DTHREAD_BARRIER_PTHREAD
endif
endif

ifdef HAVE_PATHS_H
Expand Down
8 changes: 0 additions & 8 deletions builtin/grep.c
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,6 @@ static pthread_cond_t cond_write;
/* Signalled when we are finished with everything. */
static pthread_cond_t cond_result;

/* Synchronize the start of all threads */
static maybe_thread_barrier_t start_barrier;

static int skip_first_line;

static void add_work(struct grep_opt *opt, struct grep_source *gs)
Expand Down Expand Up @@ -201,8 +198,6 @@ static void *run(void *arg)
int hit = 0;
struct grep_opt *opt = arg;

maybe_thread_barrier_wait(&start_barrier);

while (1) {
struct work_item *w = get_work();
if (!w)
Expand Down Expand Up @@ -234,7 +229,6 @@ static void start_threads(struct grep_opt *opt)
pthread_cond_init(&cond_add, NULL);
pthread_cond_init(&cond_write, NULL);
pthread_cond_init(&cond_result, NULL);
maybe_thread_barrier_init(&start_barrier, NULL, num_threads + 1);
grep_use_locks = 1;
enable_obj_read_lock();

Expand All @@ -254,7 +248,6 @@ static void start_threads(struct grep_opt *opt)
die(_("grep: failed to create thread: %s"),
strerror(err));
}
maybe_thread_barrier_wait(&start_barrier);
}

static int wait_all(void)
Expand Down Expand Up @@ -291,7 +284,6 @@ static int wait_all(void)
pthread_cond_destroy(&cond_add);
pthread_cond_destroy(&cond_write);
pthread_cond_destroy(&cond_result);
maybe_thread_barrier_destroy(&start_barrier);
grep_use_locks = 0;
disable_obj_read_lock();

Expand Down
6 changes: 0 additions & 6 deletions builtin/index-pack.c
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,6 @@ static pthread_mutex_t deepest_delta_mutex;

static pthread_key_t key;

static maybe_thread_barrier_t start_barrier;

static inline void lock_mutex(pthread_mutex_t *mutex)
{
if (threads_active)
Expand All @@ -211,7 +209,6 @@ static void init_thread(void)
if (show_stat)
pthread_mutex_init(&deepest_delta_mutex, NULL);
pthread_key_create(&key, NULL);
maybe_thread_barrier_init(&start_barrier, NULL, nr_threads);
CALLOC_ARRAY(thread_data, nr_threads);
for (i = 0; i < nr_threads; i++) {
thread_data[i].pack_fd = xopen(curr_pack, O_RDONLY);
Expand All @@ -234,7 +231,6 @@ static void cleanup_thread(void)
for (i = 0; i < nr_threads; i++)
close(thread_data[i].pack_fd);
pthread_key_delete(key);
maybe_thread_barrier_destroy(&start_barrier);
free(thread_data);
}

Expand Down Expand Up @@ -1104,8 +1100,6 @@ static int compare_ref_delta_entry(const void *a, const void *b)

static void *threaded_second_pass(void *data)
{
if (threads_active)
maybe_thread_barrier_wait(&start_barrier);
if (data)
set_thread_data(data);
for (;;) {
Expand Down
1 change: 0 additions & 1 deletion ci/lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,6 @@ linux-musl)
;;
linux-leaks|linux-reftable-leaks)
export SANITIZE=leak
export THREAD_BARRIER_PTHREAD=1
;;
linux-asan-ubsan)
export SANITIZE=address,undefined
Expand Down
23 changes: 10 additions & 13 deletions t/test-lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ prepend_var ASAN_OPTIONS : detect_leaks=0
export ASAN_OPTIONS

prepend_var LSAN_OPTIONS : $GIT_SAN_OPTIONS
prepend_var LSAN_OPTIONS : exitcode=0
prepend_var LSAN_OPTIONS : fast_unwind_on_malloc=0
export LSAN_OPTIONS

Expand Down Expand Up @@ -339,17 +340,6 @@ case "$TRASH_DIRECTORY" in
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac

# Utility functions using $TEST_RESULTS_* variables
nr_san_dir_leaks_ () {
# stderr piped to /dev/null because the directory may have
# been "rmdir"'d already.
find "$TEST_RESULTS_SAN_DIR" \
-type f \
-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
xargs grep -lv "Unable to get registers from thread" |
wc -l
}

# If --stress was passed, run this test repeatedly in several parallel loops.
if test "$GIT_TEST_STRESS_STARTED" = "done"
then
Expand Down Expand Up @@ -1180,8 +1170,15 @@ test_atexit_handler () {
}

check_test_results_san_file_empty_ () {
test -z "$TEST_RESULTS_SAN_FILE" ||
test "$(nr_san_dir_leaks_)" = 0
test -z "$TEST_RESULTS_SAN_FILE" && return 0

# stderr piped to /dev/null because the directory may have
# been "rmdir"'d already.
! find "$TEST_RESULTS_SAN_DIR" \
-type f \
-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
xargs grep ^DEDUP_TOKEN |
grep -qv sanitizer::GetThreadStackTopAndBottom
}

check_test_results_san_file_ () {
Expand Down
17 changes: 0 additions & 17 deletions thread-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,22 +53,5 @@ int dummy_pthread_init(void *);
int online_cpus(void);
int init_recursive_mutex(pthread_mutex_t*);

#ifdef THREAD_BARRIER_PTHREAD
#define maybe_thread_barrier_t pthread_barrier_t
#define maybe_thread_barrier_init pthread_barrier_init
#define maybe_thread_barrier_wait pthread_barrier_wait
#define maybe_thread_barrier_destroy pthread_barrier_destroy
#else
#define maybe_thread_barrier_t int
static inline int maybe_thread_barrier_init(maybe_thread_barrier_t *b UNUSED,
void *attr UNUSED,
unsigned nr UNUSED)
{
errno = ENOSYS;
return -1;
}
#define maybe_thread_barrier_wait(barrier)
#define maybe_thread_barrier_destroy(barrier)
#endif

#endif /* THREAD_COMPAT_H */

0 comments on commit effbef2

Please sign in to comment.