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

Strengthen exception specification for numeric operations #3887

Merged
merged 7 commits into from
Jul 26, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

Adding unconditional noexcept to suitable functions in <cmath>, <random>, and <valarray>.
(Conditional noexcept may be suitable for many functions in <random>...)

Currently only test for <valarray> is added. I hope we can add the test coverage to a new test suit for MS-specific properties and extensions in the future (see #502).

A bug of MSVC (DevCom-10416247) is discovered during testing.

Adding unconditional `noexcept` to suitable functions in `<cmath>`,
`<random>`, and `<valarray>`.

Currently only test for `<valarray>` is added.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner July 18, 2023 17:26
stl/inc/valarray Outdated Show resolved Hide resolved
stl/inc/random Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jul 18, 2023
@AlexGuteniev
Copy link
Contributor

I'm questioning noexcept on floats. There may be a scenario where signaling floats are used, and SEH are turned into C++ exceptiins.

@CaseyCarter
Copy link
Member

I'm questioning noexcept on floats. There may be a scenario where signaling floats are used, and SEH are turned into C++ exceptiins.

If we can't detect this statically, I doubt it's a supportable configuration.

@StephanTLavavej StephanTLavavej self-assigned this Jul 19, 2023
@AlexGuteniev
Copy link
Contributor

If we can't detect this statically, I doubt it's a supportable configuration.

I don't agree. I think not putting excaption specification is enough to support that config

@frederick-vs-ja
Copy link
Contributor Author

If we can't detect this statically, I doubt it's a supportable configuration.

I don't agree. I think not putting excaption specification is enough to support that config

The configuration sounds like a non-conforming extension, and is heavily broken due to MSVC's default /EHsc option.

@statementreply
Copy link
Contributor

I'm questioning noexcept on floats. There may be a scenario where signaling floats are used, and SEH are turned into C++ exceptiins.

Tested on acb8d34:

D:\Temp>type fpexcept.cpp
#include <cmath>
#include <cstdio>
#include <float.h>
#include <format>
#include <optional>
using namespace std;

#pragma fenv_access(on)

int main() {
    optional<double> result{};
    puts(format("noexcept(result = hypot(1.0, 2.0, 3.0)) == {:s}", noexcept(result = hypot(1.0, 2.0, 3.0))).c_str());

    _controlfp(0, _MCW_EM);
    try {
        result = hypot(1.0, 2.0, 3.0);
    } catch (...) {
        puts("exception caught");
    }
    _controlfp(_CW_DEFAULT, _MCW_EM);

    if (result) {
        puts(format("result = {}", *result).c_str());
    } else {
        puts("result = (nullopt)");
    }

    return 0;
}
D:\Temp>cl /W4 /EHa /std:c++20 /O2 fpexcept.cpp
用于 x64 的 Microsoft (R) C/C++ 优化编译器 19.37.32820 版
版权所有(C) Microsoft Corporation。保留所有权利。

fpexcept.cpp
fpexcept.cpp(14): warning C4996: '_controlfp': This function or variable may be unsafe. Consider using _controlfp_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
fpexcept.cpp(20): warning C4996: '_controlfp': This function or variable may be unsafe. Consider using _controlfp_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
Microsoft (R) Incremental Linker Version 14.37.32820.0
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:fpexcept.exe
fpexcept.obj

D:\Temp>.\fpexcept.exe
noexcept(result = hypot(1.0, 2.0, 3.0)) == true
exception caught
result = (nullopt)

@AlexGuteniev
Copy link
Contributor

Tested on acb8d34:

Not exactly the case I had in mind. I meant _set_se_translator, and catch(std::exception&), and without /O2

stl/inc/random Show resolved Hide resolved
stl/inc/random Outdated Show resolved Hide resolved
stl/inc/random Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Jul 22, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jul 25, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit db80afc into microsoft:main Jul 26, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

🦾 ⚠️ 🧮

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants