Skip to content

CDRIVER-5891 extend C standard coverage up to C23 #1890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Feb 27, 2025

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Feb 26, 2025

Summary

Resolves CDRIVER-5891.

Extends std-matrix Evergreen tasks to support compilation with C99 through C23 using GCC, Clang, and MSVC.

Drive-By EVG Config and Script Improvements

To facilitate the expansion of compilation coverage to newer versions of Visual Studio, compile scripts (beyond std-matrix tasks) are updated to utilize the CMake CMAKE_GENERATOR and CMAKE_GENERATOR_PLATFORM environment variables to specify the Visual Studio version and target architecture (equivalent to -G + -A; similar changes to mongodb/mongo-cxx-driver#1082). The definition of CC and CXX variables are also moved into the config generator to ensure copmiler consistency (no need for if [[ "$CC" == "gcc" ]]-esque logic in scripts), including specific compiler version requests (e.g. clang-12). The list of distros in distros.py was also refactored to use the ls_distro pattern to specify small+large distro pairs (similar changes to mongodb/mongo-cxx-driver#1082).

Compiler Support

Reliably deducing compiler support for any given standard is a chore, especially given ongoing partial implementations of the C2x standard (as well as the C17 standard for older compiler versions). For example, support for static_assert is already available as early as GCC 9, while other C2x features such as [[nodiscard]], __VA_OPT__, and nullptr are implemented by GCC 14, with more features such as #embed provided by GCC 15, and etc. (see major version changelogs for 15.1, 14.1, 13.1, etc.). Similarly, static_assert and [[nodiscard]] are supported by Clang 9, with a scattering of C2x features being partially implemented across versions up to Clang 20 (technically even C11 standard support is still in a partial state).

Instead of laborously deducing compiler support for language standards or features, detection of C standard support is completely deferred to CMake's built-in feature detection routines (regardless of what flag may be required: -std=c11, -std=c1x, -std=c17, -std=c2x, or -std=c23). When a compiler does not support a given standard or feature, CMake will emit errors similar to the following (e.g. for GCC):

CMake Error in [...]/CMakeFiles/CMakeScratch/TryCompile-cX1jWr/CMakeLists.txt:
  Target "cmTC_700ec" requires the language dialect "C23" .  But the current
  compiler "GNU" does not support this, or CMake does not know the flags to
  enable it.
CMake Error at [...]/Modules/CheckSymbolExists.cmake:154 (try_compile):
  Failed to generate test project build system.
Call Stack (most recent call first):
  [...]/Modules/CheckSymbolExists.cmake:66 (__CHECK_SYMBOL_EXISTS_IMPL)
  src/libbson/CMakeLists.txt:93 (check_symbol_exists)

The absence of C standards in the matrix for older compiler versions as proposed by this PR was entirely decided by the presence CMake errors similar to the one shown above.

Disabled Warnings

The public-header-warnings (+ tests and examples) are added to the C standard task matrix. This motivated adding some new warning flags to be explicitly disabled for Clang with -Weverything:

  • -Wc++98-compat-pedantic: we are not interested in continuing to support C++98 in either the C or C++ Driver. (Note: C++11 references the C99 spec.)
  • -Wpre-c2x-compat: we actively test pre-C2x compatibility and do not need to rely on this warning.
    • Warning for use of bool despite pre-C2x #define bool _Bool and inclusion of C99 <stdbool.h> is unnecessarily noisy.
  • -Wunsafe-buffer-usage: too aggressive, warning on nearly any subscript of a pointer. Impractical to address this warning.

C2x Boolean Compatibility

The following code:

bson_t bson;
bsonParse (bson, require (true, do ((void) 0)));
//                        ^~~~

and similarly:

bson_t other = BSON_INITIALIZER;
bsonBuildDecl (bson, insert (other, true));
//                                  ^~~~

produces the following compiler error with GCC 11 and GCC 12 given -std=c2x:

error: pasting "_bsonPredicate_Condition_" and "(" does not give a valid preprocessing token
  704 | #define _bsonPredicate(P) _bsonPredicate_Condition_##P
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~

due to GCC 11 and GCC 12 defining boolean constants as:

#define true    ((_Bool)+1u)
#define false   ((_Bool)+0u)

instead of as a simple 1 and 0 (as in C17 and prior) or as keywords (in GCC 13 and newer with -std=c2x or -std=c23). The "(" mentioned in the error message is the first ( in ((_Bool)+1u).

According to https://github.com/gcc-mirror/gcc, this change is due to this commit on Oct 29, 2020, which was motivated by this feature request. The ((_Bool)+1u) form was proposed by N2393 as an intermediate solution until the compiler has completed implementing the booleans as keywords. It seems <stdbool.h> for GCC 11 and GCC 12 implement this intermediate solution. These changes were later superceded by this commit on Sep 7, 2022, which remove the macros entirely for -std=c2x. This seems to correspond to the GCC 13 release and onwards which implemented the keyword-based solution.

Because the language does not support contextually redefining true and false as "simple" macros wherever the BSON DSL macros may be expanded (that is, we cannot #define in a #define), a solution that attempts to preserve the use of true and false as unconditional DSL predicates was considered but rejected. Instead, the special true and false DSL predicates are renamed to new and equivalent DSL predicates: always and never.

Therefore, the code above must now be written instead as:

bson_t bson;
bsonParse (bson, require (always, do ((void) 0)));
//                        ^~~~~~

and similarly:

bson_t other = BSON_INITIALIZER;
bsonBuildDecl (bson, insert (other, always));
//                                  ^~~~~~

@eramongodb eramongodb self-assigned this Feb 26, 2025
@eramongodb
Copy link
Contributor Author

Verified by this patch. The C Driver EVG Project settings have been updated to include the std-matrix variant in PR patch builds.

Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 65 to 76
RHEL_DISTROS = []
RHEL_DISTROS += ls_distro(name='rhel76', os='rhel', os_type='linux', os_ver='7.6')
RHEL_DISTROS += ls_distro(name='rhel80', os='rhel', os_type='linux', os_ver='8.0')
RHEL_DISTROS += ls_distro(name='rhel84', os='rhel', os_type='linux', os_ver='8.4')
RHEL_DISTROS += ls_distro(name='rhel90', os='rhel', os_type='linux', os_ver='9.0')
RHEL_DISTROS += ls_distro(name='rhel91', os='rhel', os_type='linux', os_ver='9.1')
RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.2')
RHEL_DISTROS += ls_distro(name='rhel93', os='rhel', os_type='linux', os_ver='9.3')
RHEL_DISTROS += ls_distro(name='rhel94', os='rhel', os_type='linux', os_ver='9.4')
RHEL_DISTROS += ls_distro(name='rhel95', os='rhel', os_type='linux', os_ver='9.5')
RHEL_DISTROS += ls_distro(name='rhel8.9', os='rhel', os_type='linux', os_ver='8.7')
RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.0')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the variadic splatting operator * in list/tuple literals (or ** in dict literals) to expand any iterable object into zero or more inline elements:

Suggested change
RHEL_DISTROS = []
RHEL_DISTROS += ls_distro(name='rhel76', os='rhel', os_type='linux', os_ver='7.6')
RHEL_DISTROS += ls_distro(name='rhel80', os='rhel', os_type='linux', os_ver='8.0')
RHEL_DISTROS += ls_distro(name='rhel84', os='rhel', os_type='linux', os_ver='8.4')
RHEL_DISTROS += ls_distro(name='rhel90', os='rhel', os_type='linux', os_ver='9.0')
RHEL_DISTROS += ls_distro(name='rhel91', os='rhel', os_type='linux', os_ver='9.1')
RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.2')
RHEL_DISTROS += ls_distro(name='rhel93', os='rhel', os_type='linux', os_ver='9.3')
RHEL_DISTROS += ls_distro(name='rhel94', os='rhel', os_type='linux', os_ver='9.4')
RHEL_DISTROS += ls_distro(name='rhel95', os='rhel', os_type='linux', os_ver='9.5')
RHEL_DISTROS += ls_distro(name='rhel8.9', os='rhel', os_type='linux', os_ver='8.7')
RHEL_DISTROS += ls_distro(name='rhel92', os='rhel', os_type='linux', os_ver='9.0')
RHEL_DISTROS = [
*ls_distro(name='rhel76', os='rhel', os_type='linux', os_ver='7.6'),
*ls_distro(name='rhel80', os='rhel', os_type='linux', os_ver='8.0'),
*ls_distro(name='rhel84', os='rhel', os_type='linux', os_ver='8.4'),
...
]

Comment on lines +113 to +118
if [[ "${CMAKE_GENERATOR:-}" =~ "Visual Studio" ]]; then
# MSBuild needs additional assistance.
# https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/
export UseMultiToolTask=1
export EnforceProcessCountAcrossBuilds=1
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move to Ninja on Windows, eventually. Maybe someday.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I really like the comprehensive std-c* tasks with the comments documenting the compiler versions.

('rhel94', 'gcc', None, [99, 11, 17, 23]), # GCC 11.4 (max: C2x)
('rhel95', 'gcc', None, [99, 11, 17, 23]), # GCC 11.5 (max: C2x)

('windows-vsCurrent', 'vs2017x64', None, [99, 11, 17, 'latest']), # Max: C17, clatest (C2x)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "latest" to "clatest"? The message in compile-std.sh refers to "clatest" (but checks for "latest"):

if [[ "${C_STD_VERSION}" == "latest" ]]; then
  [[ "${CMAKE_GENERATOR:-}" =~ "Visual Studio" ]] || {
    echo "C_STD_VERSION=clatest is only supported with Visual Studio generators" 1>&2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixed up the /std:clatest flag with the C_STD_VERSION=latest variable. Fixed.

@eramongodb eramongodb merged commit 9b19be9 into mongodb:master Feb 27, 2025
1 check was pending
@eramongodb eramongodb deleted the cdriver-5891 branch February 27, 2025 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants