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

Desktop build for Windows doesn't actually disable exceptions #1117

Closed
horenmar opened this issue Jan 14, 2024 · 9 comments
Closed

Desktop build for Windows doesn't actually disable exceptions #1117

horenmar opened this issue Jan 14, 2024 · 9 comments
Assignees
Labels
invalid Invalid issue or user error

Comments

@horenmar
Copy link

The CMake code that is supposed to disable exceptions for MSVC build instead enables fully conforming exception handling. The correct approach is to justnot have any /EH flags at all, but if you want to be explicit about it you can add /EHs- instead.

# Disable exceptions
string(REGEX REPLACE "/EH[a-z]+" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /EHsc")

@InfoTeddy InfoTeddy self-assigned this Jan 14, 2024
@InfoTeddy InfoTeddy added the bug label Jan 14, 2024
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 14, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
@InfoTeddy
Copy link
Collaborator

According to the MSVC docs, there are other /EH flags, /EHa and /EHr (respectively, enabling asynchronous exception handling and enabling generation of runtime termination checks). But it also seems like, according to this StackOverflow question at least, exceptions will be enabled if you have any try-catch in your code (including even if you don't use any in your code but you #include an STL header file which most likely does). So I'm guessing we do need the flags to disable exception handling after all, and we need to have /EHa- /EHs- /EHr-.

Either way, we need to properly make sure that the generated binaries actually are devoid of exception handling. I think I missed that step last time, but I don't remember.

InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 17, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 19, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 20, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 21, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 22, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 22, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
InfoTeddy added a commit to InfoTeddy/VVVVVV that referenced this issue Jan 22, 2024
Apparently the actual way to do this is to use '-', not 'c'.

Fixes TerryCavanagh#1117.
@InfoTeddy
Copy link
Collaborator

So I found this post covering exceptions on MSVC.

If I compile the example snippet in the post with cl ex.cpp, I get the same warning: warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc. If I compile it with cl /EHsc ex.cpp, the warning goes away.

If I try it with cl /EHs- ex.cpp, I get the warning again. So this seems to me to indicate that you can't actually disable exceptions entirely by using /EHs-. And lastly, trying cl /EHs- /EHa- /EHr- ex.cpp results in...

cl : Command line warning D9007 : '/Ehr' requires '/Eha, /EHs or /GX'; option ignored
cl : Command line warning D9002 : ignoring unknown option '/EH-'

...and still produces warning C4530.

So no, removing /EHsc or adding /EHs- doesn't work.

@InfoTeddy InfoTeddy added invalid Invalid issue or user error and removed bug labels Jan 22, 2024
@horenmar
Copy link
Author

I mean

// ehsc.test.cpp
#include <cstdlib>
#include <vector>

void sink(std::vector<int> const& s);

int main(int argc, char** argv) {
    std::vector<int> vec; vec.reserve(argc);
    for (int i = 0; i < argc; ++i) {
        vec.push_back(atoi(argv[argc]));
    }
    sink(vec);
}
> cl /c ehsc.test.cpp /Fo"ehsc.enabled.o" /EHsc
> cl /c ehsc.test.cpp /Fo"ehsc.disabled.o" /EHs-c-
> cl /c ehsc.test.cpp /Fo"ehsc.disabled.full.o" /EHs-c- /D_HAS_EXCEPTIONS=0
> dir
24.02.2024  21:18    <DIR>          .
24.02.2024  21:18    <DIR>          ..
24.02.2024  21:20            27 429 ehsc.disabled.full.o
24.02.2024  21:20            29 438 ehsc.disabled.o
24.02.2024  21:20            27 854 ehsc.enabled.o
24.02.2024  21:16               269 ehsc.test.cpp

@InfoTeddy
Copy link
Collaborator

I mean

So we should have been using /D_HAS_EXCEPTIONS=0 all this time?

@horenmar
Copy link
Author

  • /EHs- (or /EHs-c- if you want to be more explicit) tells the compiler to not bother generating exception related data/code, e.g. info for stack unwinding.
  • /D_HAS_EXCEPTIONS=0 tells the stdlib to compile without exceptions (disables try, catch, etc).
  • Without exceptions it is also preferable to link to the static version of the runtime libs. (/MT, /MTd)

@InfoTeddy
Copy link
Collaborator

  • /EHs- (or /EHs-c- if you want to be more explicit) tells the compiler to not bother generating exception related data/code, e.g. info for stack unwinding.

As I established above, /EHs- doesn't actually do anything unless there was a /EHs before (and we already remove all such flags first just to be sure).

  • /D_HAS_EXCEPTIONS=0 tells the stdlib to compile without exceptions (disables try, catch, etc).

I figured. I'll look into this more. This might be what we were looking for in the first place.

  • Without exceptions it is also preferable to link to the static version of the runtime libs. (/MT, /MTd)

We already do this.

# Statically link Microsoft's runtime library so end users don't have to install it (/MT)
# Also, build with multiple processors (/MP)
list(APPEND GLOBAL_COMPILE_FLAGS /MT /MP)

@horenmar
Copy link
Author

(and we already remove all such flags first just to be sure).

No, you do not, that's what spawned this issue.

@InfoTeddy
Copy link
Collaborator

(and we already remove all such flags first just to be sure).

No, you do not, that's what spawned this issue.

Yes, we do. The string(...) removes all flags. The set(...) does add /EHsc, and yes, that still results in some exception support, but replacing /EHsc with /EHs- or removing the set(...) would not disable exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Invalid issue or user error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants