Skip to content

Introduce --with-tsan to compile with Clang ThreadSanitizer#2343

Merged
bapt merged 1 commit into
freebsd:mainfrom
kevemueller:tsan
Nov 13, 2024
Merged

Introduce --with-tsan to compile with Clang ThreadSanitizer#2343
bapt merged 1 commit into
freebsd:mainfrom
kevemueller:tsan

Conversation

@kevemueller
Copy link
Copy Markdown

pkg repo uses multiple threads to speed up the repository creation process. On MacOS with multiple workers this is predictably crashing pkg or generating unparsable repository artefacts. Introduce --with-tsan to compile with Clang ThreadSanitizer.

While here, make the description of the configure options more clear and remove -ggdb in favour of -g.

pkg repo uses multiple threads to speed up the repository creation process. On MacOS with multiple workers this is predictably crashing ```pkg``` or generating unparsable repository artefacts.
Introduce --with-tsan to compile with Clang ThreadSanitizer.

While here, make the description of the configure options more clear and remove ```-ggdb``` in favour of ```-g```.
@kevemueller
Copy link
Copy Markdown
Author

I would be interested to see if --with-tsan on FreeBSD produces races when using pkg repo.
Both Ubuntu as well as FreeBSD don't seem to have an issue with the code (pkg repo works fine), but MacOS conistently fails even with the simplest calls e.g. from the unit tests.

On MacOS ThreadSanitizer seems to produce valid remarks, but a thorough analysis has yet to be performed.

@bapt
Copy link
Copy Markdown
Member

bapt commented Nov 13, 2024

seems like tsan does not even build on FreeBSD, I have duplicated symbols for things in libc and libpthread.

@kevemueller
Copy link
Copy Markdown
Author

:(
I might need to add a "developer" CI build to GitHub for Ubuntu/MacOS. Had no issues locally.

@bapt
Copy link
Copy Markdown
Member

bapt commented Nov 13, 2024

if needed we can switch back to the fork model we used before, we converted to thread just to simplify the code

@kevemueller
Copy link
Copy Markdown
Author

Absolutely NOT my intention with this.
Technically this could be merged regardless and the topics arising followed-up on later, but we might as well wait until I can prove that --sanitize=thread works fine on another system like Ubuntu.
Even if this is never used on FreeBSD, threading issues detected on other environments contribute to fixing the code on all environments.

@bapt
Copy link
Copy Markdown
Member

bapt commented Nov 13, 2024

fully agreed, in the mean time I have opened a bug in FreeBSD for us to fix tsan on our side

@kevemueller
Copy link
Copy Markdown
Author

Just compiled using --with-tsan without issues on Ubuntu jammy (22.04). So the changes proposed here are "working".

All tests ran fine using the pkg built with tsan. So I will need to read-up on pthread issues with MacOS....

@bapt bapt merged commit b42190e into freebsd:main Nov 13, 2024
@bapt
Copy link
Copy Markdown
Member

bapt commented Nov 13, 2024

I don't know anything about threads in MacOS, but if the C11 threads are saner to use on MacOS, I am fine with pkg using them instead of pthreads.

@kevemueller
Copy link
Copy Markdown
Author

There is nothing wrong with pthreads in MacOS, unfortunately the pkg code needs to be improved. You are not initializing your mutexes, which means they don't work the way you expect them to work.
Funny enough this has no consequences on Linux/FreeBSD.

Once properly initialized, the pkg_repo code shows the usual symptoms of multi-threaded code: deadlocks and data races in emitting the UCL fragments.

I have nothing to commit right now, am trying to deal with the fallout of adding

	/* initialize mutexes & conditions */
	pthread_mutex_init(&te->nlock, 0);
	pthread_mutex_init(&te->llock, 0);
	pthread_mutex_init(&te->flock, 0);
	pthread_cond_init(&te->cond, 0);

in pkg_repo_create.c.
Same behaviour on Linux/MacOS - pkg repo not working.
Initial work is in https://github.com/kevemueller/pkg/tree/refs/heads/fixthread and corresponding CI run at https://github.com/kevemueller/pkg/actions/runs/11820285644

I might need to park this though, just to manage your expectations. WORKERS_COUNT=1 is just so much easier to type than threaded code to fix.

@kevemueller kevemueller deleted the tsan branch November 13, 2024 16:20
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.

2 participants