LOAD_GLOBAL used to load None under certain circumstances

@loewis

BPO	16619
Nosy	@loewis, @brettcannon, @birkenfeld, @terryjreedy, @ncoghlan, @vstinner, @benjaminp, @stevendaprano, @alex, @meadori, @durban
Files	16619-1.patch: quick and dirty patch. Just proves the point. It solves the issue for < 32700 bytes functions. const-node.patch

^{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 2012-12-06.22:41:12.773>
created_at = <Date 2012-12-05.19:28:31.623>
labels = ['interpreter-core', 'type-bug']
title = 'LOAD_GLOBAL used to load `None` under certain circumstances'
updated_at = <Date 2012-12-14.16:39:12.068>
user = 'https://bugs.python.org/brunodupuis'

bugs.python.org fields:

activity = <Date 2012-12-14.16:39:12.068>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2012-12-06.22:41:12.773>
closer = 'python-dev'
components = ['Interpreter Core']
creation = <Date 2012-12-05.19:28:31.623>
creator = 'bruno.dupuis'
dependencies = []
files = ['28217', '28219']
hgrepos = []
issue_num = 16619
keywords = ['patch']
message_count = 14.0
messages = ['176999', '177007', '177008', '177009', '177011', '177013', '177017', '177018', '177023', '177024', '177026', '177027', '177046', '177062']
nosy_count = 18.0
nosy_names = ['loewis', 'brett.cannon', 'georg.brandl', 'terry.reedy', 'ncoghlan', 'vstinner', 'ikelly', 'Horpner', 'benjamin.peterson', 'mrabarnett', 'steven.daprano', 'alex', 'cvrebert', 'meador.inge', 'daniel.urban', 'python-dev', 'DragonFireCK', 'bruno.dupuis']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16619'
versions = ['Python 3.4']

We found some strange behaviour of the compiler in this discussion on python-list: http://mail.python.org/pipermail/python-list/2012-December/636104.html

The fact is, return and return None result in inconsistent bytecode depending on the context.

Consider :

>>> import dis
>>> def f(x):
...     return None
... 
>>> dis.dis(f)
  2           0 LOAD_CONST               0 (None) 
              3 RETURN_VALUE         
>>> def g(x):
...     return None
...     print(x)
... 
>>> dis.dis(g)
  2           0 LOAD_GLOBAL              0 (None) 
              3 RETURN_VALUE

3 4 LOAD_GLOBAL 1 (print)
7 LOAD_FAST 0 (x)
10 CALL_FUNCTION 1 (1 positional, 0 keyword pair)
13 POP_TOP

return None statement results in LOAD_GLOBAL 0 if there is some unreachable code after it. I first saw that as an optimization issue, but Ian Kelly's message http://mail.python.org/pipermail/python-list/2012-December/636117.html gives an extensive analysis and some examples:

"""
I think this should even be considered a bug, not just a missing
optimization. Consider:

  >>> globals()['None'] = 42
  >>> def f(x):
  ...     return None
  ...     print(x)
  ...
  >>> f('test')
  42

The use of the LOAD_GLOBAL allows None to effectively be reassigned.

"""

Ian also points out in this message that return and return None don't result in the same bytecode when followed by trash code.

The same problem occurs with both False and True.

To me, the whole issue is at line ~ 480 in peehole.c in the LOAD_NAME/LOAD_GLOBAL switch case.

This explains the difference between return and return None as the former is actually compiled to LOAD_CONST. OTOH, return None has to pass the optim pass to be changed in LOAD_CONST. The real bug is sometime it doesn't.

line 426 in peehole.c :

if (codestr[codelen-1] != RETURN_VALUE)
goto exitUnchanged;

before the optim. I'm quite sure it's here. i patch and see...

This first patch spots the issue, but doesn't solve it if the bytecode of the function is > 32700 (see PyCode_Optimize comment). However with this patch, we get the LOAD_CONST anytime for None, True and False.

There is two questions :

1- is it safe to strip all the code after RETURN_VALUE as the patch does?

2- to correct this bug, we will need a deep refactoring of PyCode_Optimize (so that it accepts any code length).

The other way, is not to rely on PyCode_Optimize to compile return None/True/False, but do modifictations in the compiler itself. This must be the right way to do this, but it's far beyond my C skills and python core knowledge.

(2.6 is security fix only)

Stripping truly dead code after return is really tricky in general because
a) it might be in a conditional block, and
b) unreachable yield and assignment can affect compilation.
Assignments that make names local are detected on a first pass, but I do not know about yield. Anyway, I believe the policy has been to not do it.

The final patch will need a cpython-only test based on g(x), with dead code.

Possibly intersecting issues are proposals to change where optimization is done and, for testing, to add a generator to dis so its output can be directly captured and analyzed instead of having to redirect, capture, and parse stdout.

We definitely need to put the code that loads constants with LOAD_CONST out of the optimization code. It's not optim, it's a language feature: None *is* a 'singleton' constant.

I'm trying to figure out how to change compile.c to achieve this, as it's my first dive into the compiler code, it's not that easy.

Another approch is to strip unreachable nodes in AST, but

a) it's quite complex, as Terry said

b) it solves only this particular bug, not the general assertion "None, True and False are reserved words bound to constants. It can not ever be loaded with LOAD_NAME or LOAD_GLOBAL"

Here's what I think we should do for 3.4. Nick, care to commment?

First reaction is +1 for finally switching to real constant nodes in the AST for 3.4+. This is an inherited behaviour from 2.x where these were ordinary names rather than true keywords, so we weren't able to completely disallow overwriting them.

As a smaller impact change for earlier versions, we should be able to do something in compiler_nameop [1] that picks up the 3 singleton names and allows only LOAD_CONST, erroring out otherwise.

http://hg.python.org/cpython/file/default/Python/compile.c#l2625

Nick, None was a constant even in 2k

True and False weren't, though (and even None wasn't a proper keyword)

The difference in the errors below is the reason the systematic fix in Benjamin's patch simply wasn't practical in 2.x (as it would have required a complex deprecation dance to turn None, True and False into real keywords):

Python 2.7.3 (default, Jul 24 2012, 10:05:38) 
[GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C: pass
... 
>>> C.None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: class C has no attribute 'None'
>>> 

Python 3.2.3 (default, Jun  8 2012, 05:36:09) 
[GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C: pass
... 
>>> C.None
  File "<stdin>", line 1
    C.None
         ^
SyntaxError: invalid syntax

I claim that this is not a bug in the existing versions. It is documented as "illegal" to assign to None:

http://docs.python.org/2.7/library/constants.html?highlight=none#None

So what exactly happens if you manage to bypass the existing checks is implementation-specific (or, in the C sense, "undefined behavior"). Therefore, the reaction of Python 2.7 (e.g.) is perfectly fine.

New changeset 03f92a9f0875 by Benjamin Peterson in branch 'default':
create NameConstant AST class for None, True, and False literals (closes bpo-16619)
http://hg.python.org/cpython/rev/03f92a9f0875

Navigation Menu

Repository navigation

LOAD_GLOBAL used to load `None` under certain circumstances #60823

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

LOAD_GLOBAL used to load None under certain circumstances #60823

Description

Activity

brunodupuis commented on Dec 5, 2012

mrabarnett commented on Dec 5, 2012

brunodupuis commented on Dec 5, 2012

brunodupuis commented on Dec 5, 2012

brunodupuis commented on Dec 5, 2012

terryjreedy commented on Dec 5, 2012

brunodupuis commented on Dec 6, 2012

benjaminp commented on Dec 6, 2012

ncoghlan commented on Dec 6, 2012

alex commented on Dec 6, 2012

ncoghlan commented on Dec 6, 2012

ncoghlan commented on Dec 6, 2012

loewis commented on Dec 6, 2012

python-dev commented on Dec 6, 2012

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

LOAD_GLOBAL used to load `None` under certain circumstances #60823