Skip to content

Fix compiler-specific coroutine flags in exported CMake targets - #720

Closed
tinadu1018 wants to merge 1 commit into
meta:mainfrom
tinadu1018:fix/gcc-coroutine-export-20261103
Closed

tinadu1018 wants to merge 1 commit into
meta:mainfrom
tinadu1018:fix/gcc-coroutine-export-20261103

Conversation

@tinadu1018

@tinadu1018 tinadu1018 commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Make the -fcoroutines usage requirement depend on the consuming C++ compiler, rather than the compiler that configured FBThrift.

thrift_configure_target currently checks CMAKE_CXX_COMPILER_ID at configure time, then exports a language-only expression:

$<$<COMPILE_LANGUAGE:CXX>:-fcoroutines>

When FBThrift is built with GCC and its installed targets are consumed by Clang, this passes the GCC-specific flag to Clang. A downstream -Werror build then fails:

clang++: error: argument unused during compilation: '-fcoroutines'
[-Werror,-Wunused-command-line-argument]

Observed in Velox's Linux ASAN/UBSAN CI.

Regression history

The flag itself is not new; exporting it to downstream consumers is what changed.

  • v2026.09.07.00 already used -fcoroutines, but through directory-level add_compile_options(). It affected FBThrift's own compilation and was not exported as a requirement for consumers of the installed targets.
  • On September 10, 2026, commit 734f1641, "Express the C++ standard as a target usage requirement," moved these settings into thrift_configure_target. Its PUBLIC call sites also exported the language-only -fcoroutines option.
  • Velox's upgrade to v2026.10.26.00 picked up that change. A GCC-built FBThrift package now passes the flag to Velox's Clang sanitizer build, where -Werror makes it fatal. The configure-time GNU check describes the producer, while the exported expression checks only whether the consumer is compiling C++.

This is a change in the exported build requirements, not a new Velox requirement. The public C++20 requirement is intentional; this PR preserves it while selecting -fcoroutines according to the consuming compiler.

Fix

Replace the configure-time condition with:

$<$<COMPILE_LANG_AND_ID:CXX,GNU>:-fcoroutines>

This evaluates the compiler for each consuming target. GNU C++ compilation retains the option, including when consuming a Clang-built package; Clang and other compilers do not receive it. No compiler warnings are suppressed, and no dependency versions, workflows, runtime code, or xxhash discovery are changed.

A same-compiler build does not expose this boundary: GCC accepts the original flag, while a Clang-configured FBThrift omits it entirely. The existing public Linux workflow configures, builds, and tests one tree rather than installing with GCC and compiling a downstream consumer with Clang.

Validation

Focused Ubuntu Linux (WSL) regression using GCC 13.3 and Clang 18.1.3. Built and installed real thrift_common using the production configuration/export functions, then compiled and ran a C++20 coroutine consumer that calls the installed library, with -Wall -Wextra -Werror and ASAN/UBSAN enabled.

Producer Consumer Before After
GCC GCC Pass Pass
GCC Clang Fails with the error above Pass
Clang GCC Pass Pass
Clang Clang Pass Pass

Also checked the generated compile commands: after the fix, only GNU consumers receive -fcoroutines. This was a focused export regression, not a local run of the full FBThrift test suite.

## Summary
Use COMPILE_LANG_AND_ID:CXX,GNU in thrift_configure_target instead of
checking the producer compiler at configure time. Exported usage
requirements now select -fcoroutines for the consuming GNU compiler,
without passing the GCC-only option to Clang.

## Prompting Intent
The engineer requested a focused upstream FBThrift pull request for the
compiler-flag failure exposed by Velox's Linux sanitizer build, and an
explanation of why same-compiler FBThrift builds can pass while an
installed package fails in a downstream Clang build. Keep the unrelated
cuDF/xxhash discovery issue and the Velox pull request unchanged.

## Linked Sources
- Downstream failure: https://github.com/facebookincubator/velox/actions/runs/41762595093/job/126353579801
- Upstream-fix review question: facebookincubator/velox#19909 (comment)
- Producer configuration: https://github.com/meta/fbthrift/blob/63d52a48493e29f4247fddad9e16c1c3a23bac0c/CMakeLists.txt
- Export helpers: https://github.com/meta/fbthrift/blob/63d52a48493e29f4247fddad9e16c1c3a23bac0c/thrift/CMakeLists.txt
- Real regression component: https://github.com/meta/fbthrift/blob/63d52a48493e29f4247fddad9e16c1c3a23bac0c/thrift/common/CMakeLists.txt
- Public Linux CI: https://github.com/meta/fbthrift/blob/63d52a48493e29f4247fddad9e16c1c3a23bac0c/.github/workflows/linux.yml
- Contribution guidance: https://github.com/meta/fbthrift/blob/63d52a48493e29f4247fddad9e16c1c3a23bac0c/CONTRIBUTING.md

## Rationale
The configure-time GNU condition describes the compiler that builds
FBThrift, not the compiler that later consumes its installed targets.
The language-only generator expression consequently leaks -fcoroutines
from a GCC-built package into Clang consumers, which fail under -Werror.

Evaluate both language and compiler in the exported generator expression.
This keeps the GNU requirement for GNU consumers, including consumers of
a Clang-built package, rather than suppressing warnings or deleting the
GCC requirement. The CMake minimum already supports this expression.

A focused Ubuntu Linux regression built and installed real thrift_common
with the production configure/export functions, then compiled and ran
C++20 coroutine consumers under ASAN, UBSAN, and -Werror. GCC 13.3 and
Clang 18.1.3 passed all four producer/consumer combinations after the fix;
the original GCC-to-Clang combination reproduced the downstream error.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fdd7945e-1c8f-4e79-ac56-0fbeeedaffbd
@meta-cla meta-cla Bot added the CLA Signed label Nov 4, 2026
@meta-codesync

meta-codesync Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

@vitaut has imported this pull request. If you are a Meta employee, you can view this in D129638627.

@meta-codesync meta-codesync Bot closed this in 2e6cd77 Nov 4, 2026
@meta-codesync meta-codesync Bot added the Merged label Nov 4, 2026
@meta-codesync

meta-codesync Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

@vitaut merged this pull request in 2e6cd77.

@vitaut

vitaut commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants