Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure to use custom hypot() when building as ANSI C #547

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

kbevers
Copy link
Member

@kbevers kbevers commented Jul 14, 2017

hypot() is not defined in C versions earlier than C99. So when trying to build PROJ.4 as ANSI C warnings are issued.

Below I build with CFLAGS=-g -Wall -Wextra -Werror -Wunused-parameter -Wmissing-prototypes -Wmissing-declarations -Wformat -Werror=format-security -Wshadow -O2 -ansi which results in a failed build because hypot() is unknown. This PR fixes the problem.

λ ninja binproj
[2/176] Building C object src/CMakeFiles/proj.dir/PJ_aeqd.c.obj
FAILED: src/CMakeFiles/proj.dir/PJ_aeqd.c.obj
C:\TDM-GCC-64\bin\gcc.exe -DHAVE_PTHREAD_MUTEX_RECURSIVE=1 -DMUTEX_pthread -DMUTEX_win32 -DPJ_SELFTEST -DPROJ_LIB=\"c:/OSGeo4W/share\" -IC:/dev/proj4/src -Isrc -g -Wall -Wextra -Werror -Wunused-parameter -Wmissing-prototypes -Wmissing-declarations -Wformat -Werror=format-security -Wshadow -O2 -ansi -O3 -DNDEBUG -MD -MT src/CMakeFiles/proj.dir/PJ_aeqd.c.obj -MF src\CMakeFiles\proj.dir\PJ_aeqd.c.obj.d -o src/CMakeFiles/proj.dir/PJ_aeqd.c.obj   -c C:/dev/proj4/src/PJ_aeqd.c
C:/dev/proj4/src/PJ_aeqd.c: In function 'e_inverse':
C:/dev/proj4/src/PJ_aeqd.c:177:14: error: implicit declaration of function 'hypot' [-Werror=implicit-function-declaration]
     if ((c = hypot(xy.x, xy.y)) < EPS10) {
              ^
cc1.exe: all warnings being treated as errors
[3/176] Building C object src/CMakeFiles/proj.dir/PJ_aea.c.obj
FAILED: src/CMakeFiles/proj.dir/PJ_aea.c.obj
C:\TDM-GCC-64\bin\gcc.exe -DHAVE_PTHREAD_MUTEX_RECURSIVE=1 -DMUTEX_pthread -DMUTEX_win32 -DPJ_SELFTEST -DPROJ_LIB=\"c:/OSGeo4W/share\" -IC:/dev/proj4/src -Isrc -g -Wall -Wextra -Werror -Wunused-parameter -Wmissing-prototypes -Wmissing-declarations -Wformat -Werror=format-security -Wshadow -O2 -ansi -O3 -DNDEBUG -MD -MT src/CMakeFiles/proj.dir/PJ_aea.c.obj -MF src\CMakeFiles\proj.dir\PJ_aea.c.obj.d -o src/CMakeFiles/proj.dir/PJ_aea.c.obj   -c C:/dev/proj4/src/PJ_aea.c
C:/dev/proj4/src/PJ_aea.c: In function 'e_inverse':
C:/dev/proj4/src/PJ_aea.c:97:19: error: implicit declaration of function 'hypot' [-Werror=implicit-function-declaration]
     if( (Q->rho = hypot(xy.x, xy.y = Q->rho0 - xy.y)) != 0.0 ) {
                   ^
cc1.exe: all warnings being treated as errors
[7/176] Building C object src/CMakeFiles/proj.dir/PJ_aitoff.c.obj
ninja: build stopped: subcommand failed.

@kbevers kbevers added this to the 4.9.4 milestone Jul 18, 2017
@kbevers kbevers merged commit f0abd52 into OSGeo:master Jul 18, 2017
@ndavid
Copy link
Contributor

ndavid commented Jul 28, 2017

When compiling master with visual studio community 2015 - 64bit there are some warnings concerning this merge request :

2>D:\codes\src\OSGEO\proj.4\src\projects.h(88): warning C4273: 'hypot': inconsistent dll linkage
2> C:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt\math.h(498): note: see previous definition of 'hypot'
2> PJ_aea.c

The preprocessor has WIN32 defined but not ANSI.

Looking at the visual studio Predefined Macros I can't see this ANSI macro. Perhaps it is only defined under gcc/mingw compiler ?

I was thinking as using

CHECK_FUNCTION_EXISTS(hypot HAVE_HYPOT)

inside Proj4Config.cmake as a cleaner way to make this check, but proj_config.h seems to not be used by project.h and an equivalent check should also be added to the autotools build..

So I'm not sure about how to resolve this issue :

  • using cmake and autotools to check if hypot exist and using the proj_config.h file
  • tweaking the preprocessor logic with some _MSC_VER

Could you share some advices ?

@kbevers
Copy link
Member Author

kbevers commented Aug 2, 2017

Nice catch. I was relying on the windows tests to catch this sort of thing. This changed worked for me, but not for the reason I expected, so let's get that fixed.

I just made a few test with the following conditional:

#if !defined(_WIN32) || (defined(__STDC_VERSION) && (__STDC_VERSION__ <= 199409L))

How does that work for you? It seems to work for me building with nmake/visual studio and gcc with the -ansi flag.

@cffk
Copy link
Contributor

cffk commented Aug 2, 2017

I've faced this issue with geodesic.[ch] which includes its own
definitions of atanh, copysign, hypot, and cbrt. Instead of testing
random macros in the source, wouldn't it be better to have cmake and
autoconf check for these. Here's what I'm using for the C version
bundled in GeographicLib; CMakeLists.txt contains

include (CheckCSourceCompiles)
if (NOT MSVC)
  set (CMAKE_REQUIRED_LIBRARIES m)
endif ()
# Check whether the C99 math function: hypot, atanh, etc. are available.
check_c_source_compiles (
  "#include <math.h>
int main() {
  int q;
  return (int)(hypot(3.0, 4.0) + atanh(0.8) + cbrt(8.0) +
               remquo(100.0, 90.0, &q) +
               remainder(100.0, 90.0) + copysign(1.0, -0.0));
}\n" C99_MATH)
if (C99_MATH)
  add_definitions (-DHAVE_C99_MATH=1)
else ()
  add_definitions (-DHAVE_C99_MATH=0)
endif ()

and geodesic.c contains

#if !defined(HAVE_C99_MATH)
#define HAVE_C99_MATH 0
#endif

...

#if HAVE_C99_MATH
#define atanhx atanh
#define copysignx copysign
#define hypotx hypot
#define cbrtx cbrt
#else
static real log1px(real x) {
  volatile real
    y = 1 + x,
    z = y - 1;
  /* Here's the explanation for this magic: y = 1 + z, exactly, and z
   * approx x, thus log(y)/z (which is nearly constant near z = 0) returns
   * a good approximation to the true log(1 + x)/x.  The multiplication x *
   * (log(y)/z) introduces little additional error. */
  return z == 0 ? x : x * log(y) / z;
}

static real atanhx(real x) {
  real y = fabs(x);             /* Enforce odd parity */
  y = log1px(2 * y/(1 - y))/2;
  return x < 0 ? -y : y;
}

static real copysignx(real x, real y) {
  return fabs(x) * (y < 0 || (y == 0 && 1/y < 0) ? -1 : 1);
}

...
#endif

(You might remember that one of the checkers dinged the copysignx
code for its use of 1/y when y = +/-0.)

By the way, in many places in the existing proj.4 code base, atanh
should be used instead of some less accurate expression involving logs.

@kbevers
Copy link
Member Author

kbevers commented Aug 2, 2017

wouldn't it be better to have cmake and
autoconf check for these.

Sure. Would you mind giving it a go? If not I'll revert the change in this PR so the bad code doesn't get in the next release. I am completely swamped the rest of the week and of on vacation for the next three weeks after that.

By the way, in many places in the existing proj.4 code base, atanh
should be used instead of some less accurate expression involving logs.

You should create an issue on it so we will remember to do something about it.

@cffk
Copy link
Contributor

cffk commented Aug 6, 2017

I've created pull request #555. At present this just creates the HAVE_C99_MATH flag and so the proj.4 build isn't changed. However the next release of geodesic.c will test this flag. Also note the issue in the check in message: what to do if HAVE_C99_MATH is 0 (define the missing functions or assume that libm defines them).

@ndavid
Copy link
Contributor

ndavid commented Aug 7, 2017

Just for the feedback the following conditional
#if !defined(_WIN32) || (defined(__STDC_VERSION) && (__STDC_VERSION__ <= 199409L))
work also for me with visual studio 2015.

Thank's @cffk for creating a new pull request, I will look at your proposal.

@kbevers kbevers deleted the hypot-ansi-safe branch October 4, 2017 14:52
@kbevers kbevers modified the milestones: 5.0.0-b, 5.0.0 Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants