[issue41059] Large number of Coverity reports for parser.c

[Guido van Rossum]

Change by Guido van Rossum :


--
resolution:  -> works for me
stage: needs patch -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Lysandros Nikolaou]

Lysandros Nikolaou  added the comment:

I propose that we take no further action and close this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Pablo Galindo Salgado]

Pablo Galindo Salgado  added the comment:

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()) {

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Gregory P. Smith]

Change by Gregory P. Smith :


--
nosy: +christian.heimes

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Guido van Rossum]

Guido van Rossum  added the comment:

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 = ;

Most of the time the action is a function call, so this looks like

_res = ;

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 {  }. 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Paul Ganssle]

Change by Paul Ganssle :


--
nosy: +pablogsal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Gregory P. Smith]

Change by Gregory P. Smith :


--
components: +Interpreter Core

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue41059] Large number of Coverity reports for parser.c

[Gregory P. Smith]

New submission from Gregory P. Smith :

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.

--
assignee: gvanrossum
messages: 371956
nosy: gregory.p.smith, gvanrossum, lys.nikolaou, p-ganssle
priority: deferred blocker
severity: normal
stage: needs patch
status: open
title: Large number of Coverity reports for parser.c
versions: Python 3.10, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com