From cb1bf09bfc912472b380d09ee2b733a6684457d7 Mon Sep 17 00:00:00 2001
From: Yanqin Jin <yanqin@fb.com>
Date: Wed, 5 Jun 2019 15:16:43 -0700
Subject: [PATCH] Fix tsan error (#5414)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Summary:
Previous code has a warning when compile with tsan, leading to an error since we have -Werror.
Compilation result
```
In file included from ./env/env_chroot.h:12,
                 from env/env_test.cc:40:
./include/rocksdb/env.h: In instantiation of ‘rocksdb::Status rocksdb::DynamicLibrary::LoadFunction(const string&, std::function<T>*) [with T = void*(void*, const char*); std::__cxx11::string = std::__cxx11::basic_string<char>]’:
env/env_test.cc:260:5:   required from here
./include/rocksdb/env.h:1010:17: error: cast between incompatible function types from ‘rocksdb::DynamicLibrary::FunctionPtr’ {aka ‘void* (*)()’} to ‘void* (*)(void*, const char*)’ [-Werror=cast-function-type]
     *function = reinterpret_cast<T*>(ptr);
                 ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make: *** [env/env_test.o] Error 1
```
It also has another error reported by clang
```
env/env_posix.cc:141:11: warning: Value stored to 'err' during its initialization is never read
    char* err = dlerror();  // Clear any old error
          ^~~   ~~~~~~~~~
1 warning generated.
```

Test plan (on my devserver).
```
$make clean
$OPT=-g ROCKSDB_FBCODE_BUILD_WITH_PLATFORM007=1 COMPILE_WITH_TSAN=1 make -j32
$
$make clean
$USE_CLANG=1 TEST_TMPDIR=/dev/shm/rocksdb OPT=-g make -j1 analyze
```
Both should pass.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5414

Differential Revision: D15637315

Pulled By: riversand963

fbshipit-source-id: 8e307483761019a4d5998cab92d49516d7edffbf
---
 env/env_posix.cc      | 27 +++++++++++++--------------
 include/rocksdb/env.h | 16 +++++++---------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/env/env_posix.cc b/env/env_posix.cc
index f1a0907c9..c0edb0096 100644
--- a/env/env_posix.cc
+++ b/env/env_posix.cc
@@ -137,13 +137,14 @@ class PosixDynamicLibrary : public DynamicLibrary {
       : name_(name), handle_(handle) {}
   ~PosixDynamicLibrary() override { dlclose(handle_); }
 
-  Status LoadSymbol(const std::string& sym_name, FunctionPtr* func) override {
-    char* err = dlerror();  // Clear any old error
-    *func = (FunctionPtr)dlsym(handle_, sym_name.c_str());
+  Status LoadSymbol(const std::string& sym_name, void** func) override {
+    assert(nullptr != func);
+    dlerror();  // Clear any old error
+    *func = dlsym(handle_, sym_name.c_str());
     if (*func != nullptr) {
       return Status::OK();
     } else {
-      err = dlerror();
+      char* err = dlerror();
       return Status::NotFound("Error finding symbol: " + sym_name, err);
     }
   }
@@ -771,16 +772,14 @@ class PosixEnv : public Env {
   }
 
 #ifndef ROCKSDB_NO_DYNAMIC_EXTENSION
-  /**
-   * Loads the named library into the result.
-   * If the input name is empty, the current executable is loaded
-   * On *nix systems, a "lib" prefix is added to the name if one is not supplied
-   * Comparably, the appropriate shared library extension is added to the name
-   * if not supplied. If search_path is not specified, the shared library will
-   * be loaded using the default path (LD_LIBRARY_PATH) If search_path is
-   * specified, the shared library will be searched for in the directories
-   * provided by the search path
-   */
+  // Loads the named library into the result.
+  // If the input name is empty, the current executable is loaded
+  // On *nix systems, a "lib" prefix is added to the name if one is not supplied
+  // Comparably, the appropriate shared library extension is added to the name
+  // if not supplied. If search_path is not specified, the shared library will
+  // be loaded using the default path (LD_LIBRARY_PATH) If search_path is
+  // specified, the shared library will be searched for in the directories
+  // provided by the search path
   Status LoadLibrary(const std::string& name, const std::string& path,
                      std::shared_ptr<DynamicLibrary>* result) override {
     Status status;
diff --git a/include/rocksdb/env.h b/include/rocksdb/env.h
index 0a055cea0..ba8978dc8 100644
--- a/include/rocksdb/env.h
+++ b/include/rocksdb/env.h
@@ -1029,25 +1029,23 @@ class FileLock {
 
 class DynamicLibrary {
  public:
-  typedef void* (*FunctionPtr)();
   virtual ~DynamicLibrary() {}
 
-  /** Returns the name of the dynamic library */
+  // Returns the name of the dynamic library.
   virtual const char* Name() const = 0;
 
-  /**
-   * Loads the symbol for sym_name from the library and updates the input
-   * function. Returns the loaded symbol
-   */
+  // Loads the symbol for sym_name from the library and updates the input
+  // function. Returns the loaded symbol.
   template <typename T>
   Status LoadFunction(const std::string& sym_name, std::function<T>* function) {
-    FunctionPtr ptr;
+    assert(nullptr != function);
+    void* ptr = nullptr;
     Status s = LoadSymbol(sym_name, &ptr);
     *function = reinterpret_cast<T*>(ptr);
     return s;
   }
-  /** Loads and returns the symbol for sym_name from the library  */
-  virtual Status LoadSymbol(const std::string& sym_name, FunctionPtr* func) = 0;
+  // Loads and returns the symbol for sym_name from the library.
+  virtual Status LoadSymbol(const std::string& sym_name, void** func) = 0;
 };
 
 extern void LogFlush(const std::shared_ptr<Logger>& info_log);