GeezerVison is showing github.com — Enable `-fstrict-overflow` · Issue #96821 · python/cpython
Enable -fstrict-overflow #96821DescriptionAt the moment we compile releases with -fwrapv which makes the code a bit safer, but disables certain optimizations. From the GCC docs:
This option instructs the compiler to assume that signed arithmetic overflow of addition, subtraction and multiplication wraps around using twos-complement representation. This flag enables some optimizations and disables others.
My experiments with running sanitisers seem to suggest that we are nearly already ready for -fno-wrapv (or -fstrict-overflow in general). Doing so could lead to quite a few speedups, but we would need to be more careful with the code we write.
It might be worthwhile to get a few benchmarks.
(To be extra precise, we give -fwrapv for clang and gcc for any build that doesn't get --with-pydebug.)
Pitch
My plan right now is to adapt the build system so that only the modules that need it are build with -fwrapv, and the rest can be build with -fstrict-overflow.
We already have config machinery that can add specific CFLAGS for specific modules only.
Perhaps the whole thing can be gated behind a configure flag, like --with-strict-overflow.
If everything goes well, and this improves performance we can consider adding this functionality to one of the standard optimization options.
We can also work on making more modules -fstrict-overflow safe.
Previous discussion
@markshannon @ericsnowcurrently
Brought up on faster-cpython/ideas#458 and inspired by #96678
Some previous issues around -fwrapv:
I'm sure there are more.
Progress so far
As far as is currently known, the three remaining modules that rely on defined integer overflow are fixed by:
Linked PRs
Activityericsnowcurrently commented on Sep 15, 2022 This is definitely worth exploring! Reacted by Matthias Görgens ericsnowcurrently commented on Sep 16, 2022
but we would need to be more careful with the code we write.
What do you mean by that?
More concretely:
- what would need to be done (or not done) specifically?
- would this mean adding to PEP 7?
- what would that text say?
- would we add some explanation to the devguide?
Reacted by Erlend E. Aasland ericsnowcurrently commented on Sep 16, 2022 Also, do clang and msvc have something similar? ericsnowcurrently commented on Sep 16, 2022 In gh-96823 I asked about detecting compatibility for the compile flag. This is probably the better place to discuss that.
Basically, how can we detect that a C file can be built with -fstrict-overflow or that it can't? Reacted by Erlend E. Aasland matthiasgoergens commented on Sep 16, 2022 ContributorAuthor More actions I ran clang's undefined behaviour sanitiser and address sanitiser over the test suite to detect the modules that need defined overflow.
Of course, strictly speaking a detection this way can only show the presence of the need for defined overflow, but it cannot show the absence of such a need.
But that's no different from any other bug or undefined behaviour, and we just rely on the additional assumption that our test suite has enough coverage.
Small complicating detail: when you use the conpiler flag to make signed integer overflow into defined behaviour, the sanitisers no longer complain about it.
So if you want to check if the flag is still necessary for a module, you need to temporarily turn it off. matthiasgoergens commented on Sep 16, 2022 ContributorAuthor More actions Clang definitely has this flag. We actually use clang for the sanitisers.
I don't know about msvc. There's no trace of anything like this in their docs https://docs.microsoft.com/en-us/cpp/build/reference/compiler-options-listed-by-category?view=msvc-170#code-generation
But we can just stick to the current situation here:
Only gcc and clang explicitly get flags about overflow, and for the other compilers we just hope and pray.
(The situation will actually improve for the other compilers, when we make all modules strict-overflow safe.)
For the actual implementation, I'll probably make configure check for the flags directly, instead of checking for the name and version of the compiler. matthiasgoergens commented on Sep 17, 2022 ContributorAuthor More actions I had a look at PEP 7. The relevant section is https://peps.python.org/pep-0007/#c-dialect
The PEP says to use C11 and doesn't mention any deviation from that for overflow. So our code should already avoid signed integer overflow. At this point that's more of an aspiration than reality, though.
So to some extent, we could describe the effort here as part of implementing PEP 7.
Strictly speaking we don't need to change anything, but I think pragmatically we should make a note that up to 3.11 signed integer overflow was tacitly tolerated, but that beginning from 3.12 we are sticking to the standard. I created a draft PR python/peps#2796 for how that could look like. (Please suggest better wording, if you have ideas.)
(Additionally, I also notice that when I am building I am getting a few warnings here or there, but the PEP suggests that we shouldn't be getting any warnings. Perhaps I'll spend a bit of time fixing the code (or suppressing these warnings in known instances that we can't or won't fix.)) matthiasgoergens commented on Sep 17, 2022 ContributorAuthor More actions Another thing: whatever policy we decide on, we should probably fix that --with-pydebug seemingly arbitrarily gives you a different policy. matthiasgoergens commented on Sep 17, 2022 ContributorAuthor More actions I just found some slightly bad news in the GCC docs:
Certain ABI-changing flags are required to match in all compilation units, and trying to override this at link time with a conflicting value is ignored. This includes options such as -freg-struct-return and -fpcc-struct-return.
Other options such as -ffp-contract, -fno-strict-overflow, -fwrapv, -fno-trapv or -fno-strict-aliasing are passed through to the link stage and merged conservatively for conflicting translation units. Specifically -fno-strict-overflow, -fwrapv and -fno-trapv take precedence; and for example -ffp-contract=off takes precedence over -ffp-contract=fast. You can override them at link time.
If I understand that right, specifying -fwrapv or -fno-strict-overflow for one C source file might essentially be enough to preclude optimizations for (almost?) everything.
So we might need to be completely clean before we can expect to get a performance benefit?
That's where we want to get to eventually anyway, but it would have been nice if a piecemeal approach worked.
Update: in experiments with clang and its undefined-behaviour sanitizer, the piecemeal approach seems to work. So at least for diagnostics it's good enough! I don't know about optimizations. matthiasgoergens commented on Sep 17, 2022 ContributorAuthor More actions I extended #96823 to incorporate what I wrote above. matthiasgoergens commented on Sep 25, 2022 ContributorAuthor More actions @mdickinson Could you please have a look at the other PRs mentioned in this issue?
There's only two left, and then we can turn on strict overflow for (hopefully) extra performance.
I went with implementation defined behaviour for those two PRs. But I'm happy to adopt your clever method for non-implementation defined behaviour, if you prefer that. matthiasgoergens commented on Sep 26, 2022 ContributorAuthor More actions I ran pyperformance benchmarks on a branch that had all the patches applied and -fstrict-overflow enabled. Overall, we are getting about 1% speedup on GCC:
| Benchmark |
2022-09-25_09-55-main-f5f047aa628c |
2022-09-25_10-17-strict_overflow_test-9e8007d7aae6 |
| unpickle |
12.2 us |
11.2 us: 1.08x faster |
| fannkuch |
346 ms |
325 ms: 1.06x faster |
| pyflate |
371 ms |
353 ms: 1.05x faster |
| scimark_sor |
95.3 ms |
92.3 ms: 1.03x faster |
| nqueens |
71.1 ms |
68.9 ms: 1.03x faster |
| float |
64.7 ms |
62.7 ms: 1.03x faster |
| chaos |
58.8 ms |
57.3 ms: 1.03x faster |
| logging_format |
6.47 us |
6.30 us: 1.03x faster |
| json_loads |
20.6 us |
20.2 us: 1.02x faster |
| nbody |
79.4 ms |
77.7 ms: 1.02x faster |
| sqlalchemy_declarative |
122 ms |
120 ms: 1.02x faster |
| spectral_norm |
86.2 ms |
84.4 ms: 1.02x faster |
| logging_silent |
83.3 ns |
81.8 ns: 1.02x faster |
| genshi_text |
19.1 ms |
18.8 ms: 1.02x faster |
| telco |
5.42 ms |
5.33 ms: 1.02x faster |
| go |
121 ms |
119 ms: 1.02x faster |
| sympy_expand |
429 ms |
422 ms: 1.02x faster |
| logging_simple |
5.73 us |
5.65 us: 1.02x faster |
| scimark_fft |
260 ms |
256 ms: 1.02x faster |
| django_template |
29.8 ms |
29.4 ms: 1.01x faster |
| xml_etree_generate |
73.1 ms |
72.2 ms: 1.01x faster |
| xml_etree_process |
50.2 ms |
49.5 ms: 1.01x faster |
| pathlib |
16.8 ms |
16.6 ms: 1.01x faster |
| json_dumps |
8.21 ms |
8.11 ms: 1.01x faster |
| pickle_pure_python |
253 us |
250 us: 1.01x faster |
| raytrace |
248 ms |
245 ms: 1.01x faster |
| xml_etree_iterparse |
87.9 ms |
86.9 ms: 1.01x faster |
| tornado_http |
105 ms |
104 ms: 1.01x faster |
| scimark_monte_carlo |
55.4 ms |
54.8 ms: 1.01x faster |
| unpickle_pure_python |
181 us |
179 us: 1.01x faster |
| meteor_contest |
90.3 ms |
89.5 ms: 1.01x faster |
| regex_v8 |
17.9 ms |
17.8 ms: 1.01x faster |
| dulwich_log |
67.8 ms |
67.3 ms: 1.01x faster |
| xml_etree_parse |
129 ms |
128 ms: 1.01x faster |
| sympy_integrate |
18.9 ms |
18.7 ms: 1.01x faster |
| mako |
8.31 ms |
8.27 ms: 1.01x faster |
| sympy_sum |
158 ms |
158 ms: 1.01x faster |
| pidigits |
166 ms |
165 ms: 1.01x faster |
| python_startup_no_site |
6.72 ms |
6.74 ms: 1.00x slower |
| python_startup |
8.70 ms |
8.75 ms: 1.00x slower |
| scimark_lu |
96.9 ms |
98.4 ms: 1.02x slower |
| regex_dna |
147 ms |
149 ms: 1.02x slower |
| crypto_pyaes |
63.0 ms |
64.4 ms: 1.02x slower |
| deltablue |
2.93 ms |
3.00 ms: 1.03x slower |
| regex_effbot |
2.43 ms |
2.51 ms: 1.04x slower |
| Geometric mean |
(ref) |
1.01x faster |
Benchmark hidden because not significant (16): scimark_sparse_mat_mult, regex_compile, sqlalchemy_imperative, html5lib, unpickle_list, pickle, chameleon, pickle_list, hexiom, genshi_xml, 2to3, richards, sympy_str, pickle_dict, unpack_sequence, sqlite_synth Reacted by Ken Jin, Shantanu and vient hauntsaninja commented on Mar 4, 2023 I've fixed and merged #96823 . A follow up improvement could be to check for the compiler switch only if --with-strict-overflow was given, as suggested by Erlend Reacted by Erlend E. Aasland and Matthias Görgens StanFromIreland commented on Oct 5, 2025
I've fixed and merged #96823 . A follow up improvement could be to check for the compiler switch only if --with-strict-overflow was given, as suggested by Erlend
I wrote #139595 for this refactor.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|