classification
Title: Fix warnings for Python/ast_unparse.c
Type: compile error Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, lukasz.langa, matrixise, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2018-01-29 13:35 by matrixise, last changed 2018-02-01 17:41 by christian.heimes. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5426 merged matrixise, 2018-01-29 13:38
PR 5475 merged miss-islington, 2018-02-01 17:00
Messages (14)
msg311131 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-01-29 13:35
Python/ast_unparse.c: At top level:
Python/ast_unparse.c:1122:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 maybe_init_static_strings()
 ^~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c: In function ‘append_ast_binop’:
Python/ast_unparse.c:105:15: warning: ‘op’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (-1 == append_charp(writer, op)) {
               ^~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c: In function ‘append_ast_unaryop’:
Python/ast_unparse.c:132:15: warning: ‘op’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (-1 == append_charp(writer, op)) {
               ^~~~~~~~~~~~~~~~~~~~~~~~
msg311132 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-01-29 13:38
I'm getting slightly different warnings with GCC 7.2.1:

Python/ast_unparse.c: In function ‘append_formattedvalue’:
Python/ast_unparse.c:859:41: warning: ordered comparison of pointer with integer zero [-Wextra]
     if (e->v.FormattedValue.format_spec > 0) {
                                         ^
Python/ast_unparse.c: At top level:
Python/ast_unparse.c:1122:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 maybe_init_static_strings()
 ^~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c: In function ‘append_ast_expr’:
Python/ast_unparse.c:23:16: warning: ‘op’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return _PyUnicodeWriter_WriteASCIIString(writer, charp, -1);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c:119:17: note: ‘op’ was declared here
     const char *op;
                 ^~
Python/ast_unparse.c:23:16: warning: ‘op’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return _PyUnicodeWriter_WriteASCIIString(writer, charp, -1);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c:79:17: note: ‘op’ was declared here
     const char *op;
msg311133 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-01-29 13:40
Hi Christian, I proposed a patch ofr the maybe_init_static_strings and for the const char *op, but I don't know how to fix for the e->v.FormattedValue.format_spec > 0, I don't know how to interpret it :/

But for _PyUnicodeWriter_WriteASCIIString, I don't get that warning.
msg311136 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-01-29 14:01
Hi @Chris, with your help I just fixed the last warning, but I don't have the warning with _PyUnicodeWriter_WriteASCIIString :/
msg311137 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-01-29 14:05
the switch blocks in both functions need to handle invalid input:

default: PyExc_SetString...; return -1;
msg311410 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-02-01 09:33
Hi Christian,

I add Lukasz in the loop because Victor asked to him a small review because he is not sure about the right way, PyErr_XXX or Py_UNREACHABLE.

Thank you for your review.
msg311418 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-01 10:34
I think it is better to not add the default case in switch statements. If once we will add a new binary or unary operator, but forgot to update ast_unparse.c the compiler will immediately warn about this if there are no default cases. But if there is a default case the compiler will be silent.
msg311424 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-02-01 12:53
Hi Serhiy,

but currently, there is some warnings because op has no value and can not be assigned to NULL because _PyUnicodeWriter_WriteASCIIString can't accept a NULL value.

What do you propose? Warnings at the compilation step and an eventual crash if we don't handle this case because there is no default?
msg311425 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-01 13:10
In my experience, Serhiy's example won't work. C doesn't guarantee that the functions will not be called with an unsupported op. Either my proposal or Victor's proposal are the correct way to solve the warning. Victor's proposal is even better because it makes the interpreter fail when an invalid operator is used.
msg311427 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-02-01 13:44
then I will use the approach of Victor with the Py_UNREACHABLE macro and will push asap.
msg311428 - (view) Author: Stéphane Wirtel (matrixise) * (Python committer) Date: 2018-02-01 13:46
Christian, I just pushed my code, wait for the feedback from Travis.

Thanks
msg311451 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-01 16:59
New changeset 83ab995871ffd504ac229bdbf5b9e9ffc1032815 by Christian Heimes (Stéphane Wirtel) in branch 'master':
bpo-32711: Fix warnings for Python/ast_unparse.c (#5426)
https://github.com/python/cpython/commit/83ab995871ffd504ac229bdbf5b9e9ffc1032815
msg311456 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-01 17:41
New changeset 78758f29b13aba6136f4c0a15d4457fbf92c5eef by Christian Heimes (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-32711: Fix warnings for Python/ast_unparse.c (GH-5426) (#5475)
https://github.com/python/cpython/commit/78758f29b13aba6136f4c0a15d4457fbf92c5eef
msg311457 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-01 17:41
Thanks!
History
Date User Action Args
2018-02-01 17:41:54christian.heimessetstatus: open -> closed
versions: + Python 3.8
type: compile error
messages: + msg311457

resolution: fixed
stage: patch review -> resolved
2018-02-01 17:41:26christian.heimessetmessages: + msg311456
2018-02-01 17:00:44miss-islingtonsetpull_requests: + pull_request5304
2018-02-01 16:59:29christian.heimessetmessages: + msg311451
2018-02-01 13:46:42matrixisesetmessages: + msg311428
2018-02-01 13:44:44matrixisesetmessages: + msg311427
2018-02-01 13:10:01christian.heimessetmessages: + msg311425
2018-02-01 12:53:08matrixisesetmessages: + msg311424
2018-02-01 10:34:39serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg311418
2018-02-01 09:33:06matrixisesetnosy: + lukasz.langa
messages: + msg311410
2018-01-29 14:05:50christian.heimessetmessages: + msg311137
2018-01-29 14:01:15matrixisesetmessages: + msg311136
2018-01-29 13:40:04matrixisesetmessages: + msg311133
2018-01-29 13:38:07matrixisesetkeywords: + patch
stage: patch review
pull_requests: + pull_request5261
2018-01-29 13:38:05christian.heimessetnosy: + christian.heimes
messages: + msg311132
2018-01-29 13:35:09matrixisecreate