Repository navigation
Windows: 3.3.0-rc2.msi: test_buffer fails #60197
Description
Activity
skrah commented
on Sep 20, 2012 skrahmannequinMannequinAuthorI'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 != 6838299039298601293skrah commented
on Sep 20, 2012 skrahmannequinMannequinAuthorBoth 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
skrah commented
on Sep 21, 2012 skrahmannequinMannequinAuthorThe 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.
loewis commented
on Sep 21, 2012 loewismannequinMannequinDeclaring 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.
loewis commented
on Sep 21, 2012 loewismannequinMannequinFor 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.pngWhich version should I try? Ultimate? Premium? Professional?
skrah commented
on Sep 21, 2012 skrahmannequinMannequinAuthorSTINNER 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.
skrah commented
on Sep 21, 2012 skrahmannequinMannequinAuthorMartin 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.
loewis commented
on Sep 21, 2012 loewismannequinMannequinI'm using Ultimate, but I think Professional should provide you with all required tools.
skrah commented
on Sep 21, 2012 skrahmannequinMannequinAuthorIt'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.skrah commented
on Sep 22, 2012 skrahmannequinMannequinAuthorSystem: Windows 7 64-bit
Build (unpatched): PCBuild\Win32-pgo\python.exe, trained with profile.batIn 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.
11 remaining items
skrah commented
on Jun 14, 2014 skrahmannequinMannequinAuthorIsn't PyLong_FromUnsignedLongLong() still involved through spec_add_field()?
loewis commented
on Jun 14, 2014 loewismannequinMannequinPlease 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.
loewis commented
on Jun 17, 2014 loewismannequinMannequinI'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).
skrah commented
on Jun 18, 2014 skrahmannequinMannequinAuthorThe 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
testopcode, 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.
loewis commented
on Jun 23, 2014 loewismannequinMannequinThanks a lot for this investigation; I'm glad you are working on this.
BreamoreBoy commented
on May 4, 2015 BreamoreBoymannequinMannequinIs 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).
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:
bugs.python.org fields: