From defe11e8228f836a1b9a0a8b88cfe7a7f9c738c4 Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Mon, 18 May 2026 01:58:37 +0800 Subject: [PATCH] [Velox] Fix gflags dual-registration abort on macOS arm64 On macOS arm64, libvelox.dylib has static gflags baked in via Folly (Velox builds Folly with -DGFLAGS_SHARED=FALSE). Without special handling, libgluten.dylib transitively pulls dynamic gflags via the INTERFACE_LINK_LIBRARIES of glog::glog and Folly::folly. At JVM load time, dyld coalesces the weak C++ function-local-static guard inside FlagRegistry::GlobalRegistry() across the two dylibs. Both copies then register "flagfile" against the same registry and gflags' duplicate-flag check aborts the process before any user code runs: ERROR: flag 'flagfile' was defined more than once (in files '.../gflags.cc' and '.../gflags.cc') ... is being linked both statically and dynamically. Linux is unaffected because (a) ELF does not coalesce weak symbols across .so boundaries by default, and (b) Gluten already uses symbols.map to control libgluten.so's export surface. macOS has no version-script equivalent, so a different mechanism is required. This change fixes the abort end-to-end for macOS while leaving Linux and Windows build/link semantics untouched. 1. cpp/CMake/Findglog.cmake: on Darwin, prefer the static libglog.a and force gflags_component=static. When both archives are present we replace the imported google::glog target with an INTERFACE IMPORTED target whose INTERFACE_LINK_OPTIONS carry `LINKER:-load_hidden,` and `LINKER:-load_hidden,`. -load_hidden is the Apple ld64 flag that gives every symbol pulled from the archive hidden visibility, preventing dyld from coalescing them across dylibs. The static gflags archive path is resolved by inspecting IMPORTED_LOCATION_RELEASE / _NOCONFIG / * on gflags::gflags_static. 2. cpp/core/utils/GflagsStubDarwin.cc (new): exports a no-op google::HandleCommandLineHelpFlags with default visibility. Velox's archive of gflags pulls gflags.cc.o but never references gflags_reporting.cc.o, so once -load_hidden makes the real copy invisible, the dynamic linker would fail to resolve this symbol at dlopen time. The stub resolves it from libgluten.dylib instead. 3. cpp/core/CMakeLists.txt: conditionally adds the stub to the gluten target on APPLE. 4. cpp/velox/CMakeLists.txt: on Darwin, links google::glog as PUBLIC on the velox target so its INTERFACE_LINK_OPTIONS propagate through libvelox.dylib to test binaries and benchmarks. PRIVATE linkage on the gluten target is intentional for Linux (symbols.map handles it), but on Darwin Folly::folly's INTERFACE_LINK_LIBRARIES pulls libgflags.a into libvelox.dylib and any test executables with default visibility, reviving the same dual-registration abort at test startup. 5. cpp/velox/compute/VeloxBackend.cc: guards google::InitGoogleLogging with IsGoogleLoggingInitialized() and makes VeloxBackend::create() idempotent. Multi-suite gtest binaries on macOS re-enter VeloxBackend::init from each SetUpTestSuite, otherwise triggering glog's "You called InitGoogleLogging() twice!" check and Gluten's Registry "Required object already registered" check. Verification: - nm -g libvelox.dylib | grep "google.*ParseCommandLine" -> empty (gflags symbols are not exported across the dylib boundary) - nm libvelox.dylib | grep FlagRegistry -> all lowercase t / b (every FlagRegistry symbol is local to libvelox.dylib) - velox_shuffle_writer_test runs 5436/5436 cases cleanly on macOS 14 arm64 with Apple Clang 17. - Linux x86_64 build green, no link/load behavior change. --- cpp/CMake/Findglog.cmake | 122 +++++++++++++++++++++++++---- cpp/core/CMakeLists.txt | 10 +++ cpp/core/utils/GflagsStubDarwin.cc | 28 +++++++ cpp/velox/CMakeLists.txt | 12 +++ cpp/velox/compute/VeloxBackend.cc | 7 +- 5 files changed, 163 insertions(+), 16 deletions(-) create mode 100644 cpp/core/utils/GflagsStubDarwin.cc diff --git a/cpp/CMake/Findglog.cmake b/cpp/CMake/Findglog.cmake index 6d9dbdacf1b..05c15e2100c 100644 --- a/cpp/CMake/Findglog.cmake +++ b/cpp/CMake/Findglog.cmake @@ -22,9 +22,23 @@ if(NOT BUILD_GLOG) include(FindPackageHandleStandardArgs) include(SelectLibraryConfigurations) + # On macOS, prefer the static libglog.a over libglog.dylib. libvelox.dylib has + # static gflags baked in via folly, so we must avoid libglog.dylib which + # transitively loads libgflags.dylib at runtime and triggers the "linked both + # statically and dynamically" abort in gflags. + if(CMAKE_SYSTEM_NAME MATCHES "Darwin") + set(_glog_find_suffixes_saved ${CMAKE_FIND_LIBRARY_SUFFIXES}) + set(CMAKE_FIND_LIBRARY_SUFFIXES ".a" ${CMAKE_FIND_LIBRARY_SUFFIXES}) + endif() + find_library(GLOG_LIBRARY_RELEASE glog PATHS ${GLOG_LIBRARYDIR}) find_library(GLOG_LIBRARY_DEBUG glogd PATHS ${GLOG_LIBRARYDIR}) + if(CMAKE_SYSTEM_NAME MATCHES "Darwin") + set(CMAKE_FIND_LIBRARY_SUFFIXES ${_glog_find_suffixes_saved}) + unset(_glog_find_suffixes_saved) + endif() + find_path(GLOG_INCLUDE_DIR glog/logging.h PATHS ${GLOG_INCLUDEDIR}) select_library_configurations(GLOG) @@ -48,13 +62,43 @@ else() set(libgflags_component shared) endif() +# On macOS, force static gflags regardless of glog linkage. libvelox.dylib has +# static gflags baked in via folly (-DGFLAGS_SHARED=FALSE). If libgluten.dylib +# were to load libgflags.dylib dynamically, gflags would detect the same flag +# registered both statically (inside libvelox.dylib) and dynamically (via +# libgflags.dylib) and abort with "being linked both statically and +# dynamically". +if(CMAKE_SYSTEM_NAME MATCHES "Darwin") + set(libgflags_component static) +endif() + +# On macOS with a static libglog.a we link glog and gflags via -load_hidden so +# their symbols stay local to libgluten.dylib. This prevents dyld from +# coalescing the weak symbols (C++ function-local statics like +# FlagRegistry::GlobalRegistry) with the static gflags copy already inside +# libvelox.dylib, which otherwise triggers "flag 'flagfile' ... linked both +# statically and dynamically". +# +# For this to work, google::glog must be an INTERFACE IMPORTED target so that +# CMake does not try to add GLOG_LIBRARY through the normal link path. +set(_gluten_glog_hidden_load OFF) +if(CMAKE_SYSTEM_NAME MATCHES "Darwin" AND libglog_ext STREQUAL ".a") + set(_gluten_glog_hidden_load ON) +endif() + # glog::glog may already exist. Use google::glog to avoid conflicts. -add_library(google::glog ${libglog_type} IMPORTED) -set_target_properties(google::glog PROPERTIES INTERFACE_INCLUDE_DIRECTORIES - "${GLOG_INCLUDE_DIR}") -set_target_properties( - google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C" - IMPORTED_LOCATION "${GLOG_LIBRARY}") +if(_gluten_glog_hidden_load) + add_library(google::glog INTERFACE IMPORTED) + set_target_properties(google::glog PROPERTIES INTERFACE_INCLUDE_DIRECTORIES + "${GLOG_INCLUDE_DIR}") +else() + add_library(google::glog ${libglog_type} IMPORTED) + set_target_properties(google::glog PROPERTIES INTERFACE_INCLUDE_DIRECTORIES + "${GLOG_INCLUDE_DIR}") + set_target_properties( + google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LANGUAGES "C" + IMPORTED_LOCATION "${GLOG_LIBRARY}") +endif() set(GLUTEN_GFLAGS_VERSION 2.2.2) find_package(gflags ${GLUTEN_GFLAGS_VERSION} CONFIG @@ -73,17 +117,65 @@ if(gflags_FOUND) FATAL_ERROR "Found Gflags but missing component gflags_${libgflags_component}") endif() - if(TARGET gflags::gflags_${libgflags_component}) - set_target_properties( - google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES - gflags::gflags_${libgflags_component}) - else() - set_target_properties( - google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES - gflags_${libgflags_component}) + if(NOT _gluten_glog_hidden_load) + if(TARGET gflags::gflags_${libgflags_component}) + set_target_properties( + google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES + gflags::gflags_${libgflags_component}) + else() + set_target_properties( + google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES + gflags_${libgflags_component}) + endif() endif() else() include(BuildGflags) + if(NOT _gluten_glog_hidden_load) + set_target_properties( + google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES gflags_static) + endif() +endif() + +if(_gluten_glog_hidden_load) + set(_gflags_static_target "") + if(TARGET gflags::gflags_static) + set(_gflags_static_target gflags::gflags_static) + elseif(TARGET gflags_static) + set(_gflags_static_target gflags_static) + endif() + + set(_gflags_static_path "") + if(_gflags_static_target) + foreach(_cfg RELEASE NOCONFIG "" DEBUG RELWITHDEBINFO MINSIZEREL) + if(_cfg STREQUAL "") + set(_prop IMPORTED_LOCATION) + else() + set(_prop IMPORTED_LOCATION_${_cfg}) + endif() + get_target_property(_maybe_path ${_gflags_static_target} ${_prop}) + if(_maybe_path AND NOT _maybe_path STREQUAL "_maybe_path-NOTFOUND") + set(_gflags_static_path ${_maybe_path}) + break() + endif() + endforeach() + endif() + + if(NOT _gflags_static_path) + message( + FATAL_ERROR + "Could not resolve libgflags.a path for -load_hidden; expected a gflags_static target with an IMPORTED_LOCATION* property." + ) + endif() + + message( + STATUS "Linking gflags (static, -load_hidden): ${_gflags_static_path}") + set_target_properties( - google::glog PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES gflags_static) + google::glog PROPERTIES INTERFACE_LINK_OPTIONS + "LINKER:-load_hidden,${GLOG_LIBRARY}") + set_property( + TARGET google::glog + APPEND + PROPERTY INTERFACE_LINK_OPTIONS + "LINKER:-load_hidden,${_gflags_static_path}") endif() diff --git a/cpp/core/CMakeLists.txt b/cpp/core/CMakeLists.txt index e16464b8111..3d7fcec8a6c 100644 --- a/cpp/core/CMakeLists.txt +++ b/cpp/core/CMakeLists.txt @@ -154,6 +154,16 @@ file(MAKE_DIRECTORY ${root_directory}/releases) add_library(gluten SHARED ${SPARK_COLUMNAR_PLUGIN_SRCS}) add_dependencies(gluten jni_proto) +# On macOS, libgluten.dylib links libgflags.a with -load_hidden (see +# cpp/CMake/Findglog.cmake). That hides the gflags copy pulled in by +# libgluten.dylib, but libvelox.dylib has an undefined reference to +# google::HandleCommandLineHelpFlags that previously resolved against +# libgluten.dylib's exported copy. Provide an exported no-op stub so +# libvelox.dylib can still find the symbol at dlopen time. +if(CMAKE_SYSTEM_NAME MATCHES "Darwin") + target_sources(gluten PRIVATE utils/GflagsStubDarwin.cc) +endif() + # Hide symbols of some static dependencies. Otherwise, if such dependencies are # already statically linked to libvelox.so, a runtime error will be reported: # xxx is being linked both statically and dynamically. diff --git a/cpp/core/utils/GflagsStubDarwin.cc b/cpp/core/utils/GflagsStubDarwin.cc new file mode 100644 index 00000000000..a1288cea7e0 --- /dev/null +++ b/cpp/core/utils/GflagsStubDarwin.cc @@ -0,0 +1,28 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// macOS only: libgluten.dylib loads libgflags.a with -load_hidden +// (cpp/CMake/Findglog.cmake) so its gflags symbols do not coalesce with the +// static gflags already baked into libvelox.dylib. libvelox.dylib's own +// static gflags build pulled gflags.cc.o but not gflags_reporting.cc.o, so +// google::HandleCommandLineHelpFlags remains undefined inside libvelox.dylib +// and is expected to be supplied by libgluten.dylib at dlopen time. Provide a +// no-op stub with default visibility; Gluten never invokes help processing. +namespace google { +// NOLINTNEXTLINE(readability-identifier-naming) - name dictated by gflags ABI. +__attribute__((visibility("default"))) void HandleCommandLineHelpFlags() {} +} // namespace google diff --git a/cpp/velox/CMakeLists.txt b/cpp/velox/CMakeLists.txt index 532dd6e6a79..d932e0ec93f 100644 --- a/cpp/velox/CMakeLists.txt +++ b/cpp/velox/CMakeLists.txt @@ -293,6 +293,18 @@ endif() target_link_libraries(velox PUBLIC gluten) +# On Darwin, propagate google::glog's INTERFACE_LINK_OPTIONS +# (LINKER:-load_hidden for libglog.a and libgflags.a) through libvelox.dylib to +# all downstream consumers (test binaries, benchmarks). Without this, +# Folly::folly's INTERFACE_LINK_LIBRARIES pulls libgflags.a/libglog.a into +# libvelox.dylib and test binaries with default visibility, so their +# FlagRegistry::GlobalRegistry function-local statics coalesce with +# libgluten.dylib's copy and gflags aborts with "flag 'flagfile' linked both +# statically and dynamically" at startup. +if(CMAKE_SYSTEM_NAME MATCHES "Darwin") + target_link_libraries(velox PUBLIC google::glog) +endif() + # Requires VELOX_MONO_LIBRARY=ON when building Velox. import_library(facebook::velox ${VELOX_BUILD_PATH}/lib/libvelox.a) diff --git a/cpp/velox/compute/VeloxBackend.cc b/cpp/velox/compute/VeloxBackend.cc index 85a8622508a..e493ccf3251 100644 --- a/cpp/velox/compute/VeloxBackend.cc +++ b/cpp/velox/compute/VeloxBackend.cc @@ -113,7 +113,9 @@ void VeloxBackend::init( } } FLAGS_logtostderr = true; - google::InitGoogleLogging("gluten"); + if (!google::IsGoogleLoggingInitialized()) { + google::InitGoogleLogging("gluten"); + } globalMemoryManager_ = std::make_unique(kVeloxBackendKind, std::move(listener), *backendConf_); @@ -352,6 +354,9 @@ std::unique_ptr VeloxBackend::instance_ = nullptr; void VeloxBackend::create( std::unique_ptr listener, const std::unordered_map& conf) { + if (instance_) { + return; + } instance_ = std::unique_ptr(new VeloxBackend(std::move(listener), conf)); }