classification
Title: [PATCH] Configure should enable -fwrapv for clang
Type: behavior Stage: resolved
Components: Build Versions: Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: adam@NetBSD.org, brett.cannon, cartman, haypo, jcea, jmr, mark.dickinson, pitrou, python-dev, rhettinger, skrah
Priority: critical Keywords: needs review, patch

Created on 2011-02-08 10:46 by cartman, last changed 2011-12-08 21:31 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
clang-fwrapv.diff cartman, 2011-02-08 10:46 Enable -fwrapv for clang review
Messages (23)
msg128170 - (view) Author: Ismail Donmez (cartman) Date: 2011-02-08 10:46
clang (as with gcc 4.x) assumes signed integer overflow is undefined. But Python depends on the fact that signed integer overflow wraps. 

Currently configure script adds -fwrapv flag for GCC (added by me back in the day), same should be done with clang.

Patch attached.
msg128171 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-02-08 10:48
You should maybe add a test into _testcapi for this issue.
msg128172 - (view) Author: Ismail Donmez (cartman) Date: 2011-02-08 10:49
Currently test_overflow inside test_list fails without this patch. Is that good enough or should we create a specific test?
msg128174 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-02-08 12:11
Ah, there is already a test for that: ok, it's fine and enough.
msg128183 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-02-08 18:52
I am using LLVM/Clang 2.8 and test_list passes fine for me as-is. Does it only fail under 32 or 64-bit conditions? Configure compiles under 32-bits for me even though I have -arch x86_64 set.
msg128184 - (view) Author: Ismail Donmez (cartman) Date: 2011-02-08 18:55
This is only reproducable with clang 2.9 trunk builds due to new optimizations. Also note that clang commits confirm that -fwrapv must be provided if you assume signed integer overflow wraps. This was also the case with a spec test.

I tested this on OSX 10.6.6 with x86-64 build.
msg128379 - (view) Author: Ismail Donmez (cartman) Date: 2011-02-11 12:40
I think the patch is good to go, any comments/questions ?
msg128443 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-12 11:53
This is a potential crasher.
It would also be nice if you listed those cases where Python assumes signed overflow behaviour. Can you open a separate issue for that?
msg128519 - (view) Author: Ismail Donmez (cartman) Date: 2011-02-13 20:33
@Antoine Pitrou , that is tracked by http://bugs.python.org/issue1621
msg131540 - (view) Author: Ismail Donmez (cartman) Date: 2011-03-20 20:47
Whats holding this up?
msg131544 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2011-03-20 21:10
Someone having the time to do a patch review.
msg135235 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2011-05-05 19:14
Brett, can you test this?
msg136575 - (view) Author: Ismail Donmez (cartman) Date: 2011-05-22 21:35
Can someone please review this patch? test_list fails with clang without this patch.
msg143991 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-09-13 22:53
Recent clang and Python2.7 (without the patch):

Python 2.7.2+ (2.7:e8d8eb9e05fd, Sep 14 2011, 00:35:51) 
[GCC 4.2.1 Compatible Clang 3.0 (trunk 139637)] on freebsd8
Type "help", "copyright", "credits" or "license" for more information.
>>> 2**63
-9223372036854775808
>>> 2**64
0
>>> 


The patch is fine and I'm going to commit it tomorrow if there are no
objections.
msg143993 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-09-13 23:12
"Recent clang and Python2.7 (without the patch):

>>> 2**64
0"

Does the test suite catch this bug?
msg144012 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2011-09-14 07:32
>>> 2**64
0

Urk!  I'd call that a release blocker.
msg144025 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-09-14 14:15
> Does the test suite catch this bug?

I think all of those fail due to the bug in pow():

20 tests failed:
    test_array test_builtin test_bytes test_decimal test_float
    test_fractions test_getargs2 test_index test_int test_itertools
    test_list test_long test_long_future test_math test_random test_re
    test_strtod test_tokenize test_types test_xrange
msg144026 - (view) Author: Roundup Robot (python-dev) Date: 2011-09-14 14:17
New changeset 0f1e8c246a7b by Stefan Krah in branch '3.2':
Issue #11149: recent versions of clang require the -fwrapv flag.
http://hg.python.org/cpython/rev/0f1e8c246a7b

New changeset 637c67b34a1a by Stefan Krah in branch 'default':
Merge fix for issue #11149.
http://hg.python.org/cpython/rev/637c67b34a1a

New changeset feed6d2097b1 by Stefan Krah in branch '2.7':
Backport fix for issue #11149.
http://hg.python.org/cpython/rev/feed6d2097b1
msg144046 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-09-14 19:50
The new issue #12980 may be a regression introduced by this issue.
msg144048 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-09-14 19:55
No, that's me playing around. I tried to use clang as the compiler
for the build slave. I can't figure out yet why the segfaults occur.

When I'm running the test manually, everything seems to work.
msg144070 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-09-15 09:18
The buildbots are fine, though I think that in this instance
Gentoo-Non-Debug-3.x is the only bot that actually exercises
the new code path.

So I tested manually on FreeBSD/clang-3.0 and I don't see
anything surprising.
msg148088 - (view) Author: Joshua Root (jmr) Date: 2011-11-21 20:55
The fix that was committed doesn't work if CC is a full path like /usr/bin/clang.
msg149055 - (view) Author: Roundup Robot (python-dev) Date: 2011-12-08 21:31
New changeset 7efad6256e58 by Stefan Krah in branch '3.2':
Issue #11149: Also enable -fwrapv if $CC is a full path
http://hg.python.org/cpython/rev/7efad6256e58

New changeset e48df59af394 by Stefan Krah in branch 'default':
Merge second fix for issue #11149.
http://hg.python.org/cpython/rev/e48df59af394

New changeset 9d329adbbb01 by Stefan Krah in branch '2.7':
Backport second fix for issue #11149.
http://hg.python.org/cpython/rev/9d329adbbb01
History
Date User Action Args
2011-12-08 21:31:16python-devsetmessages: + msg149055
2011-11-21 20:55:24jmrsetnosy: + jmr
messages: + msg148088
2011-09-29 22:31:13ned.deilylinkissue13061 superseder
2011-09-15 09:18:26skrahsetstatus: open -> closed
resolution: fixed
messages: + msg144070

stage: patch review -> resolved
2011-09-14 19:55:46skrahsetmessages: + msg144048
2011-09-14 19:50:21hayposetmessages: + msg144046
2011-09-14 14:17:35python-devsetnosy: + python-dev
messages: + msg144026
2011-09-14 14:15:30skrahsetmessages: + msg144025
2011-09-14 07:32:25mark.dickinsonsetmessages: + msg144012
2011-09-14 01:29:45jceasetnosy: + jcea
2011-09-13 23:12:07hayposetmessages: + msg143993
2011-09-13 22:53:28skrahsetpriority: high -> critical

messages: + msg143991
2011-09-13 22:47:22skrahsetnosy: + skrah, adam@NetBSD.org
2011-09-13 22:46:53skrahlinkissue12973 superseder
2011-05-22 21:35:43cartmansetmessages: + msg136575
2011-05-05 19:14:13rhettingersetassignee: brett.cannon

messages: + msg135235
nosy: + rhettinger
2011-03-20 21:10:24brett.cannonsetkeywords: + needs review
messages: + msg131544
nosy: brett.cannon, mark.dickinson, pitrou, haypo, cartman
stage: patch review
2011-03-20 20:47:03cartmansetnosy: brett.cannon, mark.dickinson, pitrou, haypo, cartman
messages: + msg131540
2011-02-13 20:33:57cartmansetnosy: brett.cannon, mark.dickinson, pitrou, haypo, cartman
messages: + msg128519
2011-02-12 11:53:45pitrousetpriority: normal -> high
versions: + Python 2.7, Python 3.3
nosy: + mark.dickinson, pitrou

messages: + msg128443
2011-02-11 12:40:05cartmansetmessages: + msg128379
2011-02-08 18:55:09cartmansetmessages: + msg128184
2011-02-08 18:52:16brett.cannonsetnosy: + brett.cannon
messages: + msg128183
2011-02-08 12:11:24hayposetmessages: + msg128174
2011-02-08 10:49:54cartmansetmessages: + msg128172
2011-02-08 10:48:29hayposetnosy: + haypo
messages: + msg128171
2011-02-08 10:46:46cartmancreate