bpo-9566: Fix compiler warnings in peephole.c#10652
Merged
vstinner merged 6 commits intopython:masterfrom Dec 7, 2018
vstinner:peephole_cast
Merged
bpo-9566: Fix compiler warnings in peephole.c#10652vstinner merged 6 commits intopython:masterfrom vstinner:peephole_cast
vstinner merged 6 commits intopython:masterfrom
vstinner:peephole_cast
Conversation
Member
Author
|
@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
Member
There was a problem hiding this comment.
Is not it always false?
Member
Author
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.
Member
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
Member
There was a problem hiding this comment.
Is not it always false?
Member
Author
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.
Member
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
Member
There was a problem hiding this comment.
nexti here should be a small integer in range 1-4.
Member
Author
There was a problem hiding this comment.
Do you suggest an assertion instead of a runtime check?
Member
Author
There was a problem hiding this comment.
I modified my PR to use an assertion.
Python/peephole.c
Outdated
Member
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.
Member
Author
There was a problem hiding this comment.
No, I didn't. Right now, I have no 32-bit platform.
Member
There was a problem hiding this comment.
Well, I'll try to test it on 32-bit Linux and Windows.
Member
There was a problem hiding this comment.
There was no warning on 32-bit Windows in the Azure Pipelines build.
Python/peephole.c
Outdated
Member
There was a problem hiding this comment.
Well, I'll try to test it on 32-bit Linux and Windows.
Python/peephole.c
Outdated
Member
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
Member
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
Python/peephole.c
Outdated
Member
|
Line 4952 in cdbcb77 INT_MAX is the upper bound of code size compiler emits. |
vstinner
requested review from
a team,
1st1,
asvetlov,
gpshead,
rhettinger and
skrah
as code owners
Ensure that opcode arguments are smaller than UINT_MAX.
Member
Author
|
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? |
Member
|
Please wait with merging this PR. It conflicts with #5077 which may make this PR unneeded. |
Member
Author
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. |
Member
Author
|
@jkloth: Would you mind to rewrite this PR? |
Member
Author
|
@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! |
Member
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
There are just random comments. You can ignore them.
| assert((size_t)arg <= UINT_MAX / sizeof(_Py_CODEUNIT)); | ||
| arg *= sizeof(_Py_CODEUNIT); | ||
|
|
||
| /* The second jump is not taken if the first is (so |
Member
There was a problem hiding this comment.
I would keep the comment at the beginning of the block.
Python/peephole.c
Outdated
| stack effect */ | ||
| h = set_arg(codestr, i, get_arg(codestr, tgt)); | ||
| } else { | ||
| Py_ssize_t arg = (tgt + 1); |
Member
There was a problem hiding this comment.
Maybe make arg and tgt an unsigned int?
Member
Author
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.
| 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); |
Member
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); |
Member
Author
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.
| /* Fixup lnotab */ | ||
| for (i = 0, nops = 0; i < codelen; i++) { | ||
| assert(i - nops <= INT_MAX); | ||
| size_t block = (size_t)i - nops; |
Member
There was a problem hiding this comment.
Maybe make i an unsigned int?
Member
Author
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; | |||
Member
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.
Member
Author
There was a problem hiding this comment.
done: I added "Py_ssize_t ilen".
Member
Author
|
@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
Member
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
It seems to me that the only effect of this change is to silence compiler warnings. It does not fix any error. I do not think that it will introduce new errors.
serhiy-storchaka
approved these changes
Dec 7, 2018
Member
Author
|
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)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.Suggestion cannot be applied right now. Please check back later.
Ensure that opcode arguments are smaller than UINT_MAX.
https://bugs.python.org/issue9566