1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

Cacus could/should provide M_PI

Issue #1377 closed
Steven R. Brandt created an issue

GRHydro_Bondi.c and GRHydro_BondiM.c use M_PI. This was a problem on Tianhe-1A. I'd suggest adding the following to both files:

#ifndef M_PI #define M_PI 3.141592653589793 #endif

Keyword: GRHydro
Keyword: M_PI

Comments (17)

  1. Roland Haas
    • removed comment

    Instead of adding M_PI to out source files, it might be better try and get the BSD (or GNU which includes them) extensions which define M_PI. The required define seems to be be -D_BSD_SOURCE . Can you try this?

  2. Ian Hinder
    • removed comment

    It would be good to have a page on the wiki which describes policies/advice such as this. Do we have one?

    I'm sure we have discussed this before, but these things tend to get lost. Alternatives would include _GNU_SOURCE and using -std=gnu99. Additionally, instead of defining the macro in CFLAGS, it could be defined at the stop of the source file that is being compiled. We could also consider defining it in one of the Cactus header files, if the use of this macro is something that Cactus officially supports.

  3. Frank Löffler

    That fixed it (-U__STRICT_ANSI__). However, M_PI is an extension of the C standard only and we should be sure we want to require it. Downgrading because of it, but why not include these lines - or maybe have Cactus provide it?

  6. Frank Löffler
    • removed comment

    If Cactus required POSIX, it should check that this is the case and abort with an informative error. If it doesn't require it, then thorns cannot and should not rely on POSIX being available and should only assume ANSI. Both should be available essentially everywhere, usually using compiler flags - but it is easy to get this wrong and then you are currently presented with some more or less random file not compiling.

    At some point Cactus started to require C99. Maybe we could think about how feasible it would be to require POSIX as well?

  7. Ian Hinder
    • removed comment

    Confusingly, on another page in the "Mathematics" section of the GNU C Library manual, it is more specific:

    https://www.gnu.org/software/libc/manual/html_node/Trig-Functions.html:

    The math library normally defines M_PI to a double approximation of pi. If strict ISO and/or POSIX compliance are requested this constant is not defined, but you can easily define it yourself:

    So M_PI is not a part of POSIX, and requiring POSIX would not solve this problem. Apparently, M_PI was removed in C99; it was present in Unix98.

    According to http://stackoverflow.com/questions/5007925/using-m-pi-with-c89-standard, the only way to get M_PI is to use compiler-specific non-standard methods. Given this, we have several options:

    1. The user defines _GNU_SOURCE or _BSD_SOURCE in their compilation flags, or in some other way arranges that M_PI is available.
    2. The thorn writer sets these macros in the source file that needs M_PI, or defines M_PI in their source files.
    3. Cactus arranges that M_PI is available by using either of the above methods or defining M_PI in the same way as the GNU C Library if it is not defined already.
    4. Cactus defines CCTK_PI in the same way as the GNU C Library.

    I prefer 3, as it uses a standard name that many people expect. The only case where this might cause a problem is if somebody is trying to import some external code to a Cactus thorn, and this thorn expects that there is no M_PI defined, and uses it itself. I think this is very unlikely.

    On a related note, I have created a page https://docs.einsteintoolkit.org/et-docs/Compiling_the_Einstein_Toolkit where we can start to collect policies and knowledge about compilation flags and optionlists. Erik, perhaps you could run through and correct/extend it?

  8. Erik Schnetter
    • removed comment

    Defining _BSD_SOURCE in a header file doesn't work because we would have to be sure that this header file is always included before math.h.

    This is not only about M_PI -- there are several other constants and several functions outside the C standard that we use in thorns. The POSIX standard http://en.wikipedia.org/wiki/POSIX is widely accepted, and is available everywhere (even Windows is POSIX compatible). I don't know why that page implies that M_PI is not defined by POSIX -- it is. But then, there are several releases of the POSIX standard.

    Another issue is that we follow the C99 standard in Cactus, not C89 (this also requires some compiler flags), with the exception of trigraphs (no one likes them, although they are in the C standard). Cactus works find out of the box on most common systems. All supercomputers are special and often require a large set of flags to make things work, and I'm sure that Tianhe is not different. What compiler are you using there? There are fewer compilers than system architectures, and we are probably using the same compiler already on some other system, so you should be able to copy the options we use there.

    You are right; Cactus should ensure that M_PI is defined. This should be done via autoconf. This could at least be used to generate an error message with an explanation if M_PI is not present.

    Finally, thorn writers can also use C++ or OpenCL, which do define this constant...

  9. Steven R. Brandt reporter
    • removed comment

    I am using icc/11.1. All other issues aside, it should be simple and mostly harmless to define M_PI. Maybe we could put it in cctk_Constants.h.

  10. Erik Schnetter
    • removed comment

    Defining M_PI would lead to a failure in this code:

    #include <cctk_Constants.h>   // defines M_PI
#include <math.h>   // error -- M_PI is already defined

    The C standard does not allow defining constants that start with "M_"; thus in this example, Cactus is at fault for violating the C standard.

    Can we close this as "wontfix"?

  11. Roland Haas
    • removed comment

    If we want to provide M_PI (as per comment:12), we can try and test for M_PI in autoconf, then if M_PI is defined define HAVE_M_PI and only define M_PI in cctk_Constants.h if HAVE_M_PI is not defined. This is the same logic we use to re-define the reserved word "restrict" (and possibly "inline", though I don't quite remember at this point) and may work fine in this case as well.

    Or we can state that Cactus requires POSIX and current POSIX versions don't require M_PI. This may lead to users defining there own M_PI, or use C++ or GSL which do define M_PI. A uniform, Cactus wide solution seems preferable.

  12. Erik Schnetter
    • removed comment

    The case for restrict and inline is slightly different. There, we disable a feature that may not be provided by the compiler, and since this is a keyword, the user has no way of working around this.

    M_PI, on the other hand, is provided on all systems. It is likely also available by default on all systems. However, some compiler options make it unavailable. (There may also be compilers that require an option to enable M_PI, but I don't know of any of those.) In particular, GCC, Clang, Intel all provide M_PI by default.

    The task here is thus: - provide sane option lists in Simfactory that don't disable M_PI - warn people if they accidentally disable M_PI with their compiler options

    By the way, the current POSIX version does require M_PI: http://pubs.opengroup.org/onlinepubs/9699919799/. (This is POSIX 2008; same for POSIX 2004.)

    I think that the presence of M_PI is not the actual problem, it is just the first problem that is encountered when compiler options are wrong in this way. Defining M_PI will allow the build process to continue a bit further, but may err out later with more POSIX requirements (M_2PI? M_SQRT2?).

    If we want to provide a Cactus-wide solution, then it would need to be CCTK_PI.

  13. Roland Haas
    • removed comment

    We still require some solution for this. M_PI may not be so annoying since it is at least a compile time failure. However (see the Cactus mailing list), we also require strdup() which does compile (and is implicitly defined to return int) even when the proper prototype is missing, which then leads to run time failures (segfault, funny strings, but not all the time).

  18. Log in to comment
Assignee
–
Type
enhancement
Priority
minor
Status
closed
Component
Cactus
Milestone
–
Version
development version
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!