Issue41059
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2020-06-20 18:56 by gregory.p.smith, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Messages (4) | |||
---|---|---|---|
msg371956 - (view) | Author: Gregory P. Smith (gregory.p.smith) * | Date: 2020-06-20 18:56 | |
Here's an example: *** CID 1464688: Control flow issues (DEADCODE) /Parser/parser.c: 24243 in _tmp_147_rule() 24237 && 24238 (z = disjunction_rule(p)) // disjunction 24239 ) 24240 { 24241 D(fprintf(stderr, "%*c+ _tmp_147[%d-%d]: %s succeeded!\n", p->level, ' ', _mark, p->mark, "'if' disjunction")); 24242 _res = z; >>> CID 1464688: Control flow issues (DEADCODE) >>> Execution cannot reach the expression "PyErr_Occurred()" inside this statement: "if (_res == NULL && PyErr_O...". 24243 if (_res == NULL && PyErr_Occurred()) { 24244 p->error_indicator = 1; 24245 D(p->level--); 24246 return NULL; 24247 } 24248 goto done; A lot of them are of that form, which seems harmless if they are true - it means a compiler may deduce the same thing and omit code generation for an impossible to trigger error block. OTOH this could just be a weakness in the scanner. (i don't know how to silence it via markers in the code, but i assume it is possible) You'll need to login to Coverity to see the full report. https://scan.coverity.com/projects/python?tab=overview. (it has been ages since i've logged in, they appear to support Github logins now. yay.) As the parser.c code is new for 3.9, I'm marking this as deferred blocker. We should pay closer attention to the reports and update the parser generator code to generate code that passes analysis cleanly before we exit the beta phase. |
|||
msg371962 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2020-06-20 20:55 | |
Good catch! Are all the coverity complaints about this kind of code? if (_res == NULL && PyErr_Occurred()) { ... } (Mostly for Pablo and Lysandros:) This comes from emit_action(). It is preceded by a line of the form _res = <action>; Most of the time the action is a function call, so this looks like _res = <some function call which can really fail>; But occasionally the action is just pulling a named item from the alternative that was just recognized, and those variables are generally known to be non-NULL (because otherwise the alternative would fail). There seem to be a whole bunch of actions of the form { <variable> }. A typical example: else_block[asdl_seq*]: 'else' ':' b=block { b } We could probably recognize actions that take the form of a single variable and suppress the error check for the result. However, there's a wrinkle. Sometimes a variable may legitimately be NULL when an alternative is recognized! This could occur if the name refers to an optional item. I even found an example: | 'class' a=NAME b=['(' z=[arguments] ')' { z }] ':' c=block { Look closely at the part starting with b=: b=['(' z=[arguments] ')' { z }] In the sub-rule we see z=[arguments] and the action is { z } so it is possible that we reach the `_res = z` part while z is NULL. In my copy of parser.c (current master) this is in _tmp_69_rule(). IMO this makes it difficult to omit the error check in exactly the right situations. Moreover, isn't it the job of the compiler to optimize code so we don't have to? The code generator is complex enough as it is; I would prefer not to have to complexificate it just so Coverity won't complain about unreachable code (no matter how right it is!). I know nothing about Coverity any more. Is it possible to exempt this giant generated file from warnings about unreachable code? |
|||
msg374419 - (view) | Author: Pablo Galindo Salgado (pablogsal) * | Date: 2020-07-27 20:03 | |
I tried reprodicing the coverity results using cppcheck (I don't have access to the coverity project, someone needs to approve :() and I got just a dozen of results: ❯ cppcheck Parser/parser.c --enable=style Checking Parser/parser.c ... Parser/parser.c:3280:22: style: Condition '_res==NULL' is always false [knownConditionTrueFalse] if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:3275:16: note: Assuming that condition 'a=_gather_33_rule(p)' is not redundant (a = _gather_33_rule(p)) // ','.import_from_as_name+ ^ Parser/parser.c:3279:20: note: Assignment '_res=a', assigned value is 0 _res = a; ^ Parser/parser.c:3280:22: note: Condition '_res==NULL' is always false if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:3365:22: style: Condition '_res==NULL' is always false [knownConditionTrueFalse] if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:3360:16: note: Assuming that condition 'a=_gather_36_rule(p)' is not redundant (a = _gather_36_rule(p)) // ','.dotted_as_name+ ^ Parser/parser.c:3364:20: note: Assignment '_res=a', assigned value is 0 _res = a; ^ Parser/parser.c:3365:22: note: Condition '_res==NULL' is always false if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:6072:22: style: Condition '_res==NULL' is always false [knownConditionTrueFalse] if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:6067:16: note: Assuming that condition 'a=_loop1_68_rule(p)' is not redundant (a = _loop1_68_rule(p)) // (('@' named_expression NEWLINE))+ ^ Parser/parser.c:6071:20: note: Assignment '_res=a', assigned value is 0 _res = a; ^ Parser/parser.c:6072:22: note: Condition '_res==NULL' is always false if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:10662:22: style: Condition '_res==NULL' is always false [knownConditionTrueFalse] if (_res == NULL && PyErr_Occurred()) { ^ Parser/parser.c:10657:16: note: Assuming that condition 'a=expression_rule(p)' is not redundant (a = expression_rule(p)) // expression ^ Parser/parser.c:10661:20: note: Assignment '_res=a', assigned value is 0 _res = a; ^ Parser/parser.c:10662:22: note: Condition '_res==NULL' is always false if (_res == NULL && PyErr_Occurred()) { |
|||
msg376046 - (view) | Author: Lysandros Nikolaou (lys.nikolaou) * | Date: 2020-08-28 18:59 | |
I propose that we take no further action and close this. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:32 | admin | set | github: 85231 |
2020-08-28 19:05:36 | gvanrossum | set | status: open -> closed resolution: works for me stage: needs patch -> resolved |
2020-08-28 18:59:16 | lys.nikolaou | set | messages: + msg376046 |
2020-07-27 20:03:35 | pablogsal | set | messages: + msg374419 |
2020-06-21 19:21:47 | gregory.p.smith | set | nosy:
+ christian.heimes |
2020-06-20 20:55:30 | gvanrossum | set | messages: + msg371962 |
2020-06-20 19:15:33 | p-ganssle | set | nosy:
+ pablogsal |
2020-06-20 18:56:50 | gregory.p.smith | set | components: + Interpreter Core |
2020-06-20 18:56:31 | gregory.p.smith | create |