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 <[email protected]> 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 <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Reviewed-on: http://gerrit.cloudera.org:8080/23009 Tested-by: Daniel Becker <[email protected]> --- 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. //
