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

CATCH_CONFIG_MAIN isn't __cdecl, causing x86 build failures #2486

Closed
davidmatson opened this issue Jul 25, 2022 · 6 comments
Closed

CATCH_CONFIG_MAIN isn't __cdecl, causing x86 build failures #2486

davidmatson opened this issue Jul 25, 2022 · 6 comments
Labels
Building and Packaging Issues affecting build/packaging scripts and utilities

Comments

@davidmatson
Copy link

Describe the bug
See title

Expected behavior
#define CATCH_CONFIG_MAIN works on x86.

Reproduction steps
Building on x86:

#define CATCH_CONFIG_MAIN
#include <catch2/catch.hpp>

TEST_CASE("passes")
{
    constexpr int x{ 1 };
    constexpr int y{ 1 };
    REQUIRE(x == 1);
}

Current workaround:
```c++
#define CATCH_CONFIG_RUNNER
#include <catch2/catch.hpp>

// Using #define CATCH_CONFIG_MAIN would be nicer, but it fails to specify __cdecl, causing a build failure on x86.
extern "C" int __cdecl wmain(int argc, wchar_t* argv[])
{
    return Catch::Session{}.run(argc, argv);
}

TEST_CASE("passes")
{
    constexpr int x{ 1 };
    constexpr int y{ 1 };
    REQUIRE(x == 1);
}

Platform information:

  • OS: Windows NT
  • Compiler+version: MSVC v19.30.30795.3
  • Catch version: v2.13.8
davidmatson added a commit to davidmatson/Catch2 that referenced this issue Jul 25, 2022
@dimztimz
Copy link
Contributor

Here https://docs.microsoft.com/en-us/cpp/c-language/using-wmain?view=msvc-170 there is nothing that says one needs to specify cdecl. It is already the default on x86 (32 bit), and it does not exist on x64 or arm at all.

Can you post full reproduction with manual compiler invocation will all CLI parametars or complete cmake script.

@davidmatson
Copy link
Author

Here's the warning it gives (docs are for main, but it handles wmain the same):
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4007?view=msvc-170

I think the default calling convention on x86 is __cdecl, so most times it wouldn't be noticed, but the default calling convention can be changed with compiler args. I'll work on figuring on a repro cl exe command line.

@davidmatson
Copy link
Author

Yep, it's /Gz (or probably other /G<> options) with warning level 4.
https://docs.microsoft.com/en-us/cpp/build/reference/gd-gr-gv-gz-calling-convention?view=msvc-170

cl /I ./include /DUNICODE /D_UNICODE /EHsc /W4 /WX /Gz /Wv:19.23 /std:c++17 program.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.32.31332 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

program.cpp
./include\catch2/catch.hpp(17527): error C2220: the following warning is treated as an error
./include\catch2/catch.hpp(17527): warning C4007: 'wmain': must be '__cdecl'

The same problem actually applies to main as well (when running without /DUNICODE /D_UNICODE):

cl /I ./include /EHsc /W4 /WX /Gz /Wv:19.23 /std:c++17 program.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.32.31332 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

program.cpp
./include\catch2/catch.hpp(17530): error C2220: the following warning is treated as an error
./include\catch2/catch.hpp(17530): warning C4007: 'main': must be '__cdecl'

@dimztimz
Copy link
Contributor

dimztimz commented Jul 26, 2022

Well, why are you changing the default calling convention in the first place? Those flags should be avoided and can be used only when you have full control of the whole source code including all the libraries. If you use third-party libraries, especially via package manager, you should not use those flags. If you use them you should be aware there are consequences.

For example, you have library compiled with ABI 1, but you consume it via its headers and on the consumption side you specify ABI 2. Kaboom.

@davidmatson
Copy link
Author

It's a publicly supported flag. In this case, we're building Windows itself.

horenmar added a commit that referenced this issue Aug 6, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fixes #2486

Co-authored-by: Martin Hořeňovský <martin.horenovsky@gmail.com>
@horenmar horenmar added the Building and Packaging Issues affecting build/packaging scripts and utilities label Aug 6, 2022
@horenmar
Copy link
Member

horenmar commented Aug 6, 2022

Autoclose won't catch this as it didn't go to the default branch.

@horenmar horenmar closed this as completed Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building and Packaging Issues affecting build/packaging scripts and utilities
Projects
None yet
Development

No branches or pull requests

3 participants