Uh oh!
There was an error while loading. Please reload this page.
branch-4.1: [fix](build) prevent math regression from global lance-c linkage - #67312
Conversation
hello-stephen
commented
Aug 29, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
linrrzqqq
commented
Aug 29, 2026
/review |
There was a problem hiding this comment.
Request changes: this restores the Lance compatibility link path, but the current patch has three blocking correctness problems.
Build/configuration checkpoint: the added trailing dot makes the top-level BE CMake listfile fail to parse in every configuration. After conceptually correcting that typo, I did not find a separate archive-order or final-consumer bypass: the compatibility/non-compatibility, macOS, x86_64/aarch64, sanitizer, test, benchmark, and tool link paths retain providers for the supported Lance interface.
Runtime/lifecycle checkpoint: the new PATH search can allocate 4352 bytes on the existing 1 KiB shared clone stack, corrupting the suspended parent's stack. The force-interposed posix_spawnp also routes ordinary Rust 1.91 process creation through a deliberately reduced clang-only backend that does not execute file actions, mishandles signal state/attributes, and reports clone failures incorrectly.
Compatibility checkpoint: I rechecked the direct preadv and splice syscall signatures, errno conversion, and x86_64/aarch64 argument packing; no separate ABI defect was substantiated. The cancellation delta has no demonstrated repository or pinned Lance caller, so it is not raised as a fourth issue.
Test checkpoint: existing CI/configuration and BE-UT linkage do not execute these new spawn branches. Please add a long-PATH regression and runtime coverage for pipes/dup2, cwd actions, signal mask/default attributes, and clone failure propagation as part of the fixes.
User focus: no additional user-provided focus points were supplied.
Review completion: complete after two rounds. Every candidate was independently verified, deduplicated, accepted, or dismissed with evidence, and all Round 2 reviewers reported NO_NEW_VALUABLE_FINDINGS.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e9f8042 to
318ad18Comparelinrrzqqq
commented
Aug 29, 2026
run buildall |
hello-stephen
commented
Aug 29, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
Aug 30, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
HappenLee
commented
Aug 31, 2026
Review 结论当前版本我不建议合入,应该 Request changes。有两个确定的正确性阻塞问题。
新增的 /tmp/doris-pr-67312/be/src/glibc-compatibility/musl/posix_spawnp.c:15 调用现有 __posix_spawnx,但这个实现自己就注明只供 clang driver 使用、删掉了一半 musl 逻辑: /tmp/doris-pr-67312/be/src/glibc-compatibility/musl/posix_spawn.c:1 具体问题包括:
我用 PR 中的源码做了最小运行探针:添加 dup2 file action 后,posix_spawnp() 返回成功,但重定向 pipe 收到 0 字节,证明 action 被静默忽略。 而且这些源码被放进 OBJECT library 强制链接,会全局覆盖进程里的 posix_spawn/posix_spawnp,影响范围不只是 Lance。
/tmp/doris-pr-67312/be/src/glibc-compatibility/musl/execvpe.c:31 在子进程栈上创建: char candidate[path_len + file_len + 1]; 最大约为: PATH_MAX + NAME_MAX + 1 = 4352 bytes 但 /tmp/doris-pr-67312/be/src/glibc-compatibility/musl/posix_spawn.c:65 给 clone() 的栈只有 1024 字节。 我用约 4090 字节的 PATH 实测,探针直接以 139/SIGSEGV 退出。由于使用了 CLONE_VM | CLONE_VFORK,还存在破坏父进程内存的风险。 这是复制代码时丢失了上下文约束:完整 musl 实现分配的是 1024 + PATH_MAX,而 Doris 现有精简版只有 1024。 |
ad72590 to
4237cfaComparelinrrzqqq
commented
Aug 31, 2026
run buildall |
zhangstar333
commented
Sep 2, 2026
/review |
There was a problem hiding this comment.
Reviewed exact head 4237cfa09bd480fd70cdefb3492dbe15467e6063 against base 2c3ae38c3448aabb2a2fcd7af2e14340a779c375.
No blocking correctness issue found; no inline comments. Review opinion: the current head is acceptable from the code-review perspective.
Build and configuration checkpoint: GLIBC_COMPATIBILITY=OFF retains the existing grouped m; lance_c path, while the enabled Linux path contains lance_c exactly once after glibc-compatibility-explicit, glibc-compatibility, and -lm. macOS remains on the forced-OFF path. Both ARM sanitizer branches retain the adapter object, and all final BE, test, benchmark, and optional-tool consumers receive the completed link list.
Runtime and ABI checkpoint: all six hidden adapters match the libc prototypes and preserve return/errno behavior. The x86_64 and aarch64 version selections exist at the declared ABI versions, the versioned aliases cannot recurse into the hidden wrappers, and posix_spawnp now forwards to glibc full spawn handling rather than the earlier reduced Doris backend. The x86 GLIBC_2.2.5 ENOEXEC shell fallback is a real historical semantic difference, but no current Doris or pinned production Lance path was found that presents an ENOEXEC target.
Compatibility and lifecycle checkpoint: the source is removed from the broad STATIC archive glob and included exactly once in both explicit OBJECT variants, avoiding duplicate/getrandom extraction. No shared mutable state, ownership, shutdown, retry, persistence, transaction, RPC, or rolling-version mechanism is changed.
Performance checkpoint: the new order scans Doris compatibility math implementations before system libm, then scans lance_c, matching the PR goal without exposing a separate archive-consumer bypass.
Test and proof checkpoint: the current exact-head compile status passes. Per the review-runner instruction, no builds were run during this review. CI has no automated final-ELF assertion for selected math providers, adapter visibility, or the maximum GLIBC version across every architecture/variant; this remains a non-blocking coverage limitation rather than a demonstrated defect.
Existing-thread checkpoint: the prior CMake parse typo and the unsafe PATH-stack/reduced-spawn implementation are absent from the current authoritative diff, so those three earlier blocking threads are addressed and were not duplicated.
User focus: no additional user-provided focus points were supplied.
Review completion: complete after one convergence round. Both normal full-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS; every main risk was independently verified and dismissed with evidence, leaving no unresolved candidate.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Sep 6, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #67237