github-actions[bot] commented on code in PR #67701:
URL: https://github.com/apache/doris/pull/67701#discussion_r3964889911
##########
be/src/glibc-compatibility/lance_symbol_versions.c:
##########
@@ -41,6 +45,65 @@
// glibc symbol, so it cannot recurse back into the hidden wrapper.
#define DORIS_HIDDEN __attribute__((visibility("hidden")))
+typedef void (*doris_tls_destructor)(void*);
+
+struct doris_tls_destructor_entry {
+ doris_tls_destructor destructor;
+ void* object;
+ struct doris_tls_destructor_entry* next;
+};
+
+static pthread_key_t doris_tls_destructor_key;
+static pthread_once_t doris_tls_destructor_once = PTHREAD_ONCE_INIT;
+
+static void doris_run_tls_destructors(void* value) {
+ struct doris_tls_destructor_entry* entry = value;
+ while (entry != NULL) {
+ struct doris_tls_destructor_entry* next = entry->next;
+ doris_tls_destructor destructor = entry->destructor;
+ void* object = entry->object;
+
+ // Publish the remainder before invoking the destructor. A destructor
+ // may register another TLS destructor, which must run before the older
+ // entries that are still pending.
+ if (pthread_setspecific(doris_tls_destructor_key, next) != 0) {
+ abort();
+ }
+ free(entry);
+ destructor(object);
+ entry = pthread_getspecific(doris_tls_destructor_key);
+ }
+}
+
+static void doris_run_main_thread_tls_destructors(void) {
+ doris_run_tls_destructors(pthread_getspecific(doris_tls_destructor_key));
+}
+
+static void doris_init_tls_destructor_key(void) {
+ if (pthread_key_create(&doris_tls_destructor_key,
doris_run_tls_destructors) != 0) {
+ abort();
+ }
+ // pthread_key_create() arranges for doris_run_tls_destructors() to be
+ // called automatically when an ordinary worker thread exits. However,
+ // returning from main() (which is equivalent to exit()) does not run the
+ // initial thread's pthread key destructors. Register an atexit handler so
+ // TLS destructors belonging to the thread that performs normal process
+ // termination are still invoked.
+ //
+ // This relies on Doris normally terminating the process from its initial
+ // thread. If another thread calls exit(), atexit handlers execute in that
+ // thread and pthread_getspecific() observes that thread's TLS state.
+ // atexit handlers are not invoked by abort(), _exit(), fatal signals, or
+ // SIGKILL; those paths are already abnormal process termination.
+ //
+ // A registration failure means that normal main-thread TLS cleanup cannot
+ // be guaranteed, so fail immediately instead of continuing with a
partially
+ // installed compatibility implementation.
+ if (atexit(doris_run_main_thread_tls_destructors) != 0) {
Review Comment:
On the supported `enable_graceful_exit_check` return-from-main path, an
ordinary `atexit` callback cannot preserve initial-thread TLS ordering. If a
TLS value registers this handler and a function-local static is initialized
later, LIFO teardown destroys that static first and invokes this TLS cleanup
afterward. Glibc deliberately calls TLS destructors before its atexit/static
list. Please preserve that ordering and add a main-thread termination-order
test.
##########
be/src/glibc-compatibility/musl/expf.c:
##########
@@ -0,0 +1,81 @@
+/* origin: FreeBSD /usr/src/lib/msun/src/e_expf.c */
+/*
+ * Conversion to float by Ian Lance Taylor, Cygnus Support, [email protected].
+ */
+/*
+ * ====================================================
+ * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
+ *
+ * Developed at SunPro, a Sun Microsystems, Inc. business.
+ * Permission to use, copy, modify, and distribute this
+ * software is freely granted, provided that this notice
+ * is preserved.
+ * ====================================================
+ */
+
+#include "libm.h"
+
+static const float
+ half[2] = {0.5,-0.5},
+ ln2hi = 6.9314575195e-1f, /* 0x3f317200 */
+ ln2lo = 1.4286067653e-6f, /* 0x35bfbe8e */
+ invln2 = 1.4426950216e+0f, /* 0x3fb8aa3b */
+ /*
+ * Domain [-0.34568, 0.34568], range ~[-4.278e-9, 4.447e-9]:
+ * |x*(exp(x)+1)/(exp(x)-1) - p(x)| < 2**-27.74
+ */
+ P1 = 1.6666625440e-1f, /* 0xaaaa8f.0p-26 */
+ P2 = -2.7667332906e-3f; /* -0xb55215.0p-32 */
+
+float expf(float x)
+{
+ float_t hi, lo, c, xx, y;
+ int k, sign;
+ uint32_t hx;
+
+ GET_FLOAT_WORD(hx, x);
+ sign = hx >> 31; /* sign bit of x */
+ hx &= 0x7fffffff; /* high word of |x| */
+
+ /* special cases */
+ if (hx >= 0x42aeac50) { /* if |x| >= -87.33655f or NaN */
Review Comment:
This block maps every negative-sign NaN to `+0`: after the sign bit is
cleared, a negative NaN skips the positive branch and satisfies the underflow
threshold below. Because the new `-u,expf` option forces this definition into
`doris_be`, ordinary callers can observe the wrong result. Please use the
corrected upstream musl form, returning NaNs before branching on `sign`, and
cover both NaN signs in compatibility tests.
##########
be/src/glibc-compatibility/musl/expf.c:
##########
@@ -0,0 +1,81 @@
+/* origin: FreeBSD /usr/src/lib/msun/src/e_expf.c */
+/*
+ * Conversion to float by Ian Lance Taylor, Cygnus Support, [email protected].
+ */
+/*
+ * ====================================================
+ * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
+ *
+ * Developed at SunPro, a Sun Microsystems, Inc. business.
+ * Permission to use, copy, modify, and distribute this
+ * software is freely granted, provided that this notice
+ * is preserved.
+ * ====================================================
+ */
+
+#include "libm.h"
+
+static const float
+ half[2] = {0.5,-0.5},
+ ln2hi = 6.9314575195e-1f, /* 0x3f317200 */
+ ln2lo = 1.4286067653e-6f, /* 0x35bfbe8e */
+ invln2 = 1.4426950216e+0f, /* 0x3fb8aa3b */
+ /*
+ * Domain [-0.34568, 0.34568], range ~[-4.278e-9, 4.447e-9]:
+ * |x*(exp(x)+1)/(exp(x)-1) - p(x)| < 2**-27.74
+ */
+ P1 = 1.6666625440e-1f, /* 0xaaaa8f.0p-26 */
+ P2 = -2.7667332906e-3f; /* -0xb55215.0p-32 */
+
+float expf(float x)
+{
+ float_t hi, lo, c, xx, y;
+ int k, sign;
+ uint32_t hx;
+
+ GET_FLOAT_WORD(hx, x);
+ sign = hx >> 31; /* sign bit of x */
+ hx &= 0x7fffffff; /* high word of |x| */
+
+ /* special cases */
+ if (hx >= 0x42aeac50) { /* if |x| >= -87.33655f or NaN */
+ if (hx >= 0x42b17218 && !sign) { /* x >= 88.722839f */
+ /* overflow */
+ x *= 0x1p127f;
+ return x;
+ }
+ if (sign) {
+ /* underflow */
+ FORCE_EVAL(-0x1p-149f/x);
+ if (hx >= 0x42cff1b5) /* x <= -103.972084f */
+ return 0;
Review Comment:
This literal zero loses the active rounding mode for finite underflow. With
`FE_UPWARD` and `x = -104.0f`, the exact result is positive but below the
smallest subnormal, so `expf` must return `0x1p-149f`; the `FORCE_EVAL` above
raises the exception but its rounded value is discarded. `WANT_ROUNDING` is
enabled locally, and `exp2f` uses `__math_uflowf(0)` for this case. Please
return the rounding-aware helper here and add a directed-rounding test.
##########
be/cmake/check_glibc_compatibility.cmake:
##########
@@ -0,0 +1,70 @@
+# 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.
+
+if (NOT DEFINED ARTIFACT OR NOT EXISTS "${ARTIFACT}")
+ message(FATAL_ERROR "Cannot check glibc compatibility: '${ARTIFACT}' does
not exist")
+endif()
+
+if (NOT DEFINED BASELINE OR BASELINE STREQUAL "")
+ message(FATAL_ERROR "Cannot check glibc compatibility: BASELINE is not
set")
+endif()
+
+if (NOT DEFINED OBJDUMP OR OBJDUMP STREQUAL "" OR NOT EXISTS "${OBJDUMP}")
+ find_program(OBJDUMP NAMES objdump llvm-objdump REQUIRED)
+endif()
+
+execute_process(
+ COMMAND "${OBJDUMP}" -T "${ARTIFACT}"
+ RESULT_VARIABLE objdump_result
+ OUTPUT_VARIABLE dynamic_symbol_table
+ ERROR_VARIABLE objdump_error)
+
+if (NOT objdump_result EQUAL 0)
+ message(FATAL_ERROR
+ "Cannot read the dynamic symbol table from '${ARTIFACT}':
${objdump_error}")
+endif()
+
+string(REPLACE "\n" ";" dynamic_symbol_lines "${dynamic_symbol_table}")
+set(incompatible_symbols)
+
+foreach (symbol_line IN LISTS dynamic_symbol_lines)
+ # Only undefined symbols are runtime requirements on the target system.
Review Comment:
This `*UND*`-only filter misses version needs carried by ELF copy
relocations. A versioned data import can become an
`R_X86_64_COPY`/`R_AARCH64_COPY` symbol defined in the executable's `.bss`,
while its version index still points into `.gnu.version_r` and the loader still
requires that GLIBC version. For example, `__libc_single_threaded@GLIBC_2.32`
can pass this check and then fail on CentOS 7. Please audit the version-needs
table, for example with `readelf --version-info`, or explicitly cover
copy-relocated symbols, and add a negative data-import test.
##########
be/src/glibc-compatibility/lance_symbol_versions.c:
##########
@@ -90,3 +153,40 @@ DORIS_HIDDEN ssize_t splice(int fd_in, off64_t* offset_in,
int fd_out, off64_t*
size_t length, unsigned int flags) {
return __doris_old_splice(fd_in, offset_in, fd_out, offset_out, length,
flags);
}
+
+// Rust std weak-links copy_file_range and otherwise issues the syscall itself.
+// Provide that syscall path locally so linking on glibc 2.27 does not attach a
+// GLIBC_2.27 version requirement. Old kernels return ENOSYS and Rust falls
back
+// to its generic copy loop.
+DORIS_HIDDEN ssize_t copy_file_range(int fd_in, off64_t* offset_in, int
fd_out, off64_t* offset_out,
+ size_t length, unsigned int flags) {
+ return (ssize_t)syscall(SYS_copy_file_range, fd_in, offset_in, fd_out,
offset_out, length,
+ flags);
+}
+
+// Rust std weak-links this glibc 2.18 entry point and has an internal fallback
+// when it is absent. Since the LDB sysroot exposes it, the final linker would
+// otherwise record GLIBC_2.18. Supply equivalent pthread-key based
registration
+// locally. All callers are linked into doris_be, so dso_symbol tracking for
+// dlclose is intentionally unnecessary.
+// See https://github.com/rust-lang/rust/issues/57497 for more details.
+DORIS_HIDDEN int __cxa_thread_atexit_impl(doris_tls_destructor destructor,
void* object,
+ void* dso_symbol) {
+ (void)dso_symbol;
Review Comment:
`dso_symbol` is what lets glibc pin a DSO until pending TLS destructors
finish. Dropping it leaves only a raw callback pointer. This is reachable here
because `doris_be` exports static libstdc++'s public `__cxa_thread_atexit`
bridge to dlopened native code, while the UDF cache can later `dlclose` those
libraries. A worker can therefore call unmapped code at thread exit. Please
preserve the DSO-lifetime contract, or prevent DSO callers from binding through
this shim, and test `dlclose` with a pending TLS destructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
