(impala) 03/06: IMPALA-14066 (Part 3): Re-applying 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

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

    IMPALA-14066 (Part 3): Re-applying IMPALA-11640/IMPALA-11641: Workaround errors in shared library build on Ubuntu 18+
    
    This commit re-applies IMPALA-11640/IMPALA-11641 to the Kudu files after
    the Kudu rebase to v1.17.1. The original commit message is below:
    
    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: I0034464a075b3add7ce591a36dab6fda334e6203
    Reviewed-on: http://gerrit.cloudera.org:8080/19104
    Reviewed-by: Daniel Becker <da...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
    Reviewed-on: http://gerrit.cloudera.org:8080/23009
    Tested-by: Daniel Becker <da...@cloudera.com>
---
 be/src/kudu/util/debug/unwind_safeness.cc | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

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.
   //