[Einstein Toolkit] #1377: GRHydro_Bondi.c and GRHydro_BondiM.c use M_PI

List overview All Threads
Download

newer

older

[Einstein Toolkit] #1789:...

[Einstein Toolkit] #1785:...

Einstein Toolkit

6 Jun 2013 6 Jun '13

12:22 a.m.

#1377: GRHydro_Bondi.c and GRHydro_BondiM.c use M_PI -----------------------------------+---------------------------------------- Reporter: sbrandt | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Keywords: GRHydro M_PI | -----------------------------------+---------------------------------------- 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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Show replies by date

Einstein Toolkit

6 Jun 6 Jun

12:41 a.m.

#1377: GRHydro_Bondi.c and GRHydro_BondiM.c use M_PI ------------------------------------+--------------------------------------- Reporter: sbrandt | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: GRHydro M_PI ------------------------------------+---------------------------------------

Comment (by rhaas):

Instead of adding M_PI to out source files, we tend to make to get the BSD (or GNU which includes them) extensions which define M_PI. The required define tends to be -D_BSD_SOURCE . Can you try this?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:1 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:11 a.m.

Comment (by hinder):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:2 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:16 a.m.

#1377: GRHydro_Bondi.c and GRHydro_BondiM.c use M_PI --------------------------+------------------------------------------------- Reporter: sbrandt | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: GRHydro M_PI --------------------------+------------------------------------------------- Changes (by knarf):

* type: defect => enhancement * component: EinsteinToolkit thorn => Cactus

Comment:

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:3 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:16 a.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI (was: GRHydro_Bondi.c and GRHydro_BondiM.c use M_PI)

#1377: Cacus could/should provide M_PI --------------------------+------------------------------------------------- Reporter: sbrandt | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: GRHydro M_PI --------------------------+-------------------------------------------------

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:4 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:29 a.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by rhaas):

Does Cactus require POSIX (not sure which version) or Unix98? Both would provide us with M_PI (according to glibc). See: https://www.gnu.org/software/libc/manual/html_node/Mathematical- Constants.html Also, Ian is right, this does seem to come up periodically, see http://lists.einsteintoolkit.org/pipermail/trac/2010-November/000131.html

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:5 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:22 a.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by knarf):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:6 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

3:13 a.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by hinder):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:7 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

7:39 a.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by eschnett):

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...

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:8 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

9 Jun 9 Jun

8:45 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by sbrandt):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:9 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

19 May 19 May

1:18 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by eschnett):

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"?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:10 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:07 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by rhaas):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:11 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:47 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by eschnett):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:12 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

6 Mar 6 Mar

12:50 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by rhaas):

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).

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:13 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:01 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

Comment (by rhaas):

Commits in

https://bitbucket.org/cactuscode/cactus/branch/rhaas%2Fcheck-for-PI-and- strdup

check for M_PI.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:14 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

2:05 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

#1377: Cacus could/should provide M_PI --------------------------+------------------------------------------------- Reporter: sbrandt | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: GRHydro M_PI --------------------------+------------------------------------------------- Changes (by rhaas):

* status: new => review

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:15 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

16 Jun 16 Jun

4:10 p.m.

New subject: [Einstein Toolkit] #1377: Cacus could/should provide M_PI

#1377: Cacus could/should provide M_PI --------------------------+------------------------------------------------- Reporter: sbrandt | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: fixed | Keywords: GRHydro M_PI --------------------------+------------------------------------------------- Changes (by eschnett):

* status: review => closed * resolution: => fixed

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/1377#comment:16 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

4133

Age (days ago)

4873

Last active (days ago)

trac@lists.einsteintoolkit.org

16 comments

1 participants

tags (0)

participants (1)

Einstein Toolkit