threading: never require logging from sync.h (take 2) #34823

nit: I don't understand why we can't easily use std::string_view for this new code. The strings get forwarded to strprintf which works well with std::string_view, so this seems like a straightforward small improvement? I don't think this introduces any new null-termination concerns that weren't already there, and I'm not sure why we need either all or none of the debugging functions to take std::string_view?

<details> <summary>git diff on 46de6787b9</summary>

diff --git a/src/sync.cpp b/src/sync.cpp
index ea5886492b..0e5c623d21 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -23,13 +23,13 @@
 #ifdef DEBUG_LOCKCONTENTION
 
 template <typename LockType>
-void ContendedLock(const char* pszName, const char* pszFile, int nLine, LockType& lock)
+void ContendedLock(std::string_view name, std::string_view file, int nLine, LockType& lock)
 {
-    LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK);
+    LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", name, file, nLine), BCLog::LOCK);
     lock.lock();
 }
-template void ContendedLock(const char* pszName, const char* pszFile, int nLine, std::unique_lock<std::mutex>& lock);
-template void ContendedLock(const char* pszName, const char* pszFile, int nLine, std::unique_lock<std::recursive_mutex>& lock);
+template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::mutex>& lock);
+template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::recursive_mutex>& lock);
 
 #endif
 
diff --git a/src/sync.h b/src/sync.h
index 9a6313d938..00fdaae942 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -80,7 +80,7 @@ inline bool LockStackEmpty() { return true; }
 #ifdef DEBUG_LOCKCONTENTION
 
 template <typename LockType>
-void ContendedLock(const char* pszName, const char* pszFile, int nLine, LockType& lock);
+void ContendedLock(std::string_view name, std::string_view file, int nLine, LockType& lock);
 #endif
 
 /**

</details>

@stickies-v I suppose there's no reason not to :)

Changed, thanks.

Leaving this as a comment for easier discussion, but unrelated to this line

An alternative approach could be to just move the UniqueLock::Enter implementation to sync.cpp, removing the need for ContendedLock altogether. It would probably prevent this function from being inlined, but compared to the cost of acquiring a lock (happy path) the extra function call should be negligible?

<details> <summary>git diff on 79467e3ec7</summary>

diff --git a/src/sync.cpp b/src/sync.cpp
index 0e5c623d21..c8b902945d 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -20,18 +20,19 @@
 #include <utility>
 #include <vector>
 
-#ifdef DEBUG_LOCKCONTENTION
-
-template <typename LockType>
-void ContendedLock(std::string_view name, std::string_view file, int nLine, LockType& lock)
+template <typename MutexType>
+void UniqueLock<MutexType>::Enter(const char* pszName, const char* pszFile, int nLine)
 {
-    LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", name, file, nLine), BCLog::LOCK);
-    lock.lock();
-}
-template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::mutex>& lock);
-template void ContendedLock(std::string_view name, std::string_view file, int nLine, std::unique_lock<std::recursive_mutex>& lock);
-
+    EnterCritical(pszName, pszFile, nLine, Base::mutex());
+#ifdef DEBUG_LOCKCONTENTION
+    if (Base::try_lock()) return;
+    LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK);
 #endif
+    Base::lock();
+}
+template void UniqueLock<Mutex>::Enter(const char*, const char*, int);
+template void UniqueLock<RecursiveMutex>::Enter(const char*, const char*, int);
+template void UniqueLock<GlobalMutex>::Enter(const char*, const char*, int);
 
 #ifdef DEBUG_LOCKORDER
 //
diff --git a/src/sync.h b/src/sync.h
index 00fdaae942..93dfe59a1e 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -73,16 +73,6 @@ inline void DeleteLock(void* cs) {}
 inline bool LockStackEmpty() { return true; }
 #endif
 
-/*
- * Called when a mutex fails to lock immediately because it is held by another
- * thread, or spuriously. Responsible for locking the lock before returning.
- */
-#ifdef DEBUG_LOCKCONTENTION
-
-template <typename LockType>
-void ContendedLock(std::string_view name, std::string_view file, int nLine, LockType& lock);
-#endif
-
 /**
  * Template mixin that adds -Wthread-safety locking annotations and lock order
  * checking to a subset of the mutex API.
@@ -153,17 +143,7 @@ class SCOPED_LOCKABLE UniqueLock : public MutexType::unique_lock
 private:
     using Base = typename MutexType::unique_lock;
 
-    void Enter(const char* pszName, const char* pszFile, int nLine)
-    {
-        EnterCritical(pszName, pszFile, nLine, Base::mutex());
-#ifdef DEBUG_LOCKCONTENTION
-        if (!Base::try_lock()) {
-            ContendedLock(pszName, pszFile, nLine, static_cast<Base&>(*this));
-        }
-#else
-        Base::lock();
-#endif
-    }
+    void Enter(const char* pszName, const char* pszFile, int nLine);
 
     bool TryEnter(const char* pszName, const char* pszFile, int nLine)
     {

</details>

I agree that losing the inlining would not be a huge deal, since a call to pthread_mutex_lock (or whatever the windows equivalent is) will likely always be emitted anyway. But I don't think we want to give up the property of sync.h being header-only, except in the obscure debugging cases.

But I don't think we want to give up the property of sync.h being header-only, except in the obscure debugging cases.

What's the benefit of a header-only sync.h, when I don't think we plan to ship util as a stand-alone library, and when the "except in the obscure debugging cases" caveat in practice doesn't make it header-only for our usage anyway?

If this is indeed a useful property, it would be good to mention that in PR description (and maybe also a brief mention in sync.h, if you're up for adding a doc commit?)

I'm fine with doing these small ad hoc changes to make working with low level headers easier. The point isn't that these should be shipped, but that they can easily be dropped into other places for experimentation and debugging (or at least that is how I used them before).