bpo-9566: Fix compiler warnings in peephole.c #10652
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@serhiy-storchaka, @methane, @pablogsal: Since this code is critical and I'm not confident in my change, would you mind to review it? I see these warnings for 5 years at least on Windows :-)
I chose to not add a NEWS entry, since fixed bugs are very unlikely: you need a code object larger than 2 GiB... |
Python/peephole.c
Outdated
| @@ -353,11 +363,18 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
| stack effect */ | |||
| h = set_arg(codestr, i, get_arg(codestr, tgt)); | |||
| } else { | |||
| Py_ssize_t arg = (tgt + 1); | |||
| if ((size_t)arg > UINT_MAX / sizeof(_Py_CODEUNIT)) { | |||
There was a problem hiding this comment.
Is not it always false?
There was a problem hiding this comment.
I don't know, that's why I added a test :-) In case of doubt, I prefer to check.
There was a problem hiding this comment.
I prefer to not add senseless checks. The size of the code < 4 GiB, thus tgt < UINT_MAX / sizeof(_Py_CODEUNIT). The code of the peepholer is already complex enough. Such checks just distract an attention from working code and spend CPU cycles.
Python/peephole.c
Outdated
| } | ||
| j *= sizeof(_Py_CODEUNIT); | ||
| copy_op_arg(codestr, op_start, opcode, j, i + 1); | ||
| if ((size_t)arg > (UINT_MAX / sizeof(_Py_CODEUNIT))) { |
There was a problem hiding this comment.
Is not it always false?
There was a problem hiding this comment.
What if you create a code object of 4 GiB? :-) This pure sanity test is mostly there to avoid to have to check multiple times above.
There was a problem hiding this comment.
The check at the beginning of PyCode_Optimize() ensure that it is smaller than 4 GiB. It is enough. And I am nut sure that the compiler is able to create code objects of 4 GiB.
No need to repeat the check in tight loops if you performed it at the beginning.
Python/peephole.c
Outdated
| @@ -477,10 +502,11 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
| break; | |||
| } | |||
| nexti = i - op_start + 1; | |||
| if (instrsize(j) > nexti) | |||
| if (instrsize(j) > nexti || (size_t)nexti > UINT_MAX) { | |||
There was a problem hiding this comment.
nexti here should be a small integer in range 1-4.
There was a problem hiding this comment.
Do you suggest an assertion instead of a runtime check?
There was a problem hiding this comment.
I modified my PR to use an assertion.
Python/peephole.c
Outdated
| Py_ssize_t codesize = PyBytes_GET_SIZE(code); | ||
| assert(codesize % sizeof(_Py_CODEUNIT) == 0); | ||
| Py_ssize_t codelen = codesize / sizeof(_Py_CODEUNIT); | ||
| if ((size_t)codelen > UINT_MAX) { |
There was a problem hiding this comment.
Did you test this on 32-bit platforms? This condition is always false on 32-bit, and this can cause new compiler warnings.
There was a problem hiding this comment.
No, I didn't. Right now, I have no 32-bit platform.
There was a problem hiding this comment.
Well, I'll try to test it on 32-bit Linux and Windows.
There was a problem hiding this comment.
There was no warning on 32-bit Windows in the Azure Pipelines build.
Python/peephole.c
Outdated
| Py_ssize_t codesize = PyBytes_GET_SIZE(code); | ||
| assert(codesize % sizeof(_Py_CODEUNIT) == 0); | ||
| Py_ssize_t codelen = codesize / sizeof(_Py_CODEUNIT); | ||
| if ((size_t)codelen > UINT_MAX) { |
There was a problem hiding this comment.
Well, I'll try to test it on 32-bit Linux and Windows.
Python/peephole.c
Outdated
| @@ -353,11 +363,18 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
| stack effect */ | |||
| h = set_arg(codestr, i, get_arg(codestr, tgt)); | |||
| } else { | |||
| Py_ssize_t arg = (tgt + 1); | |||
| if ((size_t)arg > UINT_MAX / sizeof(_Py_CODEUNIT)) { | |||
There was a problem hiding this comment.
I prefer to not add senseless checks. The size of the code < 4 GiB, thus tgt < UINT_MAX / sizeof(_Py_CODEUNIT). The code of the peepholer is already complex enough. Such checks just distract an attention from working code and spend CPU cycles.
Python/peephole.c
Outdated
| } | ||
| j *= sizeof(_Py_CODEUNIT); | ||
| copy_op_arg(codestr, op_start, opcode, j, i + 1); | ||
| if ((size_t)arg > (UINT_MAX / sizeof(_Py_CODEUNIT))) { |
There was a problem hiding this comment.
The check at the beginning of PyCode_Optimize() ensure that it is smaller than 4 GiB. It is enough. And I am nut sure that the compiler is able to create code objects of 4 GiB.
No need to repeat the check in tight loops if you performed it at the beginning.
Python/peephole.c
Outdated
| /* If instrsize(j) < nexti, we'll emit EXTENDED_ARG 0 */ | ||
| write_op_arg(codestr + h, opcode, j, nexti); | ||
| assert((size_t)nexti <= UINT_MAX); |
Python/peephole.c
Outdated
| /* If instrsize(j) < nexti, we'll emit EXTENDED_ARG 0 */ | ||
| write_op_arg(codestr + h, opcode, j, nexti); | ||
| assert((size_t)nexti <= UINT_MAX); | ||
| write_op_arg(codestr + h, opcode, j, (unsigned int)nexti); |
|
Line 4952 in cdbcb77 INT_MAX is the upper bound of code size compiler emits. |
vstinner
requested review from
1st1,
asvetlov,
gpshead,
rhettinger,
skrah and
a team
as code owners
Ensure that opcode arguments are smaller than UINT_MAX.
|
I replaced almost all runtime checks with assertions with a comment explaining why the downcast cannot overflow. Sorry, I used "git merge master" and then "git rebase -i" to merge commits, and something bad happened: I pushed 84 commits instead of a single one. I rewrote my PR to squash everything into a single commit. @serhiy-storchaka, @methane: Would you mind to have new look on the new version of my change? |
|
Please wait with merging this PR. It conflicts with #5077 which may make this PR unneeded. |
It seems like your PR is controversial :-) It is true that your PR removes code that I modified, but my PR modifies code which isn't touched nor removed by your PR. |
|
@jkloth: Would you mind to rewrite this PR? |
|
@serhiy-storchaka: Would you be ok if i merge this PR? It should be easy to fix conflicts in your PR, since your PR removes code from peephole.c :-) This PR fix one of the last compiler warnings on Windows. With this PR + PR 11010, all warnings are gone on 64-bit Windows accoding to @jkloth: https://bugs.python.org/issue9566#msg331261 I would like to fix these warnigns since 5 years and we are almost there! |
Python/peephole.c
Outdated
| /* cannot overflow: codelen <= INT_MAX */ | ||
| assert((size_t)arg <= UINT_MAX / sizeof(_Py_CODEUNIT)); | ||
| arg *= sizeof(_Py_CODEUNIT); | ||
|
|
||
| /* The second jump is not taken if the first is (so |
There was a problem hiding this comment.
I would keep the comment at the beginning of the block.
Python/peephole.c
Outdated
| @@ -353,11 +365,16 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
| stack effect */ | |||
| h = set_arg(codestr, i, get_arg(codestr, tgt)); | |||
| } else { | |||
| Py_ssize_t arg = (tgt + 1); | |||
There was a problem hiding this comment.
Maybe make arg and tgt an unsigned int?
There was a problem hiding this comment.
tgt is initialized by "tgt = find_op(codestr, codelen, h);" and find_op() return type is Py_ssize_t. It seems simpler to me to use Py_ssize_t here as well.
| @@ -383,17 +400,20 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
| codestr[op_start] = PACKOPARG(RETURN_VALUE, 0); | |||
| fill_nops(codestr, op_start + 1, i + 1); | |||
| } else if (UNCONDITIONAL_JUMP(_Py_OPCODE(codestr[tgt]))) { | |||
| j = GETJUMPTGT(codestr, tgt); | |||
| size_t arg = GETJUMPTGT(codestr, tgt); | |||
There was a problem hiding this comment.
Maybe use unsigned int?
Suggested change
| size_t arg = GETJUMPTGT(codestr, tgt); | |
| unsigned int arg = GETJUMPTGT(codestr, (unsigned int)tgt); |
There was a problem hiding this comment.
My problem is that GETJUMPTGT() return type is unclear to me. It can return (tgt+1) and tgt type is Py_ssize_t.
| @@ -427,11 +447,14 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
|
|
|||
| /* Fixup lnotab */ | |||
| for (i = 0, nops = 0; i < codelen; i++) { | |||
| assert(i - nops <= INT_MAX); | |||
| size_t block = (size_t)i - nops; | |||
There was a problem hiding this comment.
Maybe make i an unsigned int?
There was a problem hiding this comment.
Maybe make i an unsigned int?
I hesitated to do that, but right now the scope of 'i' is the whole function and the function is very long. I tried to write the minimum change.
Python/peephole.c
Outdated
| @@ -477,10 +500,12 @@ PyCode_Optimize(PyObject *code, PyObject* consts, PyObject *names, | |||
| break; | |||
| } | |||
| nexti = i - op_start + 1; | |||
There was a problem hiding this comment.
It may be better to introduce a new variable instead of reusing nexti. In other place nexti was used as an index in cedestr. Here i - op_start + 1 is the size of the instruction.
There was a problem hiding this comment.
done: I added "Py_ssize_t ilen".
|
@serhiy-storchaka: I updated my PR and it seems like I replied to all your latest comment :-) |
serhiy-storchaka
approved these changes
Dec 7, 2018
serhiy-storchaka
approved these changes
Dec 7, 2018
|
Oh wow, I didn't expect that fixing these few compiler warnings would be so painful :-/ Thanks for the very helpful review @serhiy-storchaka! |
|
vstinner
added a commit
that referenced
this pull request
Apr 23, 2019
* bpo-9566: Fix compiler warnings in gcmodule.c (GH-11010) Change PyDTrace_GC_DONE() argument type from int to Py_ssize_t. (cherry picked from commit edad38e) * bpo-30465: Fix C downcast warning on Windows in ast.c (#6593) ast.c: fstring_fix_node_location() downcasts a pointer difference to a C int. Replace int with Py_ssize_t to fix the compiler warning. (cherry picked from commit fb7e799) * bpo-9566: Fix compiler warnings in peephole.c (GH-10652) (cherry picked from commit 028f0ef) * bpo-27645, sqlite: Fix integer overflow on sleep (#6594) Use the _PyTime_t type and round away from zero (ROUND_UP, _PyTime_ROUND_TIMEOUT) the sleep duration, when converting a Python object to seconds and then to milliseconds. Raise an OverflowError in case of overflow. Previously the (int)double conversion rounded towards zero (ROUND_DOWN). (cherry picked from commit ca40501)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Suggestion cannot be applied right now. Please check back later.
Ensure that opcode arguments are smaller than UINT_MAX.
https://bugs.python.org/issue9566