Page MenuHomePhabricator

strengthen AssertLockNotHeld assertions
ClosedPublic

Authored by PiRK on Nov 29 2023, 15:46.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC8431faade189: strengthen AssertLockNotHeld assertions
Summary

This changes AssertLockNotHeld so that it is annotated with the negative capability for the mutex it refers to. clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

Note that this can't reasonably be used for globals, because clang would require every function to be annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex) for each global mutex. At present, the only global mutexes that use AssertLockNotHeld are RecursiveMutex so we treat that as an exception in order to avoid having to add an excessive number of negative annotations.

This is a backport of core#25109

Notes:

  • the changes in versionbits.h are not relevant to us because we removed most of the BIP9 related code
  • we have two variants of Misbehaving to annotate because of D384
  • m_sock_mutex is still called cs_hSocket in our codebase because of a missing backport
  • it makes sens to annotate RelayProof for the same reason as RelayTransaction
  • ReceivedAvalancheProof needs to be annotated because it calls Misbehaving and RelayProof
Test Plan

With clang and Debug
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Nov 29 2023, 15:46
PiRK planned changes to this revision.Nov 29 2023, 15:49

Let's run the debug and tsan build a few times on CI to be sure this works consistently.

@bot build-debug build-tsan

PiRK requested review of this revision.Nov 29 2023, 16:23

@bot build-debug build-tsan

This revision is now accepted and ready to land.Nov 30 2023, 09:14
This revision was automatically updated to reflect the committed changes.