| Nov | DEC | Jan |
| 19 | ||
| 2021 | 2022 | 2023 |
COLLECTED BY
Collection: Save Page Now Outlinks
Sorry, something went wrong.
PEP 7: Mention signed overflow explicitly
f1d62c2
-fstrict-overflow
python/cpython#96821
Open
hugovk
reviewed
ae434c6
to
f1d62c2
Compare
" data-pjax="true" class="Link--secondary markdown-title" href="/web/20221219200051/https://github.com/python/peps/pull/2796/commits/283eb9c04be6dbfa9794200c1bb9cc401af0f17b">PEP 7: Fix backticks
283eb9c
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>gvanrossum reviewed
| * Python versions up to and including 3.11 tacitly allowed signed arithmetic | ||
| overflow (wrapping around using twos-complement representation) via the ``-fwrapv`` compiler option. |
-fwrapv? Or do you mean that those overflows are ignored because -fwrapv is being passed? I'm not sure that a compiler flag is really the same as a C dialect (we'd have to document a ton of dialects otherwise).
Sorry, something went wrong.
Author-fwrapv flag. Later on clang came along, and needed the same treatment.
So that's the latter of the two possibilities you mentioned. (Though not exactly, because the overflow isn't ignored.)
As far as I can tell the flag also make bit shifting of negative numbers and arithmetic on the null pointer valid operations, amongst other things.
My aspiration with the linked issue python/cpython#96821 is to relieve CPython's C code from the reliance on this assumption.
That way you can build CPython with a standards compliant compiler, even if it doesn't support that flag. As far as I can tell, msvc doesn't have this flag (we definitely don't pass it) and we just got lucky so far that msvc seems to be doing the right thing for us.
The other reason to get by without -fwrapv is to enable more optimisations in GCC and clang.
About which C flags to document: I would suggest to document any settings that shift the border of defined vs undefined behaviour. Ie any settings (or assumptions!) that change anything mention in the C standard, if that makes sense?
But not flags that don't have such an effect. Eg nothing about optimisation levels, or warnings or exactly what libraries we are linking etc.
I don't have strong opinions on where to put this information. A reviewer on the linked issue suggested considering a change to this PEP, so I made a draft PR for the section that looked the most appropriate to me.
Of course, whether we want to drop reliance on defined signed integer overflow or -fwrapv at all is another question. The text in this PR is written from the perspective of dropping those two things.
In practice, we would want to drop the assumption first and clean up the code. And once we are reasonably sure, we can drop the flag, too.
Sorry, something went wrong.
Member-fwrapv. (As you say MSVC doesn't support that option, at least not in that form, but who knows what we do use there. IIUC the compiler flags are hidden in XML files.)
Finally -- is python/cpython#96739 the only fix needed to get rid of the UB?
Sorry, something went wrong.
Author-fwrapv was just intended as a stopgap until the code could be fixed, and was never meant as a conscious decision to write in non-standard C. But that's just my interpretation.
People's prompt reactions when me and others pointed out instances of normally-undefined behaviour (that are only defined thanks to -fwrapv) seem to strengthen the case that this was never a conscious choice.
IIUC the compiler flags are hidden in XML files.
Interesting! Where are those XML files? None of the XML files in the main repository seem applicable. https://github.com/python/buildmaster-config/blob/8484356e8a0854084e186d9c9eb47b3b0c6eb628/master/custom/factories.py#L311-L315 has some config, but it's not in XML. All the other relevant config I could find seems to live in places like configure.ac and Makefile.pre.in and so on.
Finally -- is python/cpython#96739 the only fix needed to get rid of the UB?
I'm running some experiments to see what undefined behaviours I can round up.
There's eg this null-pointer arithmetic at https://github.com/python/cpython/blob/0f2b469ce1a6f123ad9e151b1771651b3e1d2de6/Modules/_testcapimodule.c#L4922 as demonstrated by the first commit in python/cpython#96915
There's definitely still some UB lurking in audioop.c:
0:00:01 load avg: 4.11 [ 8/437/1] test_sunau crashed (Exit code 1)
/home/matthias/prog/python/cpythons/strict_overflow/Modules/audioop.c:1561:43: runtime error: left shift of negative value -24
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/matthias/prog/python/cpythons/strict_overflow/Modules/audioop.c:1561:43 in
And there might be some more elsewhere.
I had also assumed that the "dialect" was more about specifying the version of the C standard used and which optional parts thereof. That was probably naive.
That's definitely the most important part of that section. Whether you want to use the word 'dialect' or some other term to describe variants like -fwrapvor-fno-delete-null-pointer-checks (to take an example from the Linux kernel) is just a matter of taste and definition.
Given that the term 'dialect' is already used for K&R C vs C89 vs C11 etc, it might be useful to settle on a different term for the impact these flags have?
Apropos the Linux kernel: they made the conscious choice to pick lots of compiler flags that make their effective dialect of C into a much more defined language.
Going down that route is definitely worth considering and discussing. Nobody is forcing us to put up with all the UB of the C standard. We can deliberately choose to write in a more defined variant of C, too. (As long as all the compilers we care about support the necessary flags.)
Sorry, something went wrong.
AuthorSorry, something went wrong.
Member|
You gotta find someone else to argue, sorry. |
Sorry, something went wrong.
Member|
I'm not seeing the benefit of this PR's change. All it's saying is CPython was compiled with a certain flag up to a certain point. But PEP 7 is about how CPython is compiled but how to write C code within the project. Without specific guidance, e.g. "all arithmetic operations that may overflow are expected to ...," I'm not seeing what use core developers are supposed to derive from this to guide their coding. |
Sorry, something went wrong.
brettcannon requested changesSorry, something went wrong.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment No one assigned Labels None yetSuccessfully merging this pull request may close these issues.
None yet
4 participants Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge.