Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Clang Thread Safety Annotations (locks-only) #1036

Merged

Conversation

jiridanek
Copy link
Contributor

Uses https://clang.llvm.org/docs/ThreadSafetyAnalysis.html (with all the limitations that stem from it, namely, works only in clang or IDE based on clangd, and the GUARDED_BY annotation cannot be used in C structs, llvm/llvm-project#20777)

How it works in action

Example of a text-only compile failure message: https://github.com/jiridanek/skupper-router/actions/runs/4555107395/jobs/8033958647#step:25:282

Example of the clangd IDE integration:

Calling a _LH function without holding the lock

warning_should_hold_lock

Unlocking something that is not locked

releasing mutex not held

Returning without unlocking

return_without_unlock

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #1036 (59e500e) into main (7c658af) will decrease coverage by 0.1%.
The diff coverage is 95.5%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1036     +/-   ##
=======================================
- Coverage   77.7%   77.7%   -0.1%     
=======================================
  Files        247     247             
  Lines      63991   64003     +12     
  Branches    5896    5896             
=======================================
- Hits       49775   49755     -20     
- Misses     11532   11548     +16     
- Partials    2684    2700     +16     
Flag Coverage Δ
pysystemtests 87.7% <ø> (-0.1%) ⬇️
pyunittests 54.6% <ø> (ø)
systemtests 71.3% <95.5%> (-0.1%) ⬇️
unittests 21.9% <87.8%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
unittests 26.0% <87.8%> (+<0.1%) ⬆️
systemtests 78.4% <95.5%> (-0.1%) ⬇️

@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from 46254fd to 298aaeb Compare March 30, 2023 20:51
@jiridanek jiridanek self-assigned this Apr 1, 2023
@jiridanek jiridanek added this to the 2.4.0 milestone Apr 1, 2023
@jiridanek jiridanek changed the title Add Clang Thread Safety Annotations (locks-only) Issue #212: Add Clang Thread Safety Annotations (locks-only) Apr 4, 2023
@jiridanek jiridanek changed the title Issue #212: Add Clang Thread Safety Annotations (locks-only) Add Clang Thread Safety Annotations (locks-only) Apr 4, 2023
@ganeshmurthy ganeshmurthy removed this from the 2.4.0 milestone Apr 12, 2023
@jiridanek
Copy link
Contributor Author

jiridanek commented May 19, 2023

@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from 298aaeb to 3ef0074 Compare May 25, 2023 18:14
@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from 3ef0074 to 6535ee6 Compare June 5, 2023 17:20
@jiridanek jiridanek added this to the 2.5.0 milestone Jun 9, 2023
@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from 6535ee6 to b132cce Compare June 16, 2023 11:56
@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from b132cce to 8d7fec9 Compare July 20, 2023 19:11
@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from 8d7fec9 to 115b4ef Compare September 19, 2023 16:29
@jiridanek jiridanek modified the milestones: 2.5.0, 2.6.0 Nov 2, 2023
@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch 2 times, most recently from 3f8b95a to 6f4cb90 Compare December 30, 2023 17:15
@jiridanek jiridanek force-pushed the jd_2023_03_30_clang_thread_safety_a branch from 728f07e to 59e500e Compare January 2, 2024 15:18
@jiridanek
Copy link
Contributor Author

I'm reasonably confident that this will be low-noise and will not be causing annoying problems.

Judging from past history, the only way to get this in is probably to just push it to main and then see whether my confidence was justified, or it does cause some problems and needs to be reverted.

I still have the previous full PR (which contains additional annotation for running on the core thread) in the queue

@ganeshmurthy
Copy link
Contributor

I'm reasonably confident that this will be low-noise and will not be causing annoying problems.

Judging from past history, the only way to get this in is probably to just push it to main and then see whether my confidence was justified, or it does cause some problems and needs to be reverted.

I still have the previous full PR (which contains additional annotation for running on the core thread) in the queue

* [Add Clang Thread Safety Annotations #212](https://github.com/skupperproject/skupper-router/pull/212)

This is a big enough change that I would at least like @kgiusti to sign off on it before this lands on main

@jiridanek jiridanek merged commit 2469371 into skupperproject:main Jan 5, 2024
42 checks passed
@jiridanek jiridanek deleted the jd_2023_03_30_clang_thread_safety_a branch January 5, 2024 19:52
@jiridanek
Copy link
Contributor Author

jiridanek commented Jan 5, 2024

This is a big enough change that I would at least like @kgiusti to sign off on it before this lands on main

He said this about it (long time ago) #212 (review)

I really think this will be low friction in practice. That was the whole point of the "reduced, locks-only" PR, after all. And if not, I'm ready with my finger on the revert button. Either way, I really want to conclude this "initiative" in some way. At least this way it will be shown if thread-safety-annotations are beneficial or painful.

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.

None yet

2 participants