| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Use TLS descriptors as required by thread context.
CI Test ResultsRun: #27688371967 | Commit: 4eca7b4 | Duration: 12m 51s (longest job)
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-06-17 12:35:52 UTC |
Sorry, something went wrong.
| // This is required by thread context (https://github.com/open-telemetry/opentelemetry-specification/pull/4947) | ||
| // The dialect is only supported by GCC and clang >= 19, so probe the | ||
| // compiler before adding the flag rather than relying on its version. | ||
| if (architecture == Architecture.X64 && PlatformUtils.supportsTlsDialectGnu2(compiler)) { |
There was a problem hiding this comment.
[Sphinx Review — MEDIUM] On x86_64 Linux the gnu2 TLS dialect is documented as required for thread context, yet supportsTlsDialectGnu2 returns false for any environmental probe failure (timeout, swallowed exception, process-spawn restriction). In all such cases the flag is silently dropped and the build still succeeds, producing a binary whose thread-context feature is silently broken with no warning.
Suggestion: Log a lifecycle/warn message when the probe returns false on x86_64 Linux so a silent drop of the required flag is visible, or distinguish 'compiler genuinely rejects flag' from 'probe could not run' and fail loudly on the latter.
Sorry, something went wrong.
There was a problem hiding this comment.
-mtls-dialect=gnu2 is now gated on clang version and an exception is thrown when clang version cannot be determined.
Sorry, something went wrong.
| * parsing version strings (it also handles vendor-patched toolchains). | ||
| * Always false off x86_64 Linux — the flag is ELF/x86_64 specific. | ||
| */ | ||
| fun supportsTlsDialectGnu2(compiler: String): Boolean { |
There was a problem hiding this comment.
[Sphinx Review — MEDIUM] supportsTlsDialectGnu2 spawns a compiler subprocess each time it is called and is invoked once per build configuration (release, debug, asan, tsan, fuzzer = up to 5 times per build). The result depends only on the fixed compiler path and host platform, so it is recomputed redundantly. There is no memoization, in contrast to currentPlatform/currentArchitecture which use by lazy.
Suggestion: Memoize the result keyed by compiler path (e.g. a private val tlsGnu2Cache = ConcurrentHashMap<String, Boolean>() with cache.getOrPut(compiler) { ... }) so the probe runs at most once per distinct compiler per build.
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think that's an issue, just getting the compiler version is fast.
Sorry, something went wrong.
| * Always false off x86_64 Linux — the flag is ELF/x86_64 specific. | ||
| */ | ||
| fun supportsTlsDialectGnu2(compiler: String): Boolean { | ||
| if (currentPlatform != Platform.LINUX || currentArchitecture != Architecture.X64) { |
There was a problem hiding this comment.
[Sphinx Review — LOW] The X64 architecture gate is duplicated across two functions keyed off different sources: commonLinuxCompilerArgs checks the target architecture parameter, while supportsTlsDialectGnu2 checks the host currentArchitecture. The two guards would silently diverge under cross-compilation where host ≠ target.
Suggestion: Either remove the inner X64 guard from supportsTlsDialectGnu2 (let callers decide when to probe) and update the KDoc, or add a comment explaining why the redundant guard is intentional.
Sorry, something went wrong.
There was a problem hiding this comment.
Cross-compilation does not seem to be supported, but removed the check from commonLinuxCompilerArgs.
Sorry, something went wrong.
| val process = ProcessBuilder( | ||
| compiler, | ||
| "-mtls-dialect=gnu2", | ||
| "-fsyntax-only", |
There was a problem hiding this comment.
[Sphinx Review — LOW] The probe uses -fsyntax-only, which skips code generation. -mtls-dialect is a codegen option; a compiler (notably clang) may accept it under -fsyntax-only yet fail or ignore it during an actual compile, so the probe can over-report support.
Suggestion: Compile to an object (-c -o <tmp.o>) instead of -fsyntax-only so codegen actually exercises the TLS dialect, then check exit status.
Sorry, something went wrong.
There was a problem hiding this comment.
Switched to checking clang version.
Sorry, something went wrong.
| "-mtls-dialect=gnu2", | ||
| "-fsyntax-only", | ||
| testFile.toAbsolutePath().toString() | ||
| ).redirectErrorStream(true).start() |
There was a problem hiding this comment.
[Sphinx Review — LOW] The probe process uses redirectErrorStream(true) but never drains process.inputStream before waitFor. If the compiler produced enough output to fill the OS pipe buffer, the child would block on write and the 10s timeout would expire, silently returning false and disabling the TLS flag.
Suggestion: Drain the combined stream before waitFor (e.g. process.inputStream.bufferedReader().readText()), or redirect output to ProcessBuilder.Redirect.DISCARD so the child never blocks on a full pipe.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed (same issue is present in isCompilerAvailable)
Sorry, something went wrong.
The GNU2 TLS dialect is required by thread context but is only supported by GCC and clang >= 19, so detect the compiler before adding the flag on x86_64 Linux. Compiler detection now parses `<compiler> --version` into a CompilerInfo (executable, clang-family flag, major version); the family is read from the banner rather than guessed from the executable name. Non-clang compilers are assumed to support the dialect; clang is gated on its parsed major version and the build fails loudly if that version cannot be determined. Common compiler args are also consolidated into a single commonCompilerArgs computed once in setupStandardConfigurations and threaded into the per-config factories. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
What
Compile the profiler shared library with -mtls-dialect=gnu2 on x86_64 Linux.
Why
TLS descriptors (GNU2 dialect) is required for thread context sharing.
How
Testing
🤖 Generated with Claude Code