Skip to content
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

Give user a better error message when the selected compiler is missing required functionality #40

Open
camio opened this issue Oct 1, 2024 · 17 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@camio
Copy link
Member

camio commented Oct 1, 2024

When the user selects a compiler+flags combination that is insufficient to build the project, they're left with a build error that provides no hint that the compiler+flags combination is the problem. This frequently comes up when a user doesn't use -DCMAKE_CXX_STANDARD when invoking CMake. See #38 and bemanproject/inplace_vector#6 for examples of this.

@ComicSansMS and @DeveloperPaul123 suggested using cmake-compile-features with language standard arguments, but this has the side-effect of modifying the compiler flags selected by the user running CMake which can create incompatibilities.

This task is to incorporate a try_compile in CMakeLists.txt that attempts to compile a C++ file with the required language features. If it fails, a CMake-time error can be emitted that suggests the user use -DCMAKE_CXX_STANDARD. For beman.example, try_compile will verify namespace foo::bar compatibility.

See #38 (comment) and bemanproject/inplace_vector#6 (comment) for some discussions on this.

@bretbrownjr
Copy link
Contributor

Makes sense, and we probably want to invent a project structure to make the test source files discoverable or configurable and a CMake module to drive the testing.

I don't like the idea of copy/pasting this into each library.

However, we can start with a one-off implementation in exemplar and factor it out into something reusable across libraries later.

@wusatosi
Copy link
Member

wusatosi commented Oct 2, 2024

An alternative would be to compile a C++ project with bunch of feature tests.

E.g.

#ifndef __cpp_constexpr
static_assert(false, "Compiler must support constexpr");
#endif

@bretbrownjr
Copy link
Contributor

I like that idea better when possible.

Pros: It's more standard and portable, requiring less machinery.

On the other hand: It cannot be as specific (i.e., certain constructs that might have quality of implementation issues), and it cannot test for features that do not have standard macros.

@wusatosi
Copy link
Member

wusatosi commented Oct 2, 2024

Feature test need to work with a minimum standard C++ version, otherwise we may need to exhaustively check basic functionalities (e.g. constexpr).

Agree with @bretbrownjr that feature test cannot test for quality of implementation issue. I believe most new language feature come with a feature test flag.

The quality of implementation issue will need to be resolved by a minimum compiler version, there's no way around it. This approach is not descriptive to why the user's compiler is not supported, but is also an easy-to-maintain and testable alternative. And honestly this isn't too hard to figure out.

@ComicSansMS
Copy link
Contributor

Repeating my comment from #38:

I don't quite follow the rationale for not wanting to put the minimum language standard in the CMake script itself. You have a de-facto PUBLIC requirement on the language version in the library, whether you advertise it through the build system or not. Clients that are not at least using C++17 will not be able to consume the library.

Imho, if clients want to ensure that they always compile with a specific language version, that is the responsibility of the client's build system, not of beman.

I don't see any benefit in pulling this option to the command line, when it is a mandatory option and forgetting to specify it will just stop the library from building. Imho, command line options should be used for optional things, e.g. whether you want to build with or without tests enabled, not mandatory ones.

@DeveloperPaul123
Copy link
Member

I agree with @ComicSansMS . I don't really see a reason for not requiring a minimum C++ standard version when building a Beman project. We can use the C++ standard flags in a way that they will not be tied to the library itself so that it doesn't pollute down stream build systems for our consumers. Instead we can use these flags privately when building tests and examples inside a Beman project.

I don't see any benefit in pulling this option to the command line, when it is a mandatory option and forgetting to specify it will just stop the library from building. Imho, command line options should be used for optional things, e.g. whether you want to build with or without tests enabled, not mandatory ones.

I also agree with this. I think CMAKE_CXX_STANDARD should be part of the CMakeLists.txt instead of having to pass it in through the command line.

@camio
Copy link
Member Author

camio commented Oct 2, 2024

I don't quite follow the rationale for not wanting to put the minimum language standard in the CMake script itself.

My understanding is that using target_compile_features could result in the following two situations which would be prevented by what is proposed in this issue. Does this help?

