Windows: 3.3.0-rc2.msi: test_buffer fails

@loewis

BPO	15993
Nosy	@loewis, @vstinner, @larryhastings, @tjguk, @briancurtin, @zware, @zooba
Files	profile.bat profiletests.txt issue15993.diff ull_vctest.diff: Quick-and-dirty alternative version of PyLong_AsUnsignedLongLong(). Just for testing.

^{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 2015-07-05.02:07:22.785>
created_at = <Date 2012-09-20.20:31:49.341>
labels = ['interpreter-core', 'build', 'OS-windows']
title = 'Windows: 3.3.0-rc2.msi: test_buffer fails'
updated_at = <Date 2015-07-05.02:07:22.784>
user = 'https://github.com/skrah'

bugs.python.org fields:

activity = <Date 2015-07-05.02:07:22.784>
actor = 'steve.dower'
assignee = 'none'
closed = True
closed_date = <Date 2015-07-05.02:07:22.785>
closer = 'steve.dower'
components = ['Interpreter Core', 'Windows']
creation = <Date 2012-09-20.20:31:49.341>
creator = 'skrah'
dependencies = []
files = ['27251', '27252', '27253', '35681']
hgrepos = []
issue_num = 15993
keywords = ['patch']
message_count = 32.0
messages = ['170844', '170858', '170873', '170916', '170917', '170918', '170919', '170920', '170923', '170924', '170929', '170935', '170971', '170973', '170981', '170985', '170988', '170989', '220180', '220183', '220531', '220545', '220551', '220557', '220894', '220932', '220992', '221380', '221381', '242548', '246282', '246287']
nosy_count = 9.0
nosy_names = ['loewis', 'vstinner', 'larry', 'tim.golden', 'nadeem.vawda', 'brian.curtin', 'BreamoreBoy', 'zach.ware', 'steve.dower']
pr_nums = []
priority = 'critical'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue15993'
versions = ['Python 3.5']

I've installed 3.3.0-rc2 on Windows-7 64-bit using the msi installer.
I'm getting these failures in test_buffer, but I can *not* reproduce
them when I build Win-32/pgo/python.exe from source:

======================================================================
FAIL: test_memoryview_assign (test.test_buffer.TestBufferProtocol)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "C:\Program Files (x86)\Python33\lib\test\test_buffer.py", line 2863,
    self.assertEqual(m[i], 8)
AssertionError: 34359738368 != 8

======================================================================
FAIL: test_memoryview_struct_module (test.test_buffer.TestBufferProtocol)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "C:\Program Files (x86)\Python33\lib\test\test_buffer.py", line 2476,
    self.assertEqual(m[0], nd[0])
AssertionError: 15080797365275624638 != 6838299039298601293

Both lzma and memoryview use PyLong_AsUnsignedLongLong() in the
affected code paths. I get this with the msi installed python.exe:

>>> import array
>>> a = array.array('Q', [1,2,3,4])
>>> m = memoryview(a)
>>> m[0] = 4
>>> m[0]
17179869184
>>>

And the correct result with the self-compiled (PGO) python.exe:

>>> import array
>>> a = array.array('Q', [1,2,3,4])
>>> m = memoryview(a)
>>> m[0] = 4
>>> m[0]
4

The high and low words of the 64-bit value are switched:

>>> a = array.array('Q', [1])
>>> m = memoryview(a)
>>> m[0]= 2**32+5
>>> m[0]
21474836481
>>> struct.unpack_from('8s', m, 0)
(b'\x01\x00\x00\x00\x05\x00\x00\x00',)

Can anyone reproduce this in a source build? I think this should
be a blocker.

Issue bpo-15995 was marked as a duplicate of this issue. Copy of its initial message (msg170853):

This is similar to bpo-15993: With the installed Python from the rc2-msi
test_lzma fails. I cannot reproduce the failure with python.exe (PGO)
compiled from source:

======================================================================                                   
ERROR: test__decode_filter_properties (test.test_lzma.MiscellaneousTestCase)                             
----------------------------------------------------------------------                                   
Traceback (most recent call last):                                                                       
  File "C:\Python33\lib\test\test_lzma.py", line 1105, in test__decode_filter_properties                 
    lzma.FILTER_LZMA1, b"]\x00\x00\x80\x00")                                                             
ValueError: Invalid filter ID: 4611686018427387905                                                       
                                                                                                         
======================================================================                                   
ERROR: test_filter_properties_roundtrip (test.test_lzma.MiscellaneousTestCase)                           
----------------------------------------------------------------------                                   
Traceback (most recent call last):                                                                       
  File "C:\Python33\lib\test\test_lzma.py", line 1114, in test_filter_properties_roundtrip               
    lzma.FILTER_LZMA1, b"]\x00\x00\x80\x00")                                                             
