msg207894 - (view) |
Author: Tabrez Mohammed (tabrezm) |
Date: 2014-01-11 03:09 |
In pyconfig.h (line 216), there is this line:
#define hypot _hypot
This conflicts with the definition of _hypot that ships with VS2010 (math.h, line 161):
static __inline double __CRTDECL hypot(_In_ double _X, _In_ double _Y)
The result of the redefinition is that the following warning occurs during compilation:
1>c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\include\math.h(162): warning C4211: nonstandard extension used : redefined extern to static
When compiling with warnings being treated as errors, this will obviously result in failed builds.
The recommendation from Microsoft (see http://connect.microsoft.com/VisualStudio/feedback/details/633988/warning-in-math-h-line-162-re-nonstandard-extensions-used) is to change the definition to:
#if _MSC_VER < 1600
#define hypot _hypot
#endif
|
msg207897 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-11 06:33 |
How are you compiling to get that warning? I've never seen it, and none of the Windows buildbots do either. Also, which version of Python are you compiling on which version of Windows?
|
msg207899 - (view) |
Author: Tabrez Mohammed (tabrezm) |
Date: 2014-01-11 07:25 |
v100 toolset, with compiler setting /W4. Microsoft recommends W4 for all new C++ projects (see http://msdn.microsoft.com/en-us/library/thxezb7y(v=vs.100).aspx). I'm using 3.3.
|
msg207902 - (view) |
Author: Tabrez Mohammed (tabrezm) |
Date: 2014-01-11 08:18 |
Sorry, I realize I didn't mention this in the original post. I'm getting this compiler warning when building my Python extension, not when building Python itself.
|
msg208981 - (view) |
Author: Brecht Van Lommel (Brecht.Van.Lommel) |
Date: 2014-01-23 19:52 |
We have a similar issue in Blender (where Python is embedded), but it's actually causing a crash instead of only a compiler warning: https://developer.blender.org/T38256
The code in the Visual Studio 2013 math.h looks like this:
__inline double __CRTDECL hypot(_In_ double _X, _In_ double _Y)
{
return _hypot(_X, _Y);
}
With the #define hypot _hypot that becomes:
__inline double __CRTDECL _hypot(_In_ double _X, _In_ double _Y)
{
return _hypot(_X, _Y);
}
So you get infinite recursive calls when using hypot and the application crashes. The patch fix20221.patch that was attach here solves the issue.
|
msg209557 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-28 15:39 |
I'm having difficulty wrapping my head around why the math and cmath modules (both of which use hypot) compile fine, but your extensions don't. Anyone have any insight into why that is?
|
msg209571 - (view) |
Author: Tabrez Mohammed (tabrezm) |
Date: 2014-01-28 18:25 |
My extension doesn't compile because I treat warnings as errors. I believe Blender compiles fine, but crashes at runtime because of the infinite recursion (see the second code snippet in Brecht's response (http://bugs.python.org/msg208981).
|
msg209573 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-28 18:32 |
Sorry, I wasn't entirely clear. By "compile fine", I meant "compiles without warnings, and actually works when you try to use it". Both math and cmath make use of hypot (as defined in pyconfig.h), but don't have any problems.
They also have no problems with your patch applied, but I don't feel I understand the situation enough yet to move the patch forward.
|
msg209576 - (view) |
Author: Brecht Van Lommel (Brecht.Van.Lommel) |
Date: 2014-01-28 19:08 |
For Visual Studio 2013, here's how to redo the problem. Take this simple program:
#include <Python.h>
int main(int argc, char **argv)
{
return (int)hypot(rand(), rand());
}
And compile it:
cl.exe test.c -I include/python3.3 lib/python33.lib /W4
c:\program files (x86)\microsoft visual studio 12.0\vc\include\math.h(566) : warning C4717: '_hypot' : recursive on all control paths, function will cause runtime stack overflow
I don't know about the conditions to get a warning in VS 2010, never tried that version.
|
msg209582 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-28 20:17 |
Your test program works for VS2010 as well (/W4 is unnecessary, the default warning level gives the warning), but still doesn't answer the question of why the math module (specifically math.hypot) doesn't show the problem.
I understand why both of your cases *don't* work, I want to understand why mathmodule.c and cmathmodule.c (and complexobject.c, for that matter) *do* work. Attempting to compile mathmodule.c alone results in the warning, and even picking through mathmodule.i as produced by preprocessing to file, it looks like math_hypot should always have the problem.
The fact that math_hypot works when compiled with the rest of Python frankly frustrates me quite a lot, because I can see no reason why it should.
|
msg211746 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-02-20 19:25 |
Ok, I had missed that the warnings in your two separate cases were in fact different. I still don't understand why VC++ sees the two cases so differently (throwing different warnings), but it at least explains the different results in your two cases.
I don't see how this change can actually break anything, so I'll go ahead and commit so this makes it into 3.3.5 (and hopefully 3.4.0, but that will be up to Larry Hastings).
|
msg211754 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-02-20 21:40 |
New changeset 9aedb876c2d7 by Zachary Ware in branch '3.3':
Issue #20221: Removed conflicting (or circular) hypot definition
http://hg.python.org/cpython/rev/9aedb876c2d7
New changeset bf413a97f1a9 by Zachary Ware in branch 'default':
Issue #20221: Removed conflicting (or circular) hypot definition
http://hg.python.org/cpython/rev/bf413a97f1a9
|
msg211755 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-02-20 21:42 |
Fixed, thanks for the report and patch!
|
msg213834 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-03-17 06:31 |
New changeset 033d686af4c1 by Zachary Ware in branch '3.4':
Issue #20221: Removed conflicting (or circular) hypot definition
http://hg.python.org/cpython/rev/033d686af4c1
|
msg228230 - (view) |
Author: Shy Shalom (Shy.Shalom) |
Date: 2014-10-02 15:32 |
Any chance this would be merged to 2.7 as well?
The comment from 2014-01-11 06:33:18 says "2.7" but PC/pyconfig.h still has #define hypot _hypot
|
msg229577 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-10-17 16:11 |
New changeset 430aaeaa8087 by Zachary Ware in branch '2.7':
Issue #20221: Removed conflicting (or circular) hypot definition
https://hg.python.org/cpython/rev/430aaeaa8087
|
msg229578 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-10-17 16:12 |
It grafted very easily, so it turns out to be "yes" :)
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:56 | admin | set | github: 64420 |
2014-10-17 16:12:05 | zach.ware | set | messages:
+ msg229578 versions:
+ Python 2.7 |
2014-10-17 16:11:07 | python-dev | set | messages:
+ msg229577 |
2014-10-02 15:32:45 | Shy.Shalom | set | nosy:
+ Shy.Shalom messages:
+ msg228230
|
2014-03-17 06:31:12 | python-dev | set | messages:
+ msg213834 |
2014-02-20 21:42:38 | zach.ware | set | status: open -> closed messages:
+ msg211755
assignee: zach.ware resolution: fixed stage: patch review -> resolved |
2014-02-20 21:40:19 | python-dev | set | nosy:
+ python-dev messages:
+ msg211754
|
2014-02-20 19:25:30 | zach.ware | set | messages:
+ msg211746 |
2014-01-28 20:17:22 | zach.ware | set | stage: test needed -> patch review |
2014-01-28 20:17:07 | zach.ware | set | messages:
+ msg209582 |
2014-01-28 19:08:41 | Brecht.Van.Lommel | set | messages:
+ msg209576 |
2014-01-28 18:32:51 | zach.ware | set | messages:
+ msg209573 |
2014-01-28 18:25:47 | tabrezm | set | messages:
+ msg209571 |
2014-01-28 15:39:26 | zach.ware | set | messages:
+ msg209557 |
2014-01-23 19:54:49 | serhiy.storchaka | set | nosy:
+ mark.dickinson
|
2014-01-23 19:52:22 | Brecht.Van.Lommel | set | nosy:
+ Brecht.Van.Lommel messages:
+ msg208981
|
2014-01-13 19:07:44 | tabrezm | set | files:
+ fix20221.patch keywords:
+ patch |
2014-01-11 08:18:30 | tabrezm | set | messages:
+ msg207902 |
2014-01-11 07:25:20 | tabrezm | set | messages:
+ msg207899 |
2014-01-11 06:33:18 | zach.ware | set | versions:
- Python 3.1, Python 2.7, Python 3.2, Python 3.5 nosy:
+ zach.ware
messages:
+ msg207897
stage: test needed |
2014-01-11 03:09:28 | tabrezm | create | |