Situation 1: exploits at version upgrade

Consider a large project Big that produces a DLL and, by policy, uses a compiler with C++17 mode flags.

Big decides to use beman.hip_hop for some functionality and incorporates it using add_subdirectory(beman.hip_hop) and the beman::hip_hop target. So far everything is fine because beman.hip_hop uses target_compile_features(beman.hip_hop PUBLIC cxx_std_17) which is compatible with Big's flags.

Now beman.hip_hop developers decide to take advantage of a C++ 20 feature and change the target property to cxx_std_20. Big updates and everything compiles properly and all tests pass because depending on beman::hip_hop silently changes the build flags for all of Big to use -std=c++20.

The Big dll gets deployed and linked against executables built for C++17. Unfortuately, there was an ABI break between -std=c++17 and -std=c++20 for this platform. This causes an exploitable memory access violation and I get another damn letter saying my kids' personal data has been compromised.

Situation 2: bad error message with incompatible compiler+flags combination

Someone compiles beman.hip_hop using a version of gcc that supports C++17, but not C++20. beman.hip_hop uses target_compile_features(beman.hip_hop PUBLIC cxx_std_20) but, because this gcc version doesn't have a -std=c++20 flag, cmake inserts the -std=c++17 flag instead. Instead of failing at CMake time, there is a failure at build time because a C++20 feature is being used. The user isn't given a hint that the compiler itself is the problem.

@DeveloperPaul123
Copy link
Member

Thanks for the examples @camio . I think I misunderstood a little bit and I want to clarify my stance.

For INTERFACE targets (i.e. header only libraries) I'm of the opinion that we should not set compile feature flags on the library target itself as this will propagate publicly as David outlined. Instead, we set the necessary flags in the build targets when doing development (i.e. the tests and/or examples). This is what I meant to say in my previous comment.

For library targets, these flags should be set privately.

Of the options above, I do like the idea of using feature macros where possible, but as pointed out, we can't test for everything. It seems that try_compile will offer the clearest information to users in the event they try to use a compiler that does not support all the features needed for a specific Beman library.

@wusatosi
Copy link
Member

#56 Maybe a good example on how we approach this using feature tests (note that feature test macro is officially part of the C++ standard after C++ 20).

We could have, as part of the source CMake file, a series of check_cxx_symbol_exists.

Unfortunately due to the trivial nature of std::identity, we cannot really demonstrate this methology in exemplar.

I honestly don't see the benefit of checking individual feature flag over a direct C++ standard version test. Good error message = why we need this C++ version to develop our library.

@wusatosi
Copy link
Member

@camio can we document that we are discourage using target_compile_features and setting CMAKE_CXX_STANDARD in CMake files somewhere. e.g. beman standard?

@ClausKlein
Copy link

ClausKlein commented Nov 26, 2024

IMHO: this will not be a problem, if it is used like in my PR.

Even if it set fix to cxx_std_17, as long the compiler support this std, the project ist compiled with this std.
And this minimum required std is exported to the cmake config package.

If not, the will be a clear cmake error message AFAIK.

