Repository navigation
[bugfix](glibc) fix glibc compatible bugs - #67701
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: capped/incomplete after three rounds. The final allowed round produced one additional accepted issue, so I am not claiming convergence. All five substantiated issues found so far are included inline.
The GLIBC_2.17 symbol-table goal is not safely met yet: the new gate has a copy-relocation false negative, and the forced runtime replacements change math and TLS lifecycle semantics.
Critical checkpoint conclusions:
- Goal and proof: The link roots and POST_BUILD hook target the correct final executable, and compile CI passes, but the gate misses a real class of version needs and the replacements are not behaviorally equivalent. The PR adds no semantic or lifecycle test.
- Scope: The six-file change is focused under
GLIBC_COMPATIBILITY, but forcedexpfand the TLS ABI shim affect process-wide and dlopened-native callers. - Concurrency:
pthread_onceand per-thread key storage avoid a shared-list race and preserve reentrant LIFO registration. Pending TLS callbacks can still race with DSO unload because DSO ownership is discarded. No separate deadlock or lock-order issue was found. - Lifecycle/static state: Key initialization is safe, but initial-thread TLS destruction is ordered incorrectly and unloadable DSO callbacks are not pinned. No separate cross-TU initialization issue was found.
- Configuration: No new runtime configuration is added; the existing compile-time compatibility switch gates the behavior.
- Compatibility: The raw
copy_file_rangewrapper preserves Rust's old-kernel fallback on x86_64/aarch64. This does not touch wire/storage formats or rolling-upgrade protocols. The math/TLS defects and checker escape remain blocking ABI-compatibility issues. - Parallel paths and conditions: All
DORIS_LINK_LIBSconsumers receive the roots. The package-wide shared-library scan gap was examined but not reported because no current incompatible packaged library is established. The NaN and directed-underflow conditions are incorrect as noted inline. - Tests/results: The author supplied an
objdumpresult and compile CI passes; BE unit and regression jobs are still pending. There are no negative copy-relocation, floating special-value/rounding, main-thread termination-order, or DSO-unload/TLS tests. No expected-result file changes are involved. - Error handling/observability: Registration failures abort instead of continuing partially initialized, and build-hook diagnostics are adequate. No additional metrics or logs are needed once correctness is fixed.
- Persistence/data/FE-BE variables: Not applicable; there are no transaction, metadata, data-write, storage-format, RPC, or cross-process variable changes.
- Performance/other: No separate blocking CPU or memory regression was substantiated. The final changed-file and live-thread sweep found no duplicate or unresolved candidate, but the round cap requires this review to remain incomplete.
| hx &= 0x7fffffff; /* high word of |x| */ | ||
|
|
||
| /* special cases */ | ||
| if (hx >= 0x42aeac50) { /* if |x| >= -87.33655f or NaN */ |
Contributor
There was a problem hiding this 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.
| // 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) { |
Contributor
There was a problem hiding this 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.
| // 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; |
Contributor
There was a problem hiding this 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.
| /* underflow */ | ||
| FORCE_EVAL(-0x1p-149f/x); | ||
| if (hx >= 0x42cff1b5) /* x <= -103.972084f */ | ||
| return 0; |
Contributor
There was a problem hiding this 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.
| set(incompatible_symbols) | ||
|
|
||
| foreach (symbol_line IN LISTS dynamic_symbol_lines) | ||
| # Only undefined symbols are runtime requirements on the target system. |
Contributor
There was a problem hiding this 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.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
None
- Test: Manual test
- Static audit of Java/Python UDF loading and symbol lookup paths
- ./build.sh --be attempted but stopped at Java version detection because this macOS environment has no Java runtime
- Behavior changed: Yes; dlopened libraries can no longer bind arbitrary doris_be symbols
- Does this need documentation: No
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16810 ms |
Contributor
TPC-DS: Total hot run time: 82827 ms |
Contributor
ClickBench: Total hot run time: 14.83 s |
Contributor
There was a problem hiding this comment.
Review status: converged on c58ca63896196dc64c883c8cecc9bfbf7d1c6422 after two rounds. I found two new blocking issues and included both inline. The existing copy-relocation/version-needs issue and initial-thread TLS ordering issue also remain applicable on this head. The two earlier expf value/rounding comments are fixed by the updated upstream-style implementation. Removing executable-wide exports eliminates the supported external-DSO binding path behind the earlier DSO-lifetime concern, but it introduces the stripped-symbolization regression reported here.
Critical checkpoint conclusions:
- Goal and proof: The forced archive roots and raw
copy_file_rangeadapter are coherent, but the GLIBC 2.17 goal is not safely met while the final-ELF audit misses copy relocations and the forced libm definitions violate glibc's errno contract. No final-linked semantic test proves the new shims. - Scope: The six changed files are related to the compatibility fix, although forced public math definitions and the TLS runtime shim are process-wide changes.
- Concurrency:
pthread_onceplus per-thread lists avoid a shared-list race, and callback re-registration preserves LIFO order. The worker-key ordering matches Rust's old-system fallback; no separate reachable worker defect was established. The existing initial-thread ordering issue remains. - Lifecycle/static state: TLS entry ownership and repeat registration are otherwise sound. The prior external-DSO scenario is no longer supported after executable exports are removed; no additional initialization-order or unload path survived review.
- Configuration: No new runtime configuration is added.
GLIBC_COMPATIBILITYandSTRIP_DEBUG_INFOare existing build-time choices; the latter exposes the new symbolization regression. - Compatibility: The
copy_file_rangesignature and errno fallback match Rust 1.91 on supported x86_64/aarch64 targets. There are no wire, storage-format, or rolling-upgrade changes. The remaining ELF audit, libm errno, and main-thread TLS issues are ABI blockers. - Parallel paths and conditions: All executable consumers of
DORIS_LINK_LIBSreceive the same forced roots. The package-wide shared-library concern was rechecked but not reported because no current incompatible packaged library was established. Normal/compatibility-off, test/benchmark/tool, macOS, both Linux architectures, old-kernel fallback, and stripped-package paths were considered. - Tests/results: No test file changed. The commit records that
./build.sh --bewas attempted but stopped at Java detection; current checks confirm formatting, style, license, and secret scanning, while BE build/runtime tests are not demonstrated. Missing coverage includes copy-relocation negatives, final-linked libm errno/domain/range behavior, stripped-package symbolization, main-thread teardown ordering, and old-kernel fallback. No expected-result files are involved. - Error handling and observability: Registration/setup failures abort rather than continuing partially initialized, and build-hook command failures are diagnostic. Runtime observability regresses in stripped builds as reported inline.
- Persistence/data/FE-BE variables: Not applicable; no transaction, metadata, data-write, storage, RPC, or cross-process variable behavior changes.
- Performance/other: No separate blocking CPU, allocation, lock, or complexity regression was substantiated. The final changed-file and live-thread sweep left no unresolved or duplicate candidate.
- User focus: No additional focus was provided; the complete PR was reviewed.
| # native UDFs (runtime/user_function_cache.cpp dlopens them), and those use the Doris UDF | ||
| # ABI, which has nothing to do with RocksDB. Crash stacks do not need it either -- they are | ||
| # symbolized from debug info, which is why they name even anonymous-namespace functions. | ||
| # Keep doris_be symbols private. Java UDF callbacks are registered explicitly through JNI and |
Contributor
There was a problem hiding this comment.
Removing executable exports also removes the fallback that keeps Doris function names in .dynsym, but the supported STRIP_DEBUG_INFO=ON package cannot load its separate symbols in-process: this file installs lib/debug_info/doris_be.dbg, while SymbolIndex only checks an adjacent doris_be.debug, /usr/lib/debug/..., or a build-id path and does not follow .gnu_debuglink. After --strip-unneeded, it therefore falls back to a stripped executable with almost no Doris symbols, so exception/crash stacks and the thread-stack endpoint render most frames as ?. Please make SymbolIndex locate the packaged .dbg file (or otherwise preserve the needed symbol source) before dropping ENABLE_EXPORTS, and cover the stripped layout in a test.
Contributor
Author
There was a problem hiding this comment.
It is an old problem already exsits in the previous version. And it only happens when set STRIP_DEBUG_INFO=ON, but actually doris always set STRIP_DEBUG_INFO=OFF
| # front so the linker extracts Doris' musl implementations instead of binding | ||
| # late references to the toolchain's libm. | ||
| set(DORIS_LINK_LIBS ${DORIS_LINK_LIBS} | ||
| -Wl,-u,logf |
Contributor
There was a problem hiding this comment.
These -u roots force musl's public math definitions into every linked executable, but the local domain/range helpers only raise floating-point exceptions and never set errno. Code compiled against glibc therefore sees regressions across this block: for example expf(100)/exp2f(128) and zero logs leave errno unchanged instead of ERANGE, while negative logs or powf(-1, 0.5) omit EDOM. Please preserve glibc's errno contract for all five forced functions and add final-linked domain/range tests; fixing only the numerical result or only expf would leave the other interposed symbols incompatible.
Contributor
Author
There was a problem hiding this comment.
musl does not make sure that, so we will not deal with it in doris.
Contributor
Author
|
run buildall |
hello-stephen
approved these changes
Sep 9, 2026
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 16906 ms |
Contributor
TPC-DS: Total hot run time: 83083 ms |
Contributor
ClickBench: Total hot run time: 14.75 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
1 similar comment
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Felix-Gong
pushed a commit
to Felix-Gong/doris
that referenced
this pull request
Sep 11, 2026
### What problem does this PR solve? 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. See rust-lang/rust#57497 for more details. I have checked the output. objdump -T output/be/lib/doris_be | grep -o 'GLIBC_[0-9.]*' | sort -Vu | tail GLIBC_2.3 GLIBC_2.3.2 GLIBC_2.3.3 GLIBC_2.3.4 GLIBC_2.4 GLIBC_2.5 GLIBC_2.7 GLIBC_2.8 GLIBC_2.9 GLIBC_2.10 Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
2 of 3 tasks
Felix-Gong
pushed a commit
to Felix-Gong/doris
that referenced
this pull request
Sep 14, 2026
### What problem does this PR solve? 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. See rust-lang/rust#57497 for more details. I have checked the output. objdump -T output/be/lib/doris_be | grep -o 'GLIBC_[0-9.]*' | sort -Vu | tail GLIBC_2.3 GLIBC_2.3.2 GLIBC_2.3.3 GLIBC_2.3.4 GLIBC_2.4 GLIBC_2.5 GLIBC_2.7 GLIBC_2.8 GLIBC_2.9 GLIBC_2.10 Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
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.
See rust-lang/rust#57497 for more details.
I have checked the output.
objdump -T output/be/lib/doris_be | grep -o 'GLIBC_[0-9.]*' | sort -Vu | tail
GLIBC_2.3
GLIBC_2.3.2
GLIBC_2.3.3
GLIBC_2.3.4
GLIBC_2.4
GLIBC_2.5
GLIBC_2.7
GLIBC_2.8
GLIBC_2.9
GLIBC_2.10
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)