Bug 810847 - Python problem with nested IFs
Python problem with nested IFs
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: python (Show other bugs)
6.2
Unspecified Unspecified
unspecified Severity medium
: rc
: ---
Assigned To: Dave Malcolm
Petr Šplíchal
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-09 07:50 EDT by Zied Fakhfakh
Modified: 2013-02-21 05:31 EST (History)
0 users

See Also:
Fixed In Version: python-2.6.6-35.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-21 05:31:42 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Proposed patch to change list comprehensions/generator expressions to simply emit a single POP_TOP in the cleanup block (if there is one) (872 bytes, patch)
2012-04-09 15:20 EDT, Dave Malcolm
no flags Details | Diff

  None (edit)
Description Zied Fakhfakh 2012-04-09 07:50:48 EDT
Description of problem:
A code of 4 or more nested IFs fails to compile

Version-Release number of selected component (if applicable):
python-2.6.6-29


How reproducible:
always

Steps to Reproduce:
1.python -c "[1 for x in () if 1 if 1 if 1 if 1]"
  
Actual results:
python: Python/compile.c:3437: stackdepth_walk: Assertion `depth >= 0' failed.
Aborted (core dumped)

Expected results:


Additional info:
It's very serious to openerp as we cannot build it on RHEL6 because of this bug.
on other distros it seems to be working fine.
Comment 2 Dave Malcolm 2012-04-09 15:15:25 EDT
Thanks for filing this bug report.

I checked the latest version of the upstream hg.python.org 2.6 code, configured
  --with-pydebug
and the assertion failure *does* affect that code (that upstream branch is closed for all changes but security fixes).

You're seeing this symptom on RHEL 6's Python because we ship with C-level assertions enabled.

This affects list comprehensions with numerous "if" clauses (like the reproducer you given in the initial report)

This also affects generator expressions: as per your reproducer, but with parentheses, giving a generator:

  $ python -c "(1 for x in () if 1 if 1 if 1 if 1)"
  python: Python/compile.c:3437: stackdepth_walk: Assertion `depth >= 0' failed.
  Aborted (core dumped)

However I believe it does *not* affect the "if" statement.

It does not affect Python 2.7, which appears to be due to the rewrite of list comprehensions to use the new POP_JUMP_IF_{TRUE,FALSE} opcodes (was http://bugs.python.org/issue4715 ; see http://hg.python.org/cpython/rev/51920)
Comment 3 Dave Malcolm 2012-04-09 15:16:07 EDT
Upon removing the failing assert, dis.dis() shows that this is what would be assembled:
$ ./python -c "import dis; dis.dis(lambda: [1 for x in () if 1 if 1 if 1 if 1])"
  1           0 BUILD_LIST               0
              3 DUP_TOP             
              4 STORE_FAST               0 (_[1])
              7 LOAD_CONST               1 (())
             10 GET_ITER            
        >>   11 FOR_ITER                29 (to 43)
             14 STORE_FAST               1 (x)
             17 LOAD_FAST                0 (_[1])
             20 LOAD_CONST               0 (1)
             23 LIST_APPEND         
             24 JUMP_ABSOLUTE           32
             27 POP_TOP             
             28 JUMP_ABSOLUTE           36
             31 POP_TOP             
        >>   32 JUMP_ABSOLUTE           40
             35 POP_TOP             
        >>   36 JUMP_ABSOLUTE           11
             39 POP_TOP             
        >>   40 JUMP_ABSOLUTE           11
        >>   43 DELETE_FAST              0 (_[1])
             46 RETURN_VALUE        
[16859 refs]

Turning off the peephole optimizer shows that this bytecode would be emitted:
$ ./python -c "import dis; dis.dis(lambda: [1 for x in () if 1 if 1 if 1 if 1])"
  1           0 BUILD_LIST               0
              3 DUP_TOP             
              4 STORE_FAST               0 (_[1])
              7 BUILD_TUPLE              0
             10 GET_ITER            
        >>   11 FOR_ITER                57 (to 71)
             14 STORE_FAST               1 (x)
             17 LOAD_CONST               0 (1)
             20 JUMP_IF_FALSE           32 (to 55)
             23 POP_TOP             
             24 LOAD_CONST               0 (1)
             27 JUMP_IF_FALSE           25 (to 55)
             30 POP_TOP             
             31 LOAD_CONST               0 (1)
             34 JUMP_IF_FALSE           18 (to 55)
             37 POP_TOP             
             38 LOAD_CONST               0 (1)
             41 JUMP_IF_FALSE           11 (to 55)
             44 POP_TOP             
             45 LOAD_FAST                0 (_[1])
             48 LOAD_CONST               0 (1)
             51 LIST_APPEND         
             52 JUMP_FORWARD             1 (to 56)
        >>   55 POP_TOP             
        >>   56 JUMP_FORWARD             1 (to 60)
             59 POP_TOP             
        >>   60 JUMP_FORWARD             1 (to 64)
             63 POP_TOP             
        >>   64 JUMP_FORWARD             1 (to 68)
             67 POP_TOP             
        >>   68 JUMP_ABSOLUTE           11
        >>   71 DELETE_FAST              0 (_[1])
             74 RETURN_VALUE        
[16871 refs]

It appears to me that Python/compiler.c:compiler_listcomp_generator (and the analogous code in compiler_genexp_generator) has a bug:  the loop

  2653      for (i = 0; i < n; i++) {
  2654          ADDOP_I(c, JUMP_FORWARD, 1);
  2655          if (i == 0)
  2656              compiler_use_next_block(c, if_cleanup);
  2657          ADDOP(c, POP_TOP);
  2658      }

for the case where there is 1 or more "if" within the list comprehension generates the instructions that becomes these bytecodes:

52     JUMP_FORWARD 1
    if_cleanup:
55     POP_TOP
56     JUMP_FORWARD 1
59     POP_TOP
60     JUMP_FORWARD 1
63     POP_TOP
64     JUMP_FORWARD 1
67     POP_TOP

However, within if_cleanup, only 55 and 56, and the chain of subsequent JUMP_FORWARD instructions are reachable: none of the other POP_TOP commands can actually be reached (and only a single POP_TOP is needed, for popping the expression used by the failed conditional from the stack).

It's this "basic block" that confuses the stack depth calculator: it does not take the JUMP_FORWARD expressions into account, and mistakenly calculates the stack depth as if all of the POP_TOP instructions are executed: popping 4 values from a stack of depth 3 would crash the interpreter, and hence the assertion fires.
Comment 4 Dave Malcolm 2012-04-09 15:16:34 EDT
Some possible approaches:
  (a) fix compiler_listcomp_generator/compiler_genexp_generator to avoid generating the redundant POP_TOP operations (am about to attach a patch)
  (b) remove the assertion
  (c) turn off C-level assertions
Comment 5 Dave Malcolm 2012-04-09 15:20:35 EDT
Created attachment 576290 [details]
Proposed patch to change list comprehensions/generator expressions to simply emit a single POP_TOP in the cleanup block (if there is one)

Proposed patch to change list comprehensions/generator expressions to simply emit a single POP_TOP in the cleanup block.

With this patch, with the peephole optimizer disabled, the "raw" disassembly looks like this:
$ ./python -c "import dis; dis.dis(lambda: [1 for x in () if 1 if 1 if 1 if 1])"
  1           0 BUILD_LIST               0
              3 DUP_TOP             
              4 STORE_FAST               0 (_[1])
              7 BUILD_TUPLE              0
             10 GET_ITER            
        >>   11 FOR_ITER                45 (to 59)
             14 STORE_FAST               1 (x)
             17 LOAD_CONST               0 (1)
             20 JUMP_IF_FALSE           32 (to 55)
             23 POP_TOP             
             24 LOAD_CONST               0 (1)
             27 JUMP_IF_FALSE           25 (to 55)
             30 POP_TOP             
             31 LOAD_CONST               0 (1)
             34 JUMP_IF_FALSE           18 (to 55)
             37 POP_TOP             
             38 LOAD_CONST               0 (1)
             41 JUMP_IF_FALSE           11 (to 55)
             44 POP_TOP             
             45 LOAD_FAST                0 (_[1])
             48 LOAD_CONST               0 (1)
             51 LIST_APPEND         
             52 JUMP_FORWARD             1 (to 56)
        >>   55 POP_TOP             
        >>   56 JUMP_ABSOLUTE           11
        >>   59 DELETE_FAST              0 (_[1])
             62 RETURN_VALUE        
[16859 refs]

and with the peephole optimizer re-enabled, the optimized disassembly looks like this:
$ ./python -c "import dis; dis.dis(lambda: [1 for x in () if 1 if 1 if 1 if 1])"
  1           0 BUILD_LIST               0
              3 DUP_TOP             
              4 STORE_FAST               0 (_[1])
              7 LOAD_CONST               1 (())
             10 GET_ITER            
        >>   11 FOR_ITER                17 (to 31)
             14 STORE_FAST               1 (x)
             17 LOAD_FAST                0 (_[1])
             20 LOAD_CONST               0 (1)
             23 LIST_APPEND         
             24 JUMP_ABSOLUTE           11
             27 POP_TOP             
             28 JUMP_ABSOLUTE           11
        >>   31 DELETE_FAST              0 (_[1])
             34 RETURN_VALUE        
[16859 refs]

(Compare with comment #3.  Both with and without the patch, the peephole optimizer entirely eliminates the "if 1" conditionals, but with this patch it does a better job of eliminating the redundant cleanup code).

The patch passes a full "make test" of the upstream test suite, although that appears to contain few examples of multiple conditionals.

As a more concrete example, the following compiles correctly:
$ ./python -c "import dis; dis.dis(lambda: [x for x in (1, 2, 3, 4) if x>=1 if x<=4 if x>1 if x<4])"
  1           0 BUILD_LIST               0
              3 DUP_TOP             
              4 STORE_FAST               0 (_[1])
              7 LOAD_CONST               4 ((1, 2, 3, 4))
             10 GET_ITER            
        >>   11 FOR_ITER                69 (to 83)
             14 STORE_FAST               1 (x)
             17 LOAD_FAST                1 (x)
             20 LOAD_CONST               0 (1)
             23 COMPARE_OP               5 (>=)
             26 JUMP_IF_FALSE           50 (to 79)
             29 POP_TOP             
             30 LOAD_FAST                1 (x)
             33 LOAD_CONST               3 (4)
             36 COMPARE_OP               1 (<=)
             39 JUMP_IF_FALSE           37 (to 79)
             42 POP_TOP             
             43 LOAD_FAST                1 (x)
             46 LOAD_CONST               0 (1)
             49 COMPARE_OP               4 (>)
             52 JUMP_IF_FALSE           24 (to 79)
             55 POP_TOP             
             56 LOAD_FAST                1 (x)
             59 LOAD_CONST               3 (4)
             62 COMPARE_OP               0 (<)
             65 JUMP_IF_FALSE           11 (to 79)
             68 POP_TOP             
             69 LOAD_FAST                0 (_[1])
             72 LOAD_FAST                1 (x)
             75 LIST_APPEND         
             76 JUMP_ABSOLUTE           11
        >>   79 POP_TOP             
             80 JUMP_ABSOLUTE           11
        >>   83 DELETE_FAST              0 (_[1])
             86 RETURN_VALUE        
[16876 refs]
Comment 7 Dave Malcolm 2012-04-09 15:30:53 EDT
(In reply to comment #0)
> It's very serious to openerp as we cannot build it on RHEL6 because of this
> bug.
> on other distros it seems to be working fine.

Thanks again for filing this bug.

Note that bugzilla is merely a bug-reporting mechanism.  

Have you filed a support ticket with Red Hat Support about this yet?  See http://access.redhat.com/
Comment 9 Zied Fakhfakh 2012-04-10 14:00:50 EDT
(In reply to comment #4)
> Some possible approaches:
>   (a) fix compiler_listcomp_generator/compiler_genexp_generator to avoid
> generating the redundant POP_TOP operations (am about to attach a patch)
>   (b) remove the assertion
>   (c) turn off C-level assertions

I changed the problematic code by a patched file from Upstream openerp and it compiles perfectly.

Indeed at 2.7 this no more a problem.

Thank you for your prompt reactivity.
Comment 10 RHEL Product and Program Management 2012-07-10 04:39:43 EDT
This request was not resolved in time for the current release.
Red Hat invites you to ask your support representative to
propose this request, if still desired, for consideration in
the next release of Red Hat Enterprise Linux.
Comment 11 RHEL Product and Program Management 2012-07-10 21:57:53 EDT
This request was erroneously removed from consideration in Red Hat Enterprise Linux 6.4, which is currently under development.  This request will be evaluated for inclusion in Red Hat Enterprise Linux 6.4.
Comment 12 RHEL Product and Program Management 2012-08-14 18:01:00 EDT
This request was evaluated by Red Hat Product Management for
inclusion in a Red Hat Enterprise Linux release.  Product
Management has requested further review of this request by
Red Hat Engineering, for potential inclusion in a Red Hat
Enterprise Linux release for currently deployed products.
This request is not yet committed for inclusion in a release.
Comment 19 errata-xmlrpc 2013-02-21 05:31:42 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2013-0437.html

Note You need to log in before you can comment on or make changes to this bug.