regression in Cython when pickling objects

[tacaswell]

BPO	37289
Nosy	@scoder, @vstinner, @serhiy-storchaka, @tacaswell, @pablogsal
PRs	bpo-37289: Remove 'if False' handling in the peephole optimizer #14099 [3.7] bpo-37289: Remove 'if False' handling in the peephole optimizer (GH-14099) #14111 [3.8] bpo-37289: Remove 'if False' handling in the peephole optimizer (GH-14099) #14112 bpo-37289: Add a test for if with ifexpr in the peephole optimizer to detect regressions #14127
Files	np_fail.txt cython_test_log.txt

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-10-26.19:49:52.788>
created_at = <Date 2019-06-15.03:43:32.062>
labels = ['3.8', 'type-bug', '3.7', '3.9']
title = 'regression in Cython when pickling objects'
updated_at = <Date 2019-10-26.19:49:52.787>
user = 'https://github.com/tacaswell'

bugs.python.org fields:

activity = <Date 2019-10-26.19:49:52.787>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2019-10-26.19:49:52.788>
closer = 'serhiy.storchaka'
components = []
creation = <Date 2019-06-15.03:43:32.062>
creator = 'tcaswell'
dependencies = []
files = ['48421', '48422']
hgrepos = []
issue_num = 37289
keywords = ['patch']
message_count = 21.0
messages = ['345655', '345658', '345660', '345664', '345669', '345670', '345673', '345676', '345682', '345685', '345686', '345687', '345688', '345690', '345691', '345693', '345698', '345702', '345718', '346164', '355423']
nosy_count = 5.0
nosy_names = ['scoder', 'vstinner', 'serhiy.storchaka', 'tcaswell', 'pablogsal']
pr_nums = ['14099', '14111', '14112', '14127']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue37289'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[tacaswell]

The fix for
https://bugs.python.org/issue37213 in 3498c64 (#13969) seems to break building numpy (master) with cython (master) due to a pickle failure.

The traceback (which is mostly in the cython source) is in an attachment.

I bisected it back to this commit and tested that reverting this commit fixes the numpy build.

[tacaswell]

This change also causes failures in the cython test suite (see attached).

[pablogsal]

Thanks for the report. I will investigate, but given that the fix just optimizes bytecode as it was done in 3.7, and this is a picke failure, it seems that is likely that some data may need to be regenerated in Cython?

Stephan probably can confirm this meanwhile we investigate.

[pablogsal]

Did you open a bug in the Cython tracker?

[pablogsal]

[I have deleted some comments claiming that I was not able to reproduce the issue because I managed to reproduce the issue correctly at the end]

[pablogsal]

I managed to find the original cause, although I don't understand exactly why it happens. The error is caused by the 'if False' handling of #13332. There is some failure in that logic, so for now I made a PR reverting that change.

[serhiy-storchaka]

Could you please provide an example of the improperly optimized code?

[pablogsal]

I was not able to find it in Cython but PR14099 fixes it so I prefer to revert the optimization for now and then try to add it back once we understand what's failing exactly.

[pablogsal]

New changeset 7a68f8c by Pablo Galindo in branch 'master':
bpo-37289: Remove 'if False' handling in the peephole optimizer (GH-14099)
7a68f8c

[pablogsal]

Serhiy, this is the code that was incorrectly optimized:

3 0 LOAD_GLOBAL 0 (g)
2 LOAD_ATTR 1 (code)
4 POP_JUMP_IF_FALSE 8
6 JUMP_FORWARD 4 (to 12)
>> 8 LOAD_CONST 0 (None)
10 POP_JUMP_IF_FALSE 20

4 >> 12 LOAD_GLOBAL 2 (print)
14 LOAD_CONST 2 ('Called!')
16 CALL_FUNCTION 1
18 POP_TOP
>> 20 LOAD_CONST 0 (None)
22 RETURN_VALUE

def g():
    if True if g.__code__ else None:
        print("Called!")

g()

The problem is that [LOAD_CONST 0 (None)] made the peephole to delete the block because it does not detect that previous conditions in the same conditional could be True. The general case was made by looking back for POP_JUMP_IF_{FALSE,TRUE} but with this construct JUMP_FORWARD appears.

I think handling this condition in the peephole optimizer can be very dangerous error prone to get right because all the information is lost, but doing it at the ast level makes us lose syntax errors in optimized blocks, which is also wrong. Any ideas?

It happens here in Cython:

https://github.com/cython/cython/blob/master/Cython/Compiler/Nodes.py#L5131

That's why the errors manifest as pickling errors