ValueError: Invalid filter ID: 4611686018427387905

I fail to reproduce the issue on Windows 7 (Version 6.0.1, number 7601, Service Pack 1):
---
Microsoft Windows [version 6.1.7601]
Copyright (c) 2009 Microsoft Corporation. Tous droits réservés.

C:\Users\haypo>cd \python33

C:\Python33>python.exe
Python 3.3.0rc2 (v3.3.0rc2:88a0792e8ba3, Sep  9 2012, 10:13:38) [MSC v.1600 64 b
it (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> exit()

C:\Python33>python.exe -m test test_buffer test_lzma
[1/2] test_buffer
[2/2] test_lzma
All 2 tests OK.
---
I'm running Windows 7 in KVM, my host CPU is a Intel i7-2600.

I'm getting these failures in test_buffer,
but I can *not* reproduce them when I build
Win-32/pgo/python.exe from source:

The issue looks to be specific to 64 bits binaries. You need the professional version of Visual Studio 10. The express version doesn't support 64 bits. I only have the Express version.

Declaring this a release blocker is technically difficult. If it is a release blocker, further releases cannot be done until it is resolved. Since it is an issue with the binary only, the only possible way to resolve this is with a release. So declaring this a release blocker essentially deadlocks the release.

For the record, the released binary is not just a PGO build, but has been trained with the attached training script.

You need the professional version of Visual Studio 10.
The express version doesn't support 64 bits. I only have the
Express version.

Ah yes, I now remember my issue with VS10 Express: when I set the project to 64 bits, I get such error popup:
http://www.haypocalc.com/tmp/visual_studio_64bits.png

Which version should I try? Ultimate? Premium? Professional?

STINNER Victor <[email protected]> wrote:

Which version should I try? Ultimate? Premium? Professional?

Try Ultimate, it's AFAIK the only version these days that supports PGO.

Martin v. Löwis <[email protected]> wrote:

For the record, the released binary is not just a PGO build, but has
been trained with the attached training script.

Thanks. Now I can reproduce the issue with a source build.

I'm using Ultimate, but I think Professional should provide you with all required tools.

It's a bit late here already, but unless I'm missing something I think
this is an optimizer bug. I'm attaching a workaround for memoryview.c
and _lzmamodule.c.

System: Windows 7 64-bit
Build (unpatched): PCBuild\Win32-pgo\python.exe, trained with profile.bat

In the unpatched version, I stepped through this test case in the debugger:

import array
a = array.array('Q', [1])
m = memoryview(a)
m[0] = 1

At Objects/memoryobject.c:1572: llu == 1

At Objects/memoryobject.c:1782: pylong_as_llu returned value 4294967296

So I think it's pretty safe to say that this is indeed an optimizer bug.

Isn't PyLong_FromUnsignedLongLong() still involved through spec_add_field()?

Please don't. If the compiler is demonstrated to generate bad code in one case, we should *not* exclude that code from optimization, but not use optimization at all. How many other places will there be which also cause bad code being generated that just happens not to be uncovered by the test suite?

Isn't PyLong_FromUnsignedLongLong() still involved through spec_add_field()?

The two issues were unrelated - the 'invalid filter ID' (4611686018427387905 == 0x40000000_00000001) is the correct value but the wrong branch in the switch was taken, leading to the error message.

If the compiler is demonstrated to generate bad code in one case, we should *not* exclude that code from optimization, but not use optimization at all.

By that logic, we should be using a debug build on every platform... I've encountered various codegen bugs in gcc and MSVC, though they've all been fixed (apart from this one). All developers are human, including most compiler writers.

That said, I'll wait on the response from the PGO team. If they don't give me enough confidence, then I'll happily forget about the whole idea of using it for 3.5.

I'd be fine to reconsider if a previously-demonstrated bug is now demonstrated-fixed. However, if the actual bug persists, optimization should be disabled for all code, not just for the code that allows to demonstrate the bug. This principle should indeed been followed for all platforms (and it has, e.g. on hpux).

The two issues were unrelated - the 'invalid filter ID'
(4611686018427387905 == 0x40000000_00000001) is the correct
value but the wrong branch in the switch was taken, leading
to the error message.

Unfortunately I don't have a Visual Studio setup right now.

It seems to me that at the time the wrong branch is taken, f->id
could be in the registers in the wrong order (same as in msg170985),
but when the error message is printed, the value is read from
memory. This is just a guess of course.

As Martin, I'm uncomfortable that the memoryview issue no longer
appears, but this one still does.

I've attached an alternative version of PyLong_AsUnsignedLongLong()
that is just intended for testing the compiler.

If the optimizer does whole progam optimization, it might choke
on _PyLong_AsByteArray().

I'd be fine to reconsider if a previously-demonstrated bug is now
demonstrated-fixed. However, if the actual bug persists, optimization
should be disabled for all code, not just for the code that allows to
demonstrate the bug.

I'm okay with that. I thought you meant never enable optimizations with that compiler ever again, which is obviously ridiculous and I should have dismissed the idea on that basis rather than posting a snarky response. Sorry.

It seems to me that at the time the wrong branch is taken, f->id
could be in the registers in the wrong order (same as in msg170985),
but when the error message is printed, the value is read from
memory. This is just a guess of course.

I checked that and the registers are fine. Here's the snippet of disassembly I posted with the bug I filed:

mov edx,dword ptr [edi+4] ; == 0x40000000
mov ecx,dword ptr [edi] ; == 0x00000001
test edx,edx ; should be cmp edx,40000000h or equiv.
ja lbl1 ; 'default:'
jb lbl2 ; should be je after change above
cmp ecx,21h
jbe lbl2 ; should probably be lbl3
lbl1:
; default:
...
lbl2:
cmp ecx,1
jne lbl3
; case 0x4000000000000001
...

It's clearly an incorrect test opcode, and I'd expect switch statements where the first case is a 64-bit integer larger than 2**32 to be rare - I've certainly never encountered one before - which is why such a bug could go undiscovered.

When I looked at the disassembly for memoryview it was fine. I actually spent far longer than I should have trying to find the bug that was no longer there...

Also bear in mind that I'm working with VC14 and not VC10, so the difference is due to the compiler and not simply time or magic :)

This has been confirmed as a bug in VC14 (and earlier) and there'll be a fix going in soon.

For those interested, here's a brief rundown of the root cause:

the switch in build_filter_spec() switches on a 64-bit value
one case is 0x4000000000000001 and the rest are <=0x21
PGO detects that 0x4000000000000001 is the hot case
(bug starts here)
PGO detects that the cold cases are 32-bits or less and so enables an optimisation to skip comparing the high DWORD
PGO adds check for the hot case, but using the 32-bit optimisation

it checks for "0x1" rather than the full value
(bug ends here)

PGO adds checks for cold cases

The fix will be to check both hot and cold cases to see whether the 32-bit optimisation can be used. A "workaround" (that I wouldn't dream of using, but it illustrates the issue) would be to add a dead case that requires 64-bits. This would show up in the list of cold cases and prevent the 32-bit optimisation from being used.

No indication of when the fix will go in, but it should be in the next public release, and I'll certainly be able to test it in advance of that.

Thanks a lot for this investigation; I'm glad you are working on this.

Is this now fixed in VS? I don't believe I can test myself as I've only got express/community editions.

So, the purpose in marking this as a "release blocker" is so that we can hold up the release while we wait for Microsoft to release a new compiler? If our approach to fixing this is to get the compiler fixed, I can live with marking this as "critical", but not "release blocker".

Eh, why bother. I don't remember if the fix is in for 3.5.0b3, but I'll vouch that the compiler build with the fix does exist and will be used for 3.5, so this should just be closed (again).

Navigation Menu

Repository navigation

Windows: 3.3.0-rc2.msi: test_buffer fails #60197

11 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Windows: 3.3.0-rc2.msi: test_buffer fails #60197

Description

Activity

skrah commented on Sep 20, 2012

skrah commented on Sep 20, 2012

skrah commented on Sep 21, 2012

vstinner commented on Sep 21, 2012

vstinner commented on Sep 21, 2012

loewis commented on Sep 21, 2012

loewis commented on Sep 21, 2012

vstinner commented on Sep 21, 2012

skrah commented on Sep 21, 2012

skrah commented on Sep 21, 2012

loewis commented on Sep 21, 2012

skrah commented on Sep 21, 2012

skrah commented on Sep 22, 2012

11 remaining items

skrah commented on Jun 14, 2014

loewis commented on Jun 14, 2014

zooba commented on Jun 14, 2014

loewis commented on Jun 17, 2014

skrah commented on Jun 18, 2014

zooba commented on Jun 19, 2014

zooba commented on Jun 23, 2014

loewis commented on Jun 23, 2014

BreamoreBoy commented on May 4, 2015

larryhastings commented on Jul 5, 2015

zooba commented on Jul 5, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions