[impala] branch master updated: IMPALA-11640/IMPALA-11641: Workaround errors in shared library build on Ubuntu 18+

[db...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dbecker pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git


The following commit(s) were added to refs/heads/master by this push:
     new 97480c3e3 IMPALA-11640/IMPALA-11641: Workaround errors in shared library build on Ubuntu 18+
97480c3e3 is described below

commit 97480c3e3259777c27e37b830fbf14e27b4f4d89
Author: Joe McDonnell <jo...@cloudera.com>
AuthorDate: Thu Oct 6 08:35:14 2022 -0700

    IMPALA-11640/IMPALA-11641: Workaround errors in shared library build on Ubuntu 18+
    
    Building with -so on Ubuntu 18 or higher fails due to
    an issue finding dlopen in unwind_safeness.cc:
    
    unwind_safeness.cc:76] Check failed: !error failed to find symbol dlopen
    
    unwind_safeness.cc is using dlsym to load the dlopen symbol
    so that it can wrap it with its own dlopen code. The Impala
    build has issues with the ordering of libraries, and this
    code does not find dlopen. This has previously happened with
    the dl_iterate_phdr symbol in Kudu.
    
    This is a problem starting with Ubuntu 18.04, because Ubuntu 16.04
    uses a version of glibc that has a bug in reporting this
    error. Ubuntu 18.04 uses a newer glibc with a fix for the bug. See
    https://sourceware.org/bugzilla/show_bug.cgi?id=19509 .
    As a workaround for this issue, this tolerates not finding
    dlopen/dlclose when building with shared libraries. Impala shared
    libraries are not used in production, so this bypasses the issue.
    This also adds extra validation to make sure the symbols are non-null.
    Specifically, this adds another CHECK in dlsym_or_die to verify
    that the symbol is non-null. This also adds a DCHECK to verify that
    the symbol is non-null at dereference.
    
    This also fixes an issue where Boost was always using static
    libraries, even for shared library builds. This makes Boost use
    shared libraries for shared library builds.
    
    Testing:
     - The shared library build passes on Ubuntu 18 and Ubuntu 20
     - Impala can boot and run queries with shared libraries
    
    Change-Id: Iaab196b3d669ccc12854a98d0dbfbae2b9b91244
    Reviewed-on: http://gerrit.cloudera.org:8080/19104
    Reviewed-by: Daniel Becker <da...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 CMakeLists.txt                            | 10 ++++++++--
 be/CMakeLists.txt                         |  6 ++++++
 be/src/kudu/util/debug/unwind_safeness.cc | 28 ++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d6c58972e..0908ebcea 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -148,7 +148,13 @@ set_dep_root(CALLONCEHACK)
 # don't accidentally use it if it can be found.
 set(Boost_NO_BOOST_CMAKE ON)
 
-set(Boost_USE_STATIC_LIBS NOT ${BUILD_SHARED_LIBS})
+# Make Boost follow the preference of shared libraries vs static libraries.
+if(BUILD_SHARED_LIBS)
+  set(Boost_USE_STATIC_LIBS OFF)
+else()
+  set(Boost_USE_STATIC_LIBS ON)
+endif()
+# Always use the static Boost runtime
 set(Boost_USE_STATIC_RUNTIME ON)
 
 # Newer versions of boost (including the version in toolchain) don't build separate
@@ -195,7 +201,7 @@ find_package(Boost REQUIRED COMPONENTS thread regex filesystem system date_time
 # Mark Boost as a system header to avoid compile warnings.
 include_directories(SYSTEM ${Boost_INCLUDE_DIRS})
 message(STATUS "Boost include dir: " ${Boost_INCLUDE_DIRS})
-message(STATUS "Boost libraries: " ${Boost_LIBRARIES})
+message(STATUS "Boost libraries: ${Boost_LIBRARIES}")
 
 # If at all possible, try to link with system OpenSSL in an attempt to match what the
 # platform that this build might ultimately be deployed on. If no suitable OpenSSL is
diff --git a/be/CMakeLists.txt b/be/CMakeLists.txt
index ffd050b2f..5d1fa4d9c 100644
--- a/be/CMakeLists.txt
+++ b/be/CMakeLists.txt
@@ -62,6 +62,12 @@ IF($ENV{USE_GOLD_LINKER} STREQUAL "true")
   SET(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -fuse-ld=gold")
 ENDIF()
 
+if(BUILD_SHARED_LIBS)
+  # There is some logic in be/src/kudu/util/debug/unwind_safeness.cc that needs to adapt
+  # when using shared libraries. See IMPALA-11640.
+  SET(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -DIMPALA_SHARED_LIBRARY")
+endif()
+
 # On Apple we build with clang and need libstdc++ instead of libc++
 if (APPLE)
   SET(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -stdlib=libstdc++")
diff --git a/be/src/kudu/util/debug/unwind_safeness.cc b/be/src/kudu/util/debug/unwind_safeness.cc
index c8e0adf60..6ddcb8327 100644
--- a/be/src/kudu/util/debug/unwind_safeness.cc
+++ b/be/src/kudu/util/debug/unwind_safeness.cc
@@ -32,8 +32,16 @@
 
 #include <glog/logging.h>
 
+// DCHECK's meaning on release builds is not compatible with the comma
+// operator, so don't include the DCHECK on release builds.
+#if defined(NDEBUG)
 #define CALL_ORIG(func_name, ...) \
   ((decltype(&func_name))g_orig_ ## func_name)(__VA_ARGS__)
+#else
+#define CALL_ORIG(func_name, ...) \
+  DCHECK(g_orig_ ## func_name != nullptr), \
+  ((decltype(&func_name))g_orig_ ## func_name)(__VA_ARGS__)
+#endif
 
 // Don't hook dl_iterate_phdr in TSAN builds since TSAN already instruments this
 // function and blocks signals while calling it. And skip it for macOS; it
@@ -69,13 +77,19 @@ struct ScopedBumpDepth {
   }
 };
 
+#ifndef IMPALA_SHARED_LIBRARY
 void *dlsym_or_die(const char *sym) {
   dlerror();
   void* ret = dlsym(RTLD_NEXT, sym);
   char* error = dlerror();
   CHECK(!error) << "failed to find symbol " << sym << ": " << error;
+  // On older glibc, the error may not be reported even when the symbol
+  // was not found. As a safeguard, this also checks that the return value
+  // is non-null.
+  CHECK(ret != nullptr) << "failed to find symbol " << sym << ", but no error was set.";
   return ret;
 }
+#endif
 
 // Initialize the global variables which store the original references. This is
 // set up as a constructor so that we're guaranteed to call this before main()
@@ -97,8 +111,22 @@ void InitIfNecessary() {
   // need for any synchronization here.
   if (g_initted) return;
 
+#ifdef IMPALA_SHARED_LIBRARY
+  // When Impala is built with shared libraries, it suffers a more extensive
+  // version of the issue with "dl_iterate_phdr" below. Both dlopen and
+  // dlclose cannot be found due to the linking order in the Impala build.
+  // Since Impala does not ship with shared libraries, tolerate these missing
+  // symbols. As noted below, any actual usage will result in dereferencing
+  // a null pointer, so this will be very noticeable.
+  g_orig_dlopen = dlsym(RTLD_NEXT, "dlopen");
+  g_orig_dlclose = dlsym(RTLD_NEXT, "dlclose");
+#else
+  // Leave the original logic in place for static binaries, as there has never
+  // been an issue with these checks.
   g_orig_dlopen = dlsym_or_die("dlopen");
   g_orig_dlclose = dlsym_or_die("dlclose");
+#endif
+
 #ifdef HOOK_DL_ITERATE_PHDR
   // Failing to hook dl_iterate_phdr is non-fatal.
   //