bash-5.2$ cmake -S . -B build -G Ninja -D CMAKE_CXX_STANDARD=11 --fresh
-- The CXX compiler identification is Clang 19.1.3
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/local/opt/llvm/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
Examples to be built: fibonacci
-- Configuring done (0.9s)
-- Generating done (0.0s)
-- Build files have been written to: /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/build
bash-5.2$ ninja -C build -nv
ninja: Entering directory `build'
[1/8] "/usr/local/Cellar/llvm/19.1.3/bin/clang-scan-deps" -format=p1689 -- /usr/local/opt/llvm/bin/clang++  -I/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/include -std=gnu++20 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.1.sdk -mmacosx-version-min=14.7 -x c++ /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/tests/beman/inplace_vector/inplace_vector.test.cpp -c -o tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/inplace_vector.test.cpp.o -resource-dir "/usr/local/Cellar/llvm/19.1.3/lib/clang/19" -MT tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/inplace_vector.test.cpp.o.ddi -MD -MF tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/inplace_vector.test.cpp.o.ddi.d > tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/inplace_vector.test.cpp.o.ddi.tmp && mv tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/inplace_vector.test.cpp.o.ddi.tmp tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/inplace_vector.test.cpp.o.ddi
[2/8] "/usr/local/Cellar/llvm/19.1.3/bin/clang-scan-deps" -format=p1689 -- /usr/local/opt/llvm/bin/clang++  -I/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/include -std=gnu++20 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX15.1.sdk -mmacosx-version-min=14.7 -x c++ /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/examples/fibonacci.cpp -c -o examples/CMakeFiles/beman.inplace_vector.examples.fibonacci.dir/fibonacci.cpp.o -resource-dir "/usr/local/Cellar/llvm/19.1.3/lib/clang/19" -MT examples/CMakeFiles/beman.inplace_vector.examples.fibonacci.dir/fibonacci.cpp.o.ddi -MD -MF examples/CMakeFiles/beman.inplace_vector.examples.fibonacci.dir/fibonacci.cpp.o.ddi.d > examples/CMakeFiles/beman.inplace_vector.examples.fibonacci.dir/fibonacci.cpp.o.ddi.tmp && mv examples/CMakeFiles/beman.inplace_vector.examples.fibonacci.dir/fibonacci.cpp.o.ddi.tmp examples/CMakeFiles/beman.inplace_vector.examples.fibonacci.dir/fibonacci.cpp.o.ddi
[3/8] /usr/local/bin/cmake -E cmake_ninja_dyndep --tdi=tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/CXXDependInfo.json --lang=CXX --modmapfmt=clang --dd=tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/CXX.dd @tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/CXX.dd.rsp
ninja: build stopped: loading 'tests/beman/inplace_vector/CMakeFiles/beman.inplace_vector.test.dir/CXX.dd': No such file or directory.
bash-5.2$ 

@wusatosi
Copy link
Member

Even if it set fix to cxx_std_17, as long the compiler support this std, the project ist compiled with this std.
And this minimum required std is exported to the cmake config package.

I think you are mistaking @camio 's point. The ABI break happens at link time.

See his comment above and also here: bemanproject/optional26#80 (comment)

camio added a commit to camio/beman that referenced this issue Dec 11, 2024
The three rules added here are generally practiced in Beman repositories
as a means to mitigate ODR violation risks. While they have been discussed
at length in various PRs and issues (see
[this comment](bemanproject/exemplar#40 (comment))
for an example), this repeatedly comes up with new folks joining the
project.

By encoding this into rules linked to a PR pointing to the rationale
the hope is to gain consistency and avoid rehashing the discussions.
@maikel
Copy link

maikel commented Dec 18, 2024

Hi there,

I am new to the project and confused by the rule around target_compile_feature here bemanproject/beman#76

I try to understand where it comes from, since I don't agree with it.

Someone compiles beman.hip_hop using a version of gcc that supports C++17, but not C++20. beman.hip_hop uses target_compile_features(beman.hip_hop PUBLIC cxx_std_20) but, because this gcc version doesn't have a -std=c++20 flag, cmake inserts the -std=c++17 flag instead. Instead of failing at CMake time, there is a failure at build time because a C++20 feature is being used. The user isn't given a hint that the compiler itself is the problem.

That's not how I understand that it works. From docs: https://cmake.org/cmake/help/latest/command/target_compile_features.html

Specifies compiler features required when compiling a given target. If the feature is not listed in the CMAKE_C_COMPILE_FEATURES, CMAKE_CUDA_COMPILE_FEATURES, or CMAKE_CXX_COMPILE_FEATURES variables, then an error will be reported by CMake.

i.e. cmake will report an error if your compiler does not support C++20.

I also don't understand the first Situation that you brought up. Changing target_compile_feature changes the minimum required c++ standard version that is required for the respective project. If this changes over time, then yes: some downstreams will be broken.But it is intrinsically to changing the minimum required C++ standard version, because your library shouldn't be used with C++17 anymore.

I probably don't understand the conflict yet?

EDIT:
I am not aware whether cmake can impose a maximum C++ standard version in some way. How I see it, you are worried that some downstream client has that as a requirement that we silently break if we raise our minimum requirement to C++20 instead of C++17. So the actual use-case is about silently adding -std=c++20 where -std=c++17 would be enough, correct?

EDIT2: relevant link https://discourse.cmake.org/t/how-to-limit-maximum-cxx-standard-to-prevent-accidental-c-17-or-above-code/9862

@maikel
Copy link

maikel commented Dec 18, 2024

I understood now the following issue that motivates to ban the usage of target_compile_features to set the minimum required c++ standard:

Consider the following CMakeLists

cmake_minimum_required(VERSION 3.28)
project(testCMake CXX)

add_library(foo INTERFACE)
target_compile_features(foo INTERFACE cxx_std_20)

add_library(bar bar.cpp)
set_target_properties(bar 
  PROPERTIES 
    CXX_STANDARD 17
    CXX_STANDARD_REQUIRED ON
    CXX_EXTENSIONS OFF)
target_link_libraries(bar PRIVATE foo)

The library foo is populated with the interface compile feature cxx_std_20. CMake provides no good (native) way to require an exact C++ standard version that shall be used to build target bar. Linking bar with foo will resolve here to add -std=c++20 because CMake takes the maximum version that is required by all.

I think, I understand the use-case now. But it is still confusing to ban the usage of target_compile_features because in my understanding it is the common way to express the minimum required version, for libraries as well as for executables. But I agree that CMake is missing here a way to formulate the constraint from the other way around.

Today I learned something

@bretbrownjr
Copy link
Contributor

It's subtle, yes. The only correct place to specify ABI important flags [1] is wherever the final link of the application is configured [2]. By definition, Beman libraries are not applications, so they would ideally be configured (ergonomically!) by the user or tool running the cmake program and not by the CMakeLists.txt as such.

That being said, we could see per project settings mentioning the C++ standard requred. That would be in order to gracefully and helpfully fail to build if some impossible standard configuration is selected. We should attempt to support that use case if it's at all reasonable to do so. Unless you're regularly porting libraries to new environments, you will not know what sorts of errors are indicative of this sort of configuration mismatch. It's not reasonable in my opinion to expect that level of experience from a typical C++ engineer.

So I do expect each Beman project to mention C++ standards, but I don't expect them to self-configure which standard it builds with. That is a subtle distinction, I think, but if we provide sufficient regression test coverage and consistent workflows across the repos, I don't expect it will be too confusing what sorts of things should just work and what sorts of things are just broken.

[1] The C++ standard selection is not ABI stable in the general case. C++ compilers and standard libraries are exceptionally supporting specific forms of ABI stability across C++ versions, but in general that is not the case. Especially when standard features are backported to previous standard versions and compilation toolchains, ABI stability should not be expected. It's intentionally in scope for Beman libraries to be able to provide those kinds of backports.

[2] Sometimes libraries are built "on their own", such as for releases of prebuilt binaries. However, in those cases, it's up to the application and the dependency management systems involved to ensure ABI compatibility across the build systems for all linked binaries. That selection process is still driven by the needs of the applications being built.

camio added a commit to bemanproject/beman that referenced this issue Jan 10, 2025
* Add rules that mitigate ODR violations

The three rules added here are generally practiced in Beman repositories
as a means to mitigate ODR violation risks. While they have been discussed
at length in various PRs and issues (see
[this comment](bemanproject/exemplar#40 (comment))
for an example), this repeatedly comes up with new folks joining the
project.

By encoding this into rules linked to a PR pointing to the rationale
the hope is to gain consistency and avoid rehashing the discussions.

* Update docs/BEMAN_STANDARD.md

Co-authored-by: Darius Neațu <[email protected]>

* Fix rendering issue

---------

Co-authored-by: Darius Neațu <[email protected]>
@wusatosi
Copy link
Member

Just to make sure I understand this before any contribution, the approach for detecting functionality is standardized in: bemanproject/beman#76 , basically using check_cxx_source_compiles in combination of detecting feature flag __cplusplus? @camio @bretbrownjr

@camio
Copy link
Member Author

camio commented Jan 17, 2025

@wusatosi, yes exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants