Repository navigation
Enable -fstrict-overflow #96821
Description
Activity
- added a commit that references this issue
on Sep 14, 2022 This is definitely worth exploring!
Reacted by Matthias Görgensbut 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. AaslandAlso, do clang and msvc have something similar?
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-overflowor that it can't?Reacted by Erlend E. AaslandI 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.
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
configurecheck for the flags directly, instead of checking for the name and version of the compiler.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.))
Another thing: whatever policy we decide on, we should probably fix that
--with-pydebugseemingly arbitrarily gives you a different policy.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-returnand-fpcc-struct-return.Other options such as
-ffp-contract,-fno-strict-overflow,-fwrapv,-fno-trapvor-fno-strict-aliasingare passed through to the link stage and merged conservatively for conflicting translation units. Specifically-fno-strict-overflow,-fwrapvand-fno-trapvtake precedence; and for example-ffp-contract=offtakes precedence over-ffp-contract=fast. You can override them at link time.If I understand that right, specifying
-fwrapvor-fno-strict-overflowfor 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.
Reacted by Eric SnowI extended #96823 to incorporate what I wrote above.
3 remaining items
- added 3 commits that reference this issue
on Sep 19, 2022 @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.
I ran pyperformance benchmarks on a branch that had all the patches applied and
-fstrict-overflowenabled. 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- added a commit that references this issue
on Oct 10, 2022 - added a commit that references this issue
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-overflowwas given, as suggested by ErlendReacted by Erlend E. Aasland and Matthias Görgens- added 2 commits that reference this issue
on Mar 6, 2023
At the moment we compile releases with
-fwrapvwhich makes the code a bit safer, but disables certain optimizations. From the GCC docs:My experiments with running sanitisers seem to suggest that we are nearly already ready for
-fno-wrapv(or-fstrict-overflowin 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
-fwrapvfor 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
CFLAGSfor 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-overflowsafe.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:
_struct: gh-96735: Fix undefined behaviour in struct unpacking functions #96739audioop: gh-96821: Fix undefined behaviour inaudioop.c#96923_ctypes: gh-96821: Fix undefined behaviour in_ctypes/cfield.c#96925Linked PRs
--with-strict-overflow#96823-fstrict-overflowonly if--with-strict-overflowis passed #139595