classification
Title: #define hypot _hypot conflicts with existing definition
Type: compile error Stage: resolved
Components: Windows Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: Brecht.Van.Lommel, Shy.Shalom, mark.dickinson, python-dev, tabrezm, zach.ware
Priority: normal Keywords: patch

Created on 2014-01-11 03:09 by tabrezm, last changed 2014-10-17 16:12 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
fix20221.patch tabrezm, 2014-01-13 19:07 review
Messages (17)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2014-10-17 16:12
It grafted very easily, so it turns out to be "yes" :)
History
Date User Action Args
2014-10-17 16:12:05zach.waresetmessages: + msg229578
versions: + Python 2.7
2014-10-17 16:11:07python-devsetmessages: + msg229577
2014-10-02 15:32:45Shy.Shalomsetnosy: + Shy.Shalom
messages: + msg228230
2014-03-17 06:31:12python-devsetmessages: + msg213834
2014-02-20 21:42:38zach.waresetstatus: open -> closed
messages: + msg211755

assignee: zach.ware
resolution: fixed
stage: patch review -> resolved
2014-02-20 21:40:19python-devsetnosy: + python-dev
messages: + msg211754
2014-02-20 19:25:30zach.waresetmessages: + msg211746
2014-01-28 20:17:22zach.waresetstage: test needed -> patch review
2014-01-28 20:17:07zach.waresetmessages: + msg209582
2014-01-28 19:08:41Brecht.Van.Lommelsetmessages: + msg209576
2014-01-28 18:32:51zach.waresetmessages: + msg209573
2014-01-28 18:25:47tabrezmsetmessages: + msg209571
2014-01-28 15:39:26zach.waresetmessages: + msg209557
2014-01-23 19:54:49serhiy.storchakasetnosy: + mark.dickinson
2014-01-23 19:52:22Brecht.Van.Lommelsetnosy: + Brecht.Van.Lommel
messages: + msg208981
2014-01-13 19:07:44tabrezmsetfiles: + fix20221.patch
keywords: + patch
2014-01-11 08:18:30tabrezmsetmessages: + msg207902
2014-01-11 07:25:20tabrezmsetmessages: + msg207899
2014-01-11 06:33:18zach.waresetversions: - 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:28tabrezmcreate