Merge pull request #2292 from jamescowens/fix_gcc10_compile_warnings

refactor: Fix advanced compiler warnings

configure.ac

@@ -179,6 +179,12 @@ AC_ARG_ENABLE([glibc-back-compat],
   [use_glibc_compat=$enableval],
   [use_glibc_compat=no])
 
+AC_ARG_ENABLE([threadlocal],
+  [AS_HELP_STRING([--enable-threadlocal],
+  [enable features that depend on the c++ thread_local keyword (currently just thread names in debug logs). (default is to enabled if there is platform support and glibc-back-compat is not enabled)])],
+  [use_thread_local=$enableval],
+  [use_thread_local=auto])
+
 AC_ARG_ENABLE([asm],
   [AS_HELP_STRING([--disable-asm],
   [disable assembly routines (enabled by default)])],
@@ -229,7 +235,6 @@ if test "x$enable_werror" = "xyes"; then
   if test "x$CXXFLAG_WERROR" = "x"; then
     AC_MSG_ERROR("enable-werror set but -Werror is not usable")
   fi
-  AX_CHECK_COMPILE_FLAG([-Werror=gnu],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=gnu"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=vla],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=vla"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=shadow-field],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=shadow-field"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Werror=switch],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=switch"],,[[$CXXFLAG_WERROR]])
@@ -246,7 +251,6 @@ fi
 if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wall],[CXXFLAGS="$CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wextra],[CXXFLAGS="$CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
-  AX_CHECK_COMPILE_FLAG([-Wgnu],[CXXFLAGS="$CXXFLAGS -Wgnu"],,[[$CXXFLAG_WERROR]])
   dnl some compilers will ignore -Wformat-security without -Wformat, so just combine the two here.
   AX_CHECK_COMPILE_FLAG([-Wformat -Wformat-security],[CXXFLAGS="$CXXFLAGS -Wformat -Wformat-security"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wvla],[CXXFLAGS="$CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
@@ -782,6 +786,48 @@ AC_LINK_IFELSE([AC_LANG_SOURCE([
   ]
 )
 
+dnl thread_local is currently disabled when building with glibc back compat.
+dnl Our minimum supported glibc is 2.17, however support for thread_local
+dnl did not arrive in glibc until 2.18.
+if test "x$use_thread_local" = xyes || { test "x$use_thread_local" = xauto && test "x$use_glibc_compat" = xno; }; then
+  TEMP_LDFLAGS="$LDFLAGS"
+  LDFLAGS="$TEMP_LDFLAGS $PTHREAD_CFLAGS"
+  AC_MSG_CHECKING([for thread_local support])
+  AC_LINK_IFELSE([AC_LANG_SOURCE([
+    #include <thread>
+    static thread_local int foo = 0;
+    static void run_thread() { foo++;}
+    int main(){
+    for(int i = 0; i < 10; i++) { std::thread(run_thread).detach();}
+    return foo;
+    }
+    ])],
+    [
+     case $host in
+       *mingw*)
+          dnl mingw32's implementation of thread_local has also been shown to behave
+          dnl erroneously under concurrent usage; see:
+          dnl https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605
+          AC_MSG_RESULT(no)
+          ;;
+        *freebsd*)
+          dnl FreeBSD's implementation of thread_local is also buggy (per
+          dnl https://groups.google.com/d/msg/bsdmailinglist/22ncTZAbDp4/Dii_pII5AwAJ)
+          AC_MSG_RESULT(no)
+          ;;
+        *)
+          AC_DEFINE(HAVE_THREAD_LOCAL,1,[Define if thread_local is supported.])
+          AC_MSG_RESULT(yes)
+          ;;
+      esac
+    ],
+    [
+      AC_MSG_RESULT(no)
+    ]
+  )
+  LDFLAGS="$TEMP_LDFLAGS"
+fi
+
 dnl check for gmtime_r(), fallback to gmtime_s() if that is unavailable
 dnl fail if neither are available.
 AC_MSG_CHECKING(for gmtime_r)

src/consensus/tx_verify.cpp

@@ -6,7 +6,7 @@
 
 #include <primitives/transaction.h>
 
-bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
+bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
     AssertLockHeld(cs_main);
     // Time based nLockTime implemented in 0.1.6

src/crypter.cpp

@@ -132,3 +132,17 @@ bool DecryptSecret(const CKeyingMaterial& vMasterKey, const std::vector<unsigned
         return false;
     return cKeyCrypter.Decrypt(vchCiphertext, *((CKeyingMaterial*)&vchPlaintext));
 }
+
+bool DecryptKey(const CKeyingMaterial& vMasterKey, const std::vector<unsigned char>& vchCryptedSecret, const CPubKey& vchPubKey, CKey& key)
+{
+    CKeyingMaterial vchSecret;
+    if(!DecryptSecret(vMasterKey, vchCryptedSecret, vchPubKey.GetHash(), vchSecret))
+        return false;
+
+    if (vchSecret.size() != 32)
+        return false;
+
+    key.SetPubKey(vchPubKey);
+    key.SetSecret(vchSecret);
+    return (key.GetPubKey() == vchPubKey);
+}

src/crypter.h

@@ -121,4 +121,5 @@ class CCrypter
 
 bool EncryptSecret(CKeyingMaterial& vMasterKey, const CSecret &vchPlaintext, const uint256& nIV, std::vector<unsigned char> &vchCiphertext);
 bool DecryptSecret(const CKeyingMaterial& vMasterKey, const std::vector<unsigned char> &vchCiphertext, const uint256& nIV, CSecret &vchPlaintext);
+bool DecryptKey(const CKeyingMaterial& vMasterKey, const std::vector<unsigned char>& vchCryptedSecret, const CPubKey& vchPubKey, CKey& key);
 #endif

src/gridcoin/gridcoin.cpp

@@ -4,6 +4,7 @@
 
 #include "chainparams.h"
 #include "main.h"
+#include "util/threadnames.h"
 #include "gridcoin/backup.h"
 #include "gridcoin/contract/contract.h"
 #include "gridcoin/gridcoin.h"
@@ -218,6 +219,9 @@ void InitializeResearcherContext()
 //!
 void ThreadScraper(void* parg)
 {
+    RenameThread("grc-scraper");
+    util::ThreadSetInternalName("grc-scraper");
+
     LogPrint(BCLog::LogFlags::NOISY, "ThreadSraper starting");
 
     try {
@@ -246,6 +250,9 @@ void ThreadScraper(void* parg)
 //!
 void ThreadScraperSubscriber(void* parg)
 {
+    RenameThread("grc-scrapersubscriber");
+    util::ThreadSetInternalName("grc-scrapersubscriber");
+
     LogPrint(BCLog::LogFlags::NOISY, "ThreadScraperSubscriber starting");
 
     try {

src/gridcoin/gridcoin.h

@@ -4,7 +4,8 @@
 
 #pragma once
 
-class CBlockIndex;
+#include "fwd.h"
+
 class CScheduler;
 
 namespace GRC {

src/gridcoin/magnitude.h

@@ -150,7 +150,7 @@ class Magnitude
 
     bool operator==(const int64_t other) const
     {
-        return static_cast<int64_t>(m_scaled) == other * SCALE_FACTOR;
+        return static_cast<int64_t>(m_scaled) == other * static_cast<int64_t>(SCALE_FACTOR);
     }
 
     bool operator!=(const int64_t other) const

src/gridcoin/researcher.cpp

@@ -475,7 +475,7 @@ class RecentBeacons
     //!
     //! \param beacons Contains the set of pending beacons to import from.
     //!
-    void ImportRegistry(const BeaconRegistry& beacons)
+    void ImportRegistry(const BeaconRegistry& beacons) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pwalletMain->cs_wallet)
     {
         AssertLockHeld(cs_main);
         AssertLockHeld(pwalletMain->cs_wallet);
@@ -498,7 +498,7 @@ class RecentBeacons
     //!
     //! \return A pointer to the pending beacon if one exists for the CPID.
     //!
-    const PendingBeacon* Try(const Cpid cpid)
+    const PendingBeacon* Try(const Cpid cpid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
     {
         AssertLockHeld(cs_main);
 
@@ -531,7 +531,7 @@ class RecentBeacons
     //! \param cpid   CPID that the beacon was advertised for.
     //! \param result Contains the public key if the transaction succeeded.
     //!
-    void Remember(const Cpid cpid, const AdvertiseBeaconResult& result)
+    void Remember(const Cpid cpid, const AdvertiseBeaconResult& result) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pwalletMain->cs_wallet)
     {
         AssertLockHeld(cs_main);
 

src/gridcoin/scraper/scraper.cpp

@@ -44,6 +44,13 @@ fs::path pathScraper = {};
 
 extern bool fShutdown;
 extern CWallet* pwalletMain;
+
+CCriticalSection cs_Scraper;
+CCriticalSection cs_StructScraperFileManifest;
+CCriticalSection cs_ConvergedScraperStatsCache;
+CCriticalSection cs_TeamIDMap;
+CCriticalSection cs_VerifiedBeacons;
+
 bool fScraperActive = false;
 std::vector<std::pair<std::string, std::string>> vuserpass;
 std::vector<std::pair<std::string, int64_t>> vprojectteamids;
@@ -84,20 +91,20 @@ struct ScraperFileManifest
 // Both TeamIDMap and ProjTeamETags are protected by cs_TeamIDMap.
 // --------------- project -------------team name -- teamID
 typedef std::map<std::string, std::map<std::string, int64_t>> mTeamIDs;
-mTeamIDs TeamIDMap;
+mTeamIDs TeamIDMap GUARDED_BY(cs_TeamIDMap);
 
 // ProjTeamETags is not persisted to disk. There would be little to be gained by doing so. The scrapers are restarted very
 // rarely, and on restart, this would only save downloading team files for those projects that have one or TeamIDs missing AND
 // an ETag had NOT changed since the last pull. Not worth the complexity.
 // --------------- project ---- eTag
 typedef std::map<std::string, std::string> mProjectTeamETags;
-mProjectTeamETags ProjTeamETags;
+mProjectTeamETags ProjTeamETags GUARDED_BY(cs_TeamIDMap);
 
 
 std::vector<std::string> GetTeamWhiteList();
 
 std::string urlsanity(const std::string& s, const std::string& type);
-ScraperFileManifest StructScraperFileManifest = {};
+ScraperFileManifest StructScraperFileManifest GUARDED_BY(cs_StructScraperFileManifest) = {};
 
 // Although scraper_net.h declares these maps, we define them here instead of
 // in scraper_net.cpp to ensure that the executable destroys these objects in
@@ -109,13 +116,7 @@ std::map<uint256, CSplitBlob::CPart> CSplitBlob::mapParts;
 std::map<uint256, std::shared_ptr<CScraperManifest>> CScraperManifest::mapManifest;
 
 // Global cache for converged scraper stats. Access must be with the lock cs_ConvergedScraperStatsCache taken.
-ConvergedScraperStats ConvergedScraperStatsCache = {};
-
-CCriticalSection cs_Scraper;
-CCriticalSection cs_StructScraperFileManifest;
-CCriticalSection cs_ConvergedScraperStatsCache;
-CCriticalSection cs_TeamIDMap;
-CCriticalSection cs_VerifiedBeacons;
+ConvergedScraperStats ConvergedScraperStatsCache GUARDED_BY(cs_ConvergedScraperStatsCache) = {};
 
 void _log(logattribute eType, const std::string& sCall, const std::string& sMessage);
 
@@ -176,17 +177,17 @@ extern UniValue SuperblockToJson(const Superblock& superblock);
 namespace {
 //!
 //! \brief Get the block index for the height to consider beacons eligible for
-//! rewards.
+//! rewards. A lock must be held on cs_main before calling this function.
 //!
 //! \return Block index for the height of a block about 1 hour below the chain
 //! tip.
 //!
-const CBlockIndex* GetBeaconConsensusHeight()
+const CBlockIndex* GetBeaconConsensusHeight() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
-    static BlockFinder block_finder;
-
     AssertLockHeld(cs_main);
 
+    static BlockFinder block_finder;
+
     // Use 4 times the BLOCK_GRANULARITY which moves the consensus block every hour.
     // TODO: Make the mod a function of SCRAPER_CMANIFEST_RETENTION_TIME in scraper.h.
     return block_finder.FindByHeight(
@@ -280,7 +281,7 @@ BeaconConsensus GetConsensusBeaconList()
 // the only one to survive, which is fine. We only need one.
 
 // This map has to be global because the scraper function is reentrant.
-ScraperVerifiedBeacons g_verified_beacons;
+ScraperVerifiedBeacons g_verified_beacons GUARDED_BY(cs_VerifiedBeacons);
 
 // Use of this global should be protected by a lock on cs_VerifiedBeacons
 ScraperVerifiedBeacons& GetVerifiedBeacons()
@@ -1863,7 +1864,7 @@ bool DownloadProjectTeamFiles(const WhitelistSnapshot& projectWhitelist)
 
 // Note this should be called with a lock held on cs_TeamIDMap, which is intended to protect both
 // TeamIDMap and ProjTeamETags.
-bool ProcessProjectTeamFile(const std::string& project, const fs::path& file, const std::string& etag)
+bool ProcessProjectTeamFile(const std::string& project, const fs::path& file, const std::string& etag) EXCLUSIVE_LOCKS_REQUIRED(cs_TeamIDMap)
 {
     std::map<std::string, int64_t> mTeamIdsForProject;
 
@@ -2448,7 +2449,7 @@ uint256 GetFileHash(const fs::path& inputfile)
 
 
 // Note that cs_StructScraperFileManifest needs to be taken before calling.
-uint256 GetmScraperFileManifestHash()
+uint256 GetmScraperFileManifestHash() EXCLUSIVE_LOCKS_REQUIRED(cs_StructScraperFileManifest)
 {
     uint256 nHash;
     CDataStream ss(SER_NETWORK, 1);
@@ -2752,7 +2753,7 @@ bool StoreTeamIDList(const fs::path& file)
 
 
 // Insert entry into Manifest. Note that cs_StructScraperFileManifest needs to be taken before calling.
-bool InsertScraperFileManifestEntry(ScraperFileManifestEntry& entry)
+bool InsertScraperFileManifestEntry(ScraperFileManifestEntry& entry) EXCLUSIVE_LOCKS_REQUIRED(cs_StructScraperFileManifest)
 {
     // This less readable form is so we know whether the element already existed or not.
     std::pair<ScraperFileManifestMap::iterator, bool> ret;
@@ -2773,7 +2774,7 @@ bool InsertScraperFileManifestEntry(ScraperFileManifestEntry& entry)
 }
 
 // Delete entry from Manifest and corresponding file if it exists. Note that cs_StructScraperFileManifest needs to be taken before calling.
-unsigned int DeleteScraperFileManifestEntry(ScraperFileManifestEntry& entry)
+unsigned int DeleteScraperFileManifestEntry(ScraperFileManifestEntry& entry) EXCLUSIVE_LOCKS_REQUIRED(cs_StructScraperFileManifest)
 {
     unsigned int ret;
 
@@ -2800,7 +2801,7 @@ unsigned int DeleteScraperFileManifestEntry(ScraperFileManifestEntry& entry)
 // Mark manifest entry non-current. The reason this is encapsulated in a function is
 // to  ensure the rehash is done. Note that cs_StructScraperFileManifest needs to be
 // taken before calling.
-bool MarkScraperFileManifestEntryNonCurrent(ScraperFileManifestEntry& entry)
+bool MarkScraperFileManifestEntryNonCurrent(ScraperFileManifestEntry& entry) EXCLUSIVE_LOCKS_REQUIRED(cs_StructScraperFileManifest)
 {
     entry.current = false;
 
@@ -4063,7 +4064,7 @@ unsigned int ScraperDeleteUnauthorizedCScraperManifests()
 
 // A lock needs to be taken on cs_StructScraperFileManifest for this function.
 // The sCManifestName is the public key of the scraper in address form.
-bool ScraperSendFileManifestContents(CBitcoinAddress& Address, CKey& Key)
+bool ScraperSendFileManifestContents(CBitcoinAddress& Address, CKey& Key) EXCLUSIVE_LOCKS_REQUIRED(cs_StructScraperFileManifest)
 {
     // This "broadcasts" the current ScraperFileManifest contents to the network.
 
@@ -5303,8 +5304,13 @@ UniValue sendscraperfilemanifest(const UniValue& params, bool fHelp)
     bool ret;
     if (IsScraperAuthorizedToBroadcastManifests(AddressOut, KeyOut))
     {
+        LOCK(cs_StructScraperFileManifest);
+        _log(logattribute::INFO, "LOCK", "cs_StructScraperFileManifest");
+
         ret = ScraperSendFileManifestContents(AddressOut, KeyOut);
         uiInterface.NotifyScraperEvent(scrapereventtypes::Manifest, CT_NEW, {});
+
+        _log(logattribute::INFO, "ENDLOCK", "cs_StructScraperFileManifest");
     }
     else
         ret = false;
@@ -5525,6 +5531,8 @@ UniValue testnewsb(const UniValue& params, bool fHelp)
 
     if (params.size() == 1) nReducedCacheBits = params[0].get_int();
 
+    UniValue res(UniValue::VOBJ);
+
     {
         LOCK(cs_ConvergedScraperStatsCache);
 
@@ -5535,12 +5543,10 @@ UniValue testnewsb(const UniValue& params, bool fHelp)
 
 		    return error;
 		}
-    }
 
-    UniValue res(UniValue::VOBJ);
-
-    _log(logattribute::INFO, "testnewsb", "Size of the PastConvergences map = " + ToString(ConvergedScraperStatsCache.PastConvergences.size()));
-    res.pushKV("Size of the PastConvergences map", ToString(ConvergedScraperStatsCache.PastConvergences.size()));
+        _log(logattribute::INFO, "testnewsb", "Size of the PastConvergences map = " + ToString(ConvergedScraperStatsCache.PastConvergences.size()));
+        res.pushKV("Size of the PastConvergences map", ToString(ConvergedScraperStatsCache.PastConvergences.size()));
+    }
 
     // Contract binary pack/unpack check...
     _log(logattribute::INFO, "testnewsb", "Checking compatibility with binary SB pack/unpack by packing then unpacking, then comparing to the original");

src/gridcoin/superblock.h

@@ -22,7 +22,7 @@ extern std::vector<uint160> GetVerifiedBeaconIDs(const ScraperPendingBeaconMap&
 extern ScraperStatsAndVerifiedBeacons GetScraperStatsByConvergedManifest(const ConvergedManifest& StructConvergedManifest);
 
 class CBlockIndex;
-class ConvergedScraperStats; // Forward for Superblock
+struct ConvergedScraperStats; // Forward for Superblock
 
 namespace GRC {
 class Superblock; // Forward for QuorumHash

src/gridcoinresearchd.cpp

@@ -10,6 +10,7 @@
 #include "chainparams.h"
 #include "chainparamsbase.h"
 #include "util.h"
+#include "util/threadnames.h"
 #include "net.h"
 #include "txdb.h"
 #include "wallet/walletdb.h"
@@ -37,6 +38,8 @@ bool AppInit(int argc, char* argv[])
 #endif
 
     SetupEnvironment();
+    util::ThreadSetInternalName("gridcoinresearchd-main");
+
     SetupServerArgs();
 
     // Note every function above the InitLogging() call must use tfm::format or similar.