AssertLockHeld PRs

Wiki page to compare different PRs changing AssertLockHeld. Please edit this page!

Summary of approaches

• 1A One assert #19865: Removes runtime asserts and uses compile time static analysis, only keeping runtime asserts in cases where compile time checks don't work. Gets rid of multiple assert implementations. AssertLockHeld is the only assert and it is restored to have the same definition it had from 2018 until recently.

• 2A Two asserts #19918: Keeps runtime checks and uses two assert implementations instead of one: AssertLockHeld and WeaklyAssertLockHeld. The names are intentionally chosen so people favor the strong assertion instead of the weak assertion whenever possible, and there's never a question about which is better to use.

• PA Proper asserts #19929: Currently abandoned/closed approach that applied thread safety annotations as documented to avoid cases where the compiler might make incorrect assumptions.

• QFA Quick fix asserts #19970: Fixes usability issues in LockAssertion (so no need for unused variable names, and file/line numbers are reported correctly) and introduces a new LOCK_ASSERTION macro to wrap it.

• NA Naked asserts [1][2][3]: Avoids annotating AssertLockHeld() with any compile time attributes, leaving it pure run time. This change is orthogonal and can be combined with any of the approaches above.

Underlying issues

• Currently we have both AssertLockHeld and LockAssertion and it is confusing what the differences are between them and when each should be used.

• AssertLockHeld is currently used haphazardly in the code. The developer notes recommend that it is used every place that EXCLUSIVE_LOCK_FUNCTION is used, but the recommendation is not enforced or consistently followed. (There is also disagreement about whether the recommendation is useful on its merits. The asserts may help with readability and may help detect bugs during development when compiling locally, but they add verbosity to the code and catch fewer errors than the static checks enforced by the project's QA)

• LockAssertion class is using the EXCLUSIVE_LOCK_FUNCTION acquire annotation incorrectly: "please don't use ACQUIRE when the capability is assumed to be held previously"

• LockAssertion class has minor usability issues. It reports the wrong file/line numbers on errors, and requires naming a dummy variable.

Comparison of approaches

Advantages of 1A Approach

• One type of assert and not two. No confusion!
• Gets rid of AssertLockHeld calls which the compiler guarantees can never trigger at runtime, and which are not applied consistently in existing code
• Gets rid of LockAssertion class which is easily confused with AssertLockHeld, declares unused variable names, reports line numbers incorrectly, and is broken according to clang developers
• Falls back to runtime checks infrequently only where compile time checks don't work, and only requires a single assert macro AssertLockHeld
• Fixes LockAssertion annotation bug and usability problems by deleting LockAssertion

Disadvantages of 1A Approach

• Will only detect problems locally if using Clang and configured with --enable-debug (not enabled by default). Checks are enforced on every PR and on the master branch in QA.
• Problems are reported in the form of compile time warnings which can be missed unless configured with --enable-werror (not enabled by default locally, but enabled in QA).
• May not detect problems if Clang has bugs. Clang is spooky. There is some strange behavior that the amount of warnings produced depends on the order of the attributes and static thread analysis has known limitations.
• Despite being a 3-line scripted diff, it is an intrusive patch that touches lots of code.

Advantages of 2A Approach

• Despite still requiring two different assert implementations AssertLockHeld and WeaklyAssertLockHeld, it at least names them consistently, and tries to nudge in direction of avoiding the weaker assert in cases where the stronger assert can be used.
• It documents previously undocumented assert functions to explain what each does
• Unlike 1A approach, it does not drop runtime checks. This means if compile time checking is broken or disabled and thread sanitizer is broken or disabled, there is an extra level of checking
• Redundant AssertLockHeld calls may help with readability because unlike EXCLUSIVE_LOCKS_REQUIRED annotations you can see them in the body of the function, not just attached to the function declaration.
• Like 1A approach, fixes LockAssertion annotation bug and usability problems by deleting LockAssertion

Disadvantages of 2A Approach

• Requires two different assert implementations instead of one.

• Unlike 1A approach, does not clean up inconsistent runtime assertions in current code. It keeps developer notes recommendation to add them more places.

• WeaklyAssertLockHeld name may be confusing. Since both asserts do exactly the same thing at runtime and only compile time annotations differ, different naming schemes are possible. Feel free to add suggestions below:

  	ASSERT_EXCLUSIVE_LOCK assert	EXCLUSIVE_LOCKS_REQUIRED assert	Naked assert
  Pre-#13423			AssertLockHeld
  Post-#13423	AssertLockHeld
  Post-#14437	AssertLockHeld & LockAnnotation*
  Post-#16034	AssertLockHeld & LockAssertion*
  Post-#19668	LockAssertion	AssertLockHeld
  1A approach	AssertLockHeld
  2A approach	WeaklyAssertLockHeld	AssertLockHeld
  QFA approach	LOCK_ASSERTION*	AssertLockHeld
  Alternate suggestion	LOCK_ALREADY_HELD	AssertLockHeld
  Alternate suggestion	RuntimeAssertLockHeld	AssertLockHeld
  Alternate suggestion	RuntimeAssertLockHeld	CompileTimeAssertLockHeld
  Alternate suggestion	AssertLockHeld	RedundantlyAssertLockHeld
  Alternate suggestion	UnprovenAssertLockHeld	ProvenAssertLockHeld
  Alternate suggestion	UnsafelyAssertLockHeld	AssertLockHeld
  Alternate suggestion	UnprovablyAssertLockHeld	AssertLockHeld
  Other suggestions?

  (*) LockAnnotation, LockAssertion, and LOCK_ASSERTION are shown in the ASSERT_EXCLUSIVE_LOCK column of this table because they emulate ASSERT_EXCLUSIVE_LOCK functions. But the emulation isn't 100%, and has a few differences (see QFA Approach sections below for details)

Advantages of PA Approach

• Uses thread safety annotations as documented and potentially avoids problems with false assumptions made by new compilers or future compiler versions

Disadvantages of PA Approach

• Various practical drawbacks #19929 (comment)

Advantages of QFA Approach

• Addresses LockAssertion usability issues
• Unlike other approaches which remove LockAssertion class replacing with ASSERT_EXCLUSIVE_LOCK function, keeping the class has some advantages
  • LockAssertion gives a compile error if it's used unnecessarily (init.cpp:1548:13: error: acquiring mutex 'cs_main' that is already held [-Werror,-Wthread-safety-analysis] ... init.cpp:1547:64: note: mutex acquired here)
  • LockAssertion doesn't hide errors in some cases that we probably don't care about (assertion is inside a try block, an exception prior to the assertion is caught via a catch clause and the lock might not be held via that path, and something needing the lock is accessed after the try/catch block)
• Fixes easy bugs quickly
• Doesn't make anything worse

Disadvantages of QFA Approach

• Doesn't address LockAssertion annotation misuse
• Additional changes needed / rebasing needed on other PRs.

Advantages of NA Approach

• Conceptually simpler to keep run-time and compile-time checks separate and allow AssertLockHeld to be use freely in any context without having an impact on the compiler's static analysis

Disadvantages of NA Approach

• Hurts readability. If AssertLockHeld is annotated with ASSERT_EXCLUSIVE_LOCK, a reader can be sure that the compiler has verified the lock is held at the point of the assert. Without the annotation, the assert statement is an unproven claim only checked by runtime tests

• Less safe. AssertLockHeld has a greater likelihood of aborting if the compiler hasn't already proven the lock is held where it's called