classification
Title: Large number of Coverity reports for parser.c
Type: Stage: needs patch
Components: Interpreter Core Versions: Python 3.10, Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: christian.heimes, gregory.p.smith, gvanrossum, lys.nikolaou, p-ganssle, pablogsal
Priority: deferred blocker Keywords:

Created on 2020-06-20 18:56 by gregory.p.smith, last changed 2020-07-27 20:03 by pablogsal.

Messages (3)
msg371956 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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()) {
History
Date User Action Args
2020-07-27 20:03:35pablogsalsetmessages: + msg374419
2020-06-21 19:21:47gregory.p.smithsetnosy: + christian.heimes
2020-06-20 20:55:30gvanrossumsetmessages: + msg371962
2020-06-20 19:15:33p-gansslesetnosy: + pablogsal
2020-06-20 18:56:50gregory.p.smithsetcomponents: + Interpreter Core
2020-06-20 18:56:31gregory.p.smithcreate