Fix and check for potential ODR violations (#14096)

Summary:
... caused by public headers depending on build parameters (macro definitions). This change also adds a check under 'make check-headers' (already in CI) looking for potential future violations.

I've audited the uses of '#if' in public headers and either
* Eliminated them
* Systematically excluded them because they are intentional or similar (details in comments in check-public-header.sh
* Manually excluded them as being ODR-SAFE

In the case of ROCKSDB_USING_THREAD_STATUS, there was no good reason for this to appear in public headers so I've replaced it with a static bool ThreadStatus::kEnabled. I considered getting rid of the ability to disable this code but some relatively recent PRs have been submitted for fixing that case. I've added a release note and updated one of the CI jobs to use this build configuration. (I didn't want to combine with some jobs like no_compression and status_checked because the interaction might limit what is checked.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/14096

Test Plan: manual 'make check-headers' + manual cmake as in new CI config + CI

Reviewed By: jaykorean

Differential Revision: D86241864

Pulled By: pdillinger

fbshipit-source-id: d16addc9e3480706b174a006720a4def0740bf2e

.github/workflows/pr-jobs.yml

@@ -141,7 +141,7 @@ jobs:
         cache-hit: ${{ steps.cache-folly.outputs.cache-hit }}
     - run: "(mkdir build && cd build && cmake -DUSE_COROUTINES=1 -DWITH_GFLAGS=1 -DROCKSDB_BUILD_SHARED=0 .. && make V=1 -j20 && ctest -j20)"
     - uses: "./.github/actions/post-steps"
-  build-linux-cmake-with-benchmark:
+  build-linux-cmake-with-benchmark-no-thread-status:
     if: ${{ github.repository_owner == 'facebook' }}
     runs-on:
       labels: 16-core-ubuntu
@@ -151,7 +151,7 @@ jobs:
     steps:
     - uses: actions/checkout@v4.1.0
     - uses: "./.github/actions/pre-steps"
-    - run: mkdir build && cd build && cmake -DWITH_GFLAGS=1 -DWITH_BENCHMARK=1 .. && make V=1 -j20 && ctest -j20
+    - run: mkdir build && cd build && cmake -DWITH_GFLAGS=1 -DWITH_BENCHMARK=1 -DCMAKE_CXX_FLAGS=-DNROCKSDB_THREAD_STATUS .. && make V=1 -j20 && ctest -j20
     - uses: "./.github/actions/post-steps"
   build-linux-encrypted_env-no_compression:
     if: ${{ github.repository_owner == 'facebook' }}

Makefile

@@ -700,6 +700,7 @@ am__v_CCH_1 =
 %.h.pub: %.h # .h.pub not actually created, so re-checked on each invocation
 	$(AM_V_CCH) cd include/ && echo '#include "$(patsubst include/%,%,$<)"' | \
 	  $(CXX) -std=$(or $(ROCKSDB_CXX_STANDARD),c++20) -I. -DROCKSDB_NAMESPACE=42 -x c++ -c - -o /dev/null
+	build_tools/check-public-header.sh $<
 
 check-headers: $(HEADER_OK_FILES)
 

build_tools/check-public-header.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+# Copyright (c) Meta Platforms, Inc. and affiliates. All Rights Reserved.
+#
+# Check for some simple mistakes in public headers (on the command line)
+# that should prevent commit or push
+
+BAD=""
+
+# Look for potential for ODR violations caused by public headers depending on
+# build parameters that could vary between RocksDB build and application build.
+# * Cases like LUA, ROCKSDB_NAMESPACE, and ROCKSDB_ASSERT_STATUS_CHECKED are
+#   intentional, hard to avoid. (We expect definitions to change and the user
+#   should also.)
+# * Cases like _WIN32, OS_WIN, and __cplusplus are essentially ODR-safe.
+# * Cases like
+#   #ifdef BLAH  // ODR-SAFE
+#   #undef BLAH
+#   #endif
+#   that should not cause ODR violations can be exempted with the ODR-SAFE
+#   marker recognized here.
+
+grep -nHE '^#if' -- "$@" | grep -vE 'ROCKSDB_NAMESPACE|ROCKSDB_ASSERT_STATUS_CHECKED|LUA|_WIN32|OS_WIN|ODR-SAFE|__cplusplus|ROCKSDB_DLL|ROCKSDB_LIBRARY_EXPORTS'
+if [ "$?" != "1" ]; then
+  echo "^^^^^ #if in public API could cause an ODR violation."
+  echo "      Add // ODR-SAFE if verified safe."
+  BAD=1
+fi
+
+if [ "$BAD" ]; then
+  exit 1
+fi

db/db_impl/db_impl.cc

@@ -5506,7 +5506,7 @@ Status DBImpl::RenameTempFileToOptionsFile(const std::string& file_name,
   return s;
 }
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 
 void DBImpl::NewThreadStatusCfInfo(ColumnFamilyData* cfd) const {
   if (immutable_db_options_.enable_thread_tracking) {
@@ -5533,7 +5533,7 @@ void DBImpl::NewThreadStatusCfInfo(ColumnFamilyData* /*cfd*/) const {}
 void DBImpl::EraseThreadStatusCfInfo(ColumnFamilyData* /*cfd*/) const {}
 
 void DBImpl::EraseThreadStatusDbInfo() const {}
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 
 //
 // A global method that can dump out the build version

db/db_test.cc

@@ -4934,7 +4934,7 @@ TEST_F(DBTest, DynamicMemtableOptions) {
   ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
 }
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 namespace {
 bool VerifyOperationCount(Env* env, ThreadStatus::OperationType op_type,
                           int expected_count) {
@@ -5392,7 +5392,7 @@ TEST_P(DBTestWithParam, PreShutdownCompactionMiddle) {
   ASSERT_EQ(operation_count[ThreadStatus::OP_COMPACTION], 0);
 }
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 
 TEST_F(DBTest, FlushOnDestroy) {
   WriteOptions wo;
@@ -6127,9 +6127,9 @@ TEST_F(DBTest, MergeTestTime) {
 
   ASSERT_EQ(1, count);
   ASSERT_EQ(4000000, TestGetTickerCount(options, MERGE_OPERATION_TOTAL_TIME));
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
   ASSERT_GT(TestGetTickerCount(options, FLUSH_WRITE_BYTES), 0);
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 }
 
 TEST_P(DBTestWithParam, MergeCompactionTimeTest) {

db/listener_test.cc

@@ -163,9 +163,7 @@ TEST_F(EventListenerTest, OnSingleDBCompactionTest) {
   options.max_bytes_for_level_base = options.target_file_size_base * 2;
   options.max_bytes_for_level_multiplier = 2;
   options.compression = kNoCompression;
-#ifdef ROCKSDB_USING_THREAD_STATUS
-  options.enable_thread_tracking = true;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+  options.enable_thread_tracking = ThreadStatus::kEnabled;
   options.level0_file_num_compaction_trigger = kNumL0Files;
   options.table_properties_collector_factories.push_back(
       std::make_shared<TestPropertiesCollectorFactory>());
@@ -229,7 +227,7 @@ class TestFlushListener : public EventListener {
     ASSERT_EQ(info.file_checksum, kUnknownFileChecksum);
     ASSERT_EQ(info.file_checksum_func_name, kUnknownFileChecksumFuncName);
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
     // Verify the id of the current thread that created this table
     // file matches the id of any active flush or compaction thread.
     uint64_t thread_id = env_->GetThreadID();
@@ -246,7 +244,7 @@ class TestFlushListener : public EventListener {
       }
     }
     ASSERT_TRUE(found_match);
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
   }
 
   void OnFlushCompleted(DB* db, const FlushJobInfo& info) override {
@@ -310,9 +308,7 @@ TEST_F(EventListenerTest, OnSingleDBFlushTest) {
   Options options;
   options.env = CurrentOptions().env;
   options.write_buffer_size = k110KB;
-#ifdef ROCKSDB_USING_THREAD_STATUS
-  options.enable_thread_tracking = true;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+  options.enable_thread_tracking = ThreadStatus::kEnabled;
   TestFlushListener* listener = new TestFlushListener(options.env, this);
   options.listeners.emplace_back(listener);
   std::vector<std::string> cf_names = {"pikachu",  "ilya",     "muromec",
@@ -357,9 +353,7 @@ TEST_F(EventListenerTest, MultiCF) {
     Options options;
     options.env = CurrentOptions().env;
     options.write_buffer_size = k110KB;
-#ifdef ROCKSDB_USING_THREAD_STATUS
-    options.enable_thread_tracking = true;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+    options.enable_thread_tracking = ThreadStatus::kEnabled;
     options.atomic_flush = atomic_flush;
     options.create_if_missing = true;
     DestroyAndReopen(options);
@@ -407,9 +401,7 @@ TEST_F(EventListenerTest, MultiCF) {
 TEST_F(EventListenerTest, MultiDBMultiListeners) {
   Options options;
   options.env = CurrentOptions().env;
-#ifdef ROCKSDB_USING_THREAD_STATUS
-  options.enable_thread_tracking = true;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+  options.enable_thread_tracking = ThreadStatus::kEnabled;
   options.table_properties_collector_factories.push_back(
       std::make_shared<TestPropertiesCollectorFactory>());
   std::vector<TestFlushListener*> listeners;
@@ -497,9 +489,7 @@ TEST_F(EventListenerTest, MultiDBMultiListeners) {
 TEST_F(EventListenerTest, DisableBGCompaction) {
   Options options;
   options.env = CurrentOptions().env;
-#ifdef ROCKSDB_USING_THREAD_STATUS
-  options.enable_thread_tracking = true;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+  options.enable_thread_tracking = ThreadStatus::kEnabled;
   TestFlushListener* listener = new TestFlushListener(options.env, this);
   const int kCompactionTrigger = 1;
   const int kSlowdownTrigger = 5;

include/rocksdb/compaction_filter.h

@@ -284,9 +284,7 @@ class CompactionFilter : public Customizable {
       std::string* new_value,
       std::vector<std::pair<std::string, std::string>>* /* new_columns */,
       std::string* skip_until) const {
-#ifdef NDEBUG
     (void)existing_columns;
-#endif
 
     assert(!existing_value || !existing_columns);
     assert(value_type == ValueType::kWideColumnEntity || existing_value);

include/rocksdb/db.h

@@ -35,12 +35,6 @@
 #include "rocksdb/version.h"
 #include "rocksdb/wide_columns.h"
 
-#if defined(__GNUC__) || defined(__clang__)
-#define ROCKSDB_DEPRECATED_FUNC __attribute__((__deprecated__))
-#elif _WIN32
-#define ROCKSDB_DEPRECATED_FUNC __declspec(deprecated)
-#endif
-
 namespace ROCKSDB_NAMESPACE {
 
 struct ColumnFamilyOptions;

include/rocksdb/env.h

@@ -39,7 +39,7 @@
 #undef LoadLibrary
 #endif
 
-#if defined(__GNUC__) || defined(__clang__)
+#if defined(__GNUC__) || defined(__clang__)  // ODR-SAFE (essentially)
 #define ROCKSDB_PRINTF_FORMAT_ATTR(format_param, dots_param) \
   __attribute__((__format__(__printf__, format_param, dots_param)))
 #else

include/rocksdb/file_system.h

@@ -558,7 +558,7 @@ class FileSystem : public Customizable {
   }
 
 // This seems to clash with a macro on Windows, so #undef it here
-#ifdef DeleteFile
+#ifdef DeleteFile  // ODR-SAFE
 #undef DeleteFile
 #endif
   // Delete the named file.
@@ -719,7 +719,7 @@ class FileSystem : public Customizable {
       const ImmutableDBOptions& db_options) const;
 
 // This seems to clash with a macro on Windows, so #undef it here
-#ifdef GetFreeSpace
+#ifdef GetFreeSpace  // ODR-SAFE
 #undef GetFreeSpace
 #endif
 

include/rocksdb/options.h

@@ -32,7 +32,7 @@
 #include "rocksdb/version.h"
 #include "rocksdb/write_buffer_manager.h"
 
-#ifdef max
+#ifdef max  // ODR-SAFE
 #undef max
 #endif
 

include/rocksdb/sst_file_writer.h

@@ -15,12 +15,6 @@
 #include "rocksdb/types.h"
 #include "rocksdb/wide_columns.h"
 
-#if defined(__GNUC__) || defined(__clang__)
-#define ROCKSDB_DEPRECATED_FUNC __attribute__((__deprecated__))
-#elif _WIN32
-#define ROCKSDB_DEPRECATED_FUNC __declspec(deprecated)
-#endif
-
 namespace ROCKSDB_NAMESPACE {
 
 class Comparator;
@@ -117,7 +111,7 @@ class SstFileWriter {
   // REQUIRES: user_key is after any previously added point (Put/Merge/Delete)
   //           key according to the comparator.
   // REQUIRES: comparator is *not* timestamp-aware.
-  ROCKSDB_DEPRECATED_FUNC Status Add(const Slice& user_key, const Slice& value);
+  [[deprecated]] Status Add(const Slice& user_key, const Slice& value);
 
   // Add a Put key with value to currently opened file
   // REQUIRES: user_key is after any previously added point (Put/Merge/Delete)

include/rocksdb/thread_status.h

@@ -22,24 +22,16 @@
 
 #include "rocksdb/rocksdb_namespace.h"
 
-#if !defined(NROCKSDB_THREAD_STATUS)
-#define ROCKSDB_USING_THREAD_STATUS
-#endif
-
 namespace ROCKSDB_NAMESPACE {
 
-// TODO(yhchiang): remove this function once c++14 is available
-//                 as std::max will be able to cover this.
-// Current MS compiler does not support constexpr
-template <int A, int B>
-struct constexpr_max {
-  static const int result = (A > B) ? A : B;
-};
-
 // A structure that describes the current status of a thread.
 // The status of active threads can be fetched using
 // ROCKSDB_NAMESPACE::GetThreadList().
 struct ThreadStatus {
+  // Whether RocksDB was built with !NROCKSDB_THREAD_STATUS for
+  // ROCKSDB_NAMESPACE::GetThreadList() to be supported.
+  static const bool kEnabled;
+
   // The type of a thread.
   enum ThreadType : int {
     HIGH_PRIORITY = 0,  // RocksDB BG thread in high-pri thread pool
@@ -102,8 +94,8 @@ struct ThreadStatus {
 
   // The maximum number of properties of an operation.
   // This number should be set to the biggest NUM_XXX_PROPERTIES.
-  static const int kNumOperationProperties =
-      constexpr_max<NUM_COMPACTION_PROPERTIES, NUM_FLUSH_PROPERTIES>::result;
+  static constexpr int kNumOperationProperties =
+      std::max(int{NUM_COMPACTION_PROPERTIES}, int{NUM_FLUSH_PROPERTIES});
 
   // The type used to refer to a thread state.
   // A state describes lower-level action of a thread

include/rocksdb/utilities/env_mirror.h

@@ -68,7 +68,7 @@ class EnvMirror : public EnvWrapper {
     assert(as == bs);
     return as;
   }
-#if defined(_MSC_VER)
+#if defined(_MSC_VER)  // ODR-SAFE
 #pragma warning(push)
 // logical operation on address of string constant
 #pragma warning(disable : 4130)
@@ -87,7 +87,7 @@ class EnvMirror : public EnvWrapper {
     *r = ar;
     return as;
   }
-#if defined(_MSC_VER)
+#if defined(_MSC_VER)  // ODR-SAFE
 #pragma warning(pop)
 #endif
   Status DeleteFile(const std::string& f) override {

include/rocksdb/utilities/ldb_cmd_execute_result.h

@@ -9,10 +9,6 @@
 
 #include "rocksdb/rocksdb_namespace.h"
 
-#ifdef FAILED
-#undef FAILED
-#endif
-
 namespace ROCKSDB_NAMESPACE {
 
 class LDBCommandExecuteResult {

monitoring/thread_status_impl.cc

@@ -13,7 +13,9 @@
 
 namespace ROCKSDB_NAMESPACE {
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
+const bool ThreadStatus::kEnabled = true;
+
 std::string ThreadStatus::GetThreadTypeName(
     ThreadStatus::ThreadType thread_type) {
   switch (thread_type) {
@@ -117,6 +119,7 @@ std::map<std::string, uint64_t> ThreadStatus::InterpretOperationProperties(
 }
 
 #else
+const bool ThreadStatus::kEnabled = false;
 
 std::string ThreadStatus::GetThreadTypeName(
     ThreadStatus::ThreadType /*thread_type*/) {
@@ -159,5 +162,5 @@ std::map<std::string, uint64_t> ThreadStatus::InterpretOperationProperties(
   return std::map<std::string, uint64_t>();
 }
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 }  // namespace ROCKSDB_NAMESPACE

monitoring/thread_status_updater.cc

@@ -14,7 +14,7 @@
 
 namespace ROCKSDB_NAMESPACE {
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 
 thread_local ThreadStatusData* ThreadStatusUpdater::thread_status_data_ =
     nullptr;
@@ -324,5 +324,5 @@ void ThreadStatusUpdater::SetThreadOperationProperty(int /*i*/,
 void ThreadStatusUpdater::IncreaseThreadOperationProperty(int /*i*/,
                                                           uint64_t /*delta*/) {}
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 }  // namespace ROCKSDB_NAMESPACE

monitoring/thread_status_updater.h

@@ -47,7 +47,7 @@ class ColumnFamilyHandle;
 
 // The structure that keeps constant information about a column family.
 struct ConstantColumnFamilyInfo {
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
  public:
   ConstantColumnFamilyInfo(const void* _db_key, const std::string& _db_name,
                            const std::string& _cf_name)
@@ -55,13 +55,13 @@ struct ConstantColumnFamilyInfo {
   const void* db_key;
   const std::string db_name;
   const std::string cf_name;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 };
 
 // the internal data-structure that is used to reflect the current
 // status of a thread using a set of atomic pointers.
 struct ThreadStatusData {
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
   explicit ThreadStatusData() {
     enable_tracking.store(false);
     thread_id.store(0);
@@ -86,7 +86,7 @@ struct ThreadStatusData {
   std::atomic<ThreadStatus::OperationStage> operation_stage;
   std::atomic<uint64_t> op_properties[ThreadStatus::kNumOperationProperties];
   std::atomic<ThreadStatus::StateType> state_type;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 };
 
 // The class that stores and updates the status of the current thread
@@ -190,7 +190,7 @@ class ThreadStatusUpdater {
       const std::vector<ColumnFamilyHandle*>& handles, bool check_exist);
 
  protected:
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
   // The thread-local variable for storing thread status.
   static thread_local ThreadStatusData* thread_status_data_;
 
@@ -220,7 +220,7 @@ class ThreadStatusUpdater {
 
 #else
   static ThreadStatusData* thread_status_data_;
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 };
 
 }  // namespace ROCKSDB_NAMESPACE

monitoring/thread_status_updater_debug.cc

@@ -12,7 +12,7 @@
 namespace ROCKSDB_NAMESPACE {
 
 #ifndef NDEBUG
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 void ThreadStatusUpdater::TEST_VerifyColumnFamilyInfoMap(
     const std::vector<ColumnFamilyHandle*>& handles, bool check_exist) {
   std::unique_lock<std::mutex> lock(thread_list_mutex_);
@@ -37,7 +37,7 @@ void ThreadStatusUpdater::TEST_VerifyColumnFamilyInfoMap(
     const std::vector<ColumnFamilyHandle*>& /*handles*/, bool /*check_exist*/) {
 }
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 #endif  // !NDEBUG
 
 }  // namespace ROCKSDB_NAMESPACE

monitoring/thread_status_util.cc

@@ -11,7 +11,7 @@
 
 namespace ROCKSDB_NAMESPACE {
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 thread_local ThreadStatusUpdater*
     ThreadStatusUtil::thread_updater_local_cache_ = nullptr;
 thread_local bool ThreadStatusUtil::thread_updater_initialized_ = false;
@@ -171,9 +171,10 @@ AutoThreadOperationStageUpdater::~AutoThreadOperationStageUpdater() {
 ThreadStatusUpdater* ThreadStatusUtil::thread_updater_local_cache_ = nullptr;
 bool ThreadStatusUtil::thread_updater_initialized_ = false;
 
-bool ThreadStatusUtil::MaybeInitThreadLocalUpdater(const Env* /*env*/) {
-  return false;
-}
+void ThreadStatusUtil::RegisterThread(
+    const Env* /*env*/, ThreadStatus::ThreadType /*thread_type*/) {}
+
+void ThreadStatusUtil::UnregisterThread() {}
 
 void ThreadStatusUtil::SetEnableTracking(bool /*enable_tracking*/) {}
 
@@ -204,11 +205,15 @@ void ThreadStatusUtil::EraseDatabaseInfo(const DB* /*db*/) {}
 
 void ThreadStatusUtil::ResetThreadStatus() {}
 
+bool ThreadStatusUtil::MaybeInitThreadLocalUpdater(const Env* /*env*/) {
+  return false;
+}
+
 AutoThreadOperationStageUpdater::AutoThreadOperationStageUpdater(
     ThreadStatus::OperationStage /*stage*/) {}
 
 AutoThreadOperationStageUpdater::~AutoThreadOperationStageUpdater() {}
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 
 }  // namespace ROCKSDB_NAMESPACE

monitoring/thread_status_util.h

@@ -90,7 +90,7 @@ class ThreadStatusUtil {
   // a non-null pointer.
   static bool MaybeInitThreadLocalUpdater(const Env* env);
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
   // A boolean flag indicating whether thread_updater_local_cache_
   // is initialized.  It is set to true when an Env uses any
   // ThreadStatusUtil functions using the current thread other
@@ -130,7 +130,7 @@ class AutoThreadOperationStageUpdater {
   explicit AutoThreadOperationStageUpdater(ThreadStatus::OperationStage stage);
   ~AutoThreadOperationStageUpdater();
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
  private:
   ThreadStatus::OperationStage prev_stage_;
 #endif

unreleased_history/public_api_changes/odr_thread_status.md

@@ -0,0 +1 @@
+* To reduce risk of ODR violations or similar, `ROCKSDB_USING_THREAD_STATUS` has been removed from public headers and replaced with static `const bool ThreadStatus::kEnabled`. Some other uses of conditional compilation have been removed from public API headers to reduce risk of ODR violations or other issues.

util/thread_list_test.cc

@@ -10,7 +10,7 @@
 #include "rocksdb/db.h"
 #include "test_util/testharness.h"
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 
 namespace ROCKSDB_NAMESPACE {
 
@@ -359,4 +359,4 @@ int main(int argc, char** argv) {
   return 0;
 }
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS

util/thread_operation.h

@@ -19,7 +19,7 @@
 
 namespace ROCKSDB_NAMESPACE {
 
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
 
 // The structure that describes a major thread operation.
 struct OperationInfo {
@@ -120,5 +120,5 @@ struct OperationInfo {};
 
 struct StateInfo {};
 
-#endif  // ROCKSDB_USING_THREAD_STATUS
+#endif  // !NROCKSDB_THREAD_STATUS
 }  // namespace ROCKSDB_NAMESPACE

util/threadpool_imp.cc

@@ -324,7 +324,7 @@ void ThreadPoolImpl::Impl::BGThreadWrapper(void* arg) {
   BGThreadMetadata* meta = static_cast<BGThreadMetadata*>(arg);
   size_t thread_id = meta->thread_id_;
   ThreadPoolImpl::Impl* tp = meta->thread_pool_;
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
   // initialize it because compiler isn't good enough to see we don't use it
   // uninitialized
   ThreadStatus::ThreadType thread_type = ThreadStatus::NUM_THREAD_TYPES;
@@ -350,7 +350,7 @@ void ThreadPoolImpl::Impl::BGThreadWrapper(void* arg) {
 #endif
   delete meta;
   tp->BGThread(thread_id);
-#ifdef ROCKSDB_USING_THREAD_STATUS
+#ifndef NROCKSDB_THREAD_STATUS
   ThreadStatusUtil::UnregisterThread();
 #endif
   return;