bugGNU Octave - Bugs: bug #61812, Math constants (e.g. M_PI) are not...

 
 

bug #61812: Math constants (e.g. M_PI) are not part of C/C++ standard

Submitted by:  Markus Mützel <mmuetzel>
Submitted on:  Thu 13 Jan 2022 02:40:23 PM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Thu 27 Jan 2022 11:44:12 AM UTC, comment #39: 

Looking at the code in ax_cxx_compile_stdcxx.m4 in more detail, it looks like it doesn't check for GNU extensions at all.
I misunderstood the documentation of that macro. We should probably just remove the additional checks I wrote about before.
Done here:
https://hg.savannah.gnu.org/hgweb/octave/rev/3ca33008c4a9

Markus Mützel <mmuetzel>
Project Member
Thu 27 Jan 2022 11:05:53 AM UTC, comment #38: 

From GCC's documentation:
https://gcc.gnu.org/onlinedocs/gcc/Standards.html

> By default, GCC also provides some additional extensions to the C++ language that on rare occasions conflict with the C++ standard. See Options Controlling C++ Dialect. Use of the -std options listed above disables these extensions where they they conflict with the C++ standard version selected. You may also select an extended version of the C++ language explicitly with -std=gnu++98 (for C++98 with GNU extensions), or -std=gnu++11 (for C++11 with GNU extensions), or -std=gnu++14 (for C++14 with GNU extensions), or -std=gnu++17 (for C++17 with GNU extensions), or -std=gnu++20 (for C++20 with GNU extensions).
>
> The default, if no C++ language dialect options are given, is -std=gnu++17.


So, I would have thought no flags would be necessary to get the full set of features for C++11 with GNU extensions with a modern GCC.
For some reason, the macro determines that it does need setting -std=gnu++11 to get what is requested...

Markus Mützel <mmuetzel>
Project Member
Thu 27 Jan 2022 10:52:46 AM UTC, comment #37: 

@arb: Before the patch, AX_CXX_COMPILE_STDCXX was called without setting any preference when it comes to GNU extensions.
After the patch, that macro is used to first check for the flags to enable GNU extensions and only compile without them if that check fails.
What I was trying to write was that we might not need the check for GNU extensions.
If we removed them, the flags would probably be back to what they were before the patch with a modern compiler.
For an older compiler, -std=c++17 might be added if configured with --enable-std-pmr-polymorphic-allocator. For even older compilers, -std=c++11 might be added to the compiler flags. (Or whatever flags the compiler vendor requires to enable support for C++11.)

Markus Mützel <mmuetzel>
Project Member
Thu 27 Jan 2022 10:45:00 AM UTC, comment #36: 

I think the issue might be that after the patch with a compiler supporting c++17 with --enable-std-pmr-polymorphic-allocator an explicit -std=gnu++17 is supplied to the compiler, and with --disable-std-pmr-polymorphic-allocator an explicit -std=gnu+11 (disabling std::variant), whereas before this patch I suspect the -std option was not specified (and is default c++17 for gcc 11.2, allowing std::variant to be used).

A.R. Burgers <arb>
Thu 27 Jan 2022 08:36:45 AM UTC, comment #35: 

Re comment #32: I just meant that the style would be more appropriate for C++ as opposed to C.

Markus Mützel <mmuetzel>
Project Member
Thu 27 Jan 2022 08:35:07 AM UTC, comment #34: 

@arb: Hmmm. I would have thought that that macro would prefer no flags to activate the desired feature set over any flags. I.e., I would have expected that no flags would be added for a modern GCC compiler for C++11 with GNU extensions.
I might have either misunderstood what that macro does, or GCC doesn't use GNU extensions by default.
Maybe we should remove those parts of the check and just go with whatever the compiler prefers when it comes to GNU extensions...

Setting HAVE_CXX17=0 is no signal to the compiler to disable support for C++17. It is just a means to have simpler conditions in the following tests.

Markus Mützel <mmuetzel>
Project Member
Thu 27 Jan 2022 08:03:11 AM UTC, comment #33: 

The changeset from https://hg.savannah.gnu.org/hgweb/octave/rev/9c98df2035a3 disables C++17 features (setting HAVE_CXX17=0) unless --enable-std-pmr-polymorphic-allocator is enabled. For instance the std::variant trials from bug #61800, using gcc 11.2, now only compiles with --enable-std-pmr-polymorphic-allocator, where as previously it compiled independently from the polymorphic allocator. Not a big deal, but a heads-up.

A.R. Burgers <arb>
Wed 26 Jan 2022 08:24:11 PM UTC, comment #32: 

What is the C++-y variant in comment #30? Is that C++20 or C++23?

Anonymous
Wed 26 Jan 2022 05:14:14 PM UTC, comment #31: 

If we are planning to use C++20 features, we should definitely check for compiler switches that enable support for that. GNU's ax_cxx_compile_stdcxx macro currently only checks for feature set versions up to C++17.
Is there something we could take for that test?

In the next step, we could manually add checks for specific features we'd like to use. (Similarly to what we already do for C++17 polymorphic allocators.)

Preferably, that would not make the API dependent on the C++ standard version. That means we can't rely on results during configure of Octave being valid when our headers are included in downstream projects.
This might get tricky...

Markus Mützel <mmuetzel>
Project Member
Wed 26 Jan 2022 04:43:56 PM UTC, comment #30: 

Pushed a modified version of the patch from comment #25 here:
https://hg.savannah.gnu.org/hgweb/octave/rev/9c98df2035a3

I believe we needed that change in any case.

I agree that it could be the way forward to adapt a more C++-y style of using those constant.
It looks like they are constexpr. So, that might also be something we could use.

Markus Mützel <mmuetzel>
Project Member
Tue 25 Jan 2022 06:43:19 PM UTC, comment #29: 

@jwe: No problem adopting a more C++ way of doing things.  It seems to me that any fix is starting to grow to a mildly large patch at this point.

First there needs to be configure.ac tests for the constants we are using.  Some compilers will not be C++20 compatible so std::numbers won't exist.  That means we should provide our own replacements.  Maybe the best solution is an octave::numbers class which maps on to std::numbers when it exists or a replacement value if it does not?

Rik <rik5>
Project Administrator
Tue 25 Jan 2022 05:56:06 PM UTC, comment #28: 

Instead of using C constants and a gnulib module (which is mostly about C, not C++ anyway) could we check for the existence of <numbers> and provide definitions for what we need if it is missing?  Then I think we should be able to use

std::numbers::pi

and so on where we need them.  Our replacement for <numbers> might not be 100% compatible with what C++20 provides but I don't see that we need that.  All we really need is a way to access these constants in a reasonably efficient way and using the future C++ names seems better to me than using macros like M_PI.

John W. Eaton <jwe>
Project Administrator
Tue 25 Jan 2022 04:05:03 PM UTC, comment #27: 

Afaict, there is no gnulib module that would convert a compiler that doesn't support GNU extensions to one that does. Their math module adds support for some macros and functions for some platforms. But afaict, none of the XSI macros.

However, using GNU extensions doesn't mean that we would be limited to gcc. IIUC, many other compilers from different vendors support GNU extensions, too. ISTR that clang aims to be source compatible to GCC. Another example are IBM's compilers:
https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.1?topic=both-extensions-gnu-compatibility

IIUC, the math headers are standard library specific (not compiler specific). That might be different for other GNU extensions.

I didn't fully understand it yet, but it might be that compiling with `-std=gnu++11` (as opposed to `-std=c++11`) sets "_XOPEN_SOURCE 700". So, it's no longer necessary to set it explicitly in the sources. (But I might be wrong on this.)

Anyway, I'll probably remove the FIXME notes and push the patch from comment #25 if no one opposes. With it the compiler features are checked in the following order:

  • If configuring with `--enable-std-pmr-polymorphic-allocator`, check if the compiler supports C++17 with GNU extensions.
  • If that fails, check if the compiler supports C++17 without GNU extensions.
  • If that fails or configuring without `--enable-std-pmr-polymorphic-allocator`, check if the compiler supports C++11 with GNU extensions.
  • If that fails, check if the compiler supports C++11 without GNU extensions.
  • If that fails, terminate configure with an error.
Markus Mützel <mmuetzel>
Project Member
Mon 24 Jan 2022 09:36:00 PM UTC, comment #26: 

I would generally like to be able to count on M_PI, M_E and other GNU extensions being defined.  Is there a way to do that through gnulib such that if the OS-provided header file doesn't include the definitions then the gnulib version which does is used?

However, if that is not possible then I think it would be nicer to define our own constants as a fallback rather than just aborting compilation.  I'm sure there are some weird platforms where maybe g++/gcc aren't ported and it seems mean to try and require them to build g++ from sources.

Rik <rik5>
Project Administrator
Mon 24 Jan 2022 07:05:47 PM UTC, comment #25: 

I'd like to push the attached patch to the stable branch. It checks for compiler options that enable GNU extensions and prefers them over not using GNU extensions.
Is that the right thing? Do we even support compiling without GNU extensions? Should we fail early on during configure if they aren't available?

(file #52719)

Markus Mützel <mmuetzel>
Project Member
Mon 24 Jan 2022 04:00:38 PM UTC, comment #24: 

Like suggested by jwe in comment #21, I moved the implementation of the templates to a .cc file to avoid using non-ISO C/C++ in a public header file:
https://hg.savannah.gnu.org/hgweb/octave/rev/965ead41658b

Followed up by a change to reduce some duplicate code:
https://hg.savannah.gnu.org/hgweb/octave/rev/20b05c5c41a1

The last paragraph of comment #23 still stands.

Markus Mützel <mmuetzel>
Project Member
Mon 17 Jan 2022 08:28:18 AM UTC, comment #23: 

I don't know how to check for all of the XSI/POSIX extensions over ISO C/C++ that we might be using.

Afaict, the only header that uses any of those extensions from the <math.h> or <cmath> header is `oct-cmplx.h`.

Removing those extensions from the public facing headers would also be a possible alternative IIUC. In that case, projects using Octave header files wouldn't necessarily need to be XSI compatible either.

It might still be a good idea to `#define _XOPEN_SOURCE 700` when we compile Octave (to comply to the POSIX spec). Maybe that could be done in `oct-conf-post-private.h` on the default branch?

Markus Mützel <mmuetzel>
Project Member
Mon 17 Jan 2022 04:27:00 AM UTC, comment #22: 

@jwe: There could be other constants besides M_PI.  In data.cc, for example, wee use M_E.

#if defined (M_E)
  double e_val = M_E;
#else
  double e_val = exp (1.0);
#endif

In actual header files, though, only oct-cmplx.h uses a 'M_' constant and that constant is M_PI.

Rik <rik5>
Project Administrator
Fri 14 Jan 2022 05:50:22 PM UTC, comment #21: 

The only header that uses M_PI is oct-cmplx.h.  It is used in some template function definitions but maybe there is some way we could change that code so M_PI would not be needed in the header file?  As far as I can tell, we only care about std::complex<float> and std::complex<double> so it seems the header file could just provide the necessary extern template declarations for the specializations that we need?

Is M_PI the only defined constant we use or are there others that may also appear in public header files?

John W. Eaton <jwe>
Project Administrator
Fri 14 Jan 2022 04:01:15 PM UTC, comment #20: 

IIUC, projects including Octave headers always(?) needed to be XSI compatible anyway. (We are using things like M_PI in public headers after all.) That macro would just make that fact "more official"...

Markus Mützel <mmuetzel>
Project Member
Fri 14 Jan 2022 03:57:28 PM UTC, comment #19: 

I'm assuming we are getting this right already when we are compiling Octave. Having this "more in line" with the POSIX specs would just be a positive side effect.
My main intent is to avoid compilation errors when public headers are included in user code (or Octave packages).
So, I was planning on modifying octave-config.h (or its generating script on stable) and/or probably oct-conf-post-public.in.h (on default).

Markus Mützel <mmuetzel>
Project Member
Fri 14 Jan 2022 03:51:24 PM UTC, comment #18: 

Is your intent to define _XOPEN_SOURCE in the public octave-config.h header file so that we are defining that for all users of the Octave libraries (we tell people to include octave-config.h in their .oct file sources, for example) or is this something we should only do for the Octave source files using Octave's config.h file, probably by defining something in the oct-conf-post.in.h file (stable) or oct-conf-post-public.in.h (default)?

John W. Eaton <jwe>
Project Administrator
Fri 14 Jan 2022 03:47:16 PM UTC, comment #17: 

I'm debating if we should `#define _XOPEN_SOURCE 700` also on the stable branch. (I'm leaning towards yes.) Or do you think that change would be too intrusive in the current phase?

The other part of the change (i.e., possibly removing all manually defined M_PI) should probably go on the default branch only...

Markus Mützel <mmuetzel>
Project Member
Fri 14 Jan 2022 03:27:59 PM UTC, comment #16: 

@Rik: I searched for `octave-config.h` in all headers. In most of these, the match was in line 29. I assumed that all of those have the default license header and are including that header first thing after the inclusion guard. I manually checked the remaining headers (where the match was in a different line).

Markus Mützel <mmuetzel>
Project Member
Fri 14 Jan 2022 03:23:16 PM UTC, comment #15: 

@Markus: Were you able to use `grep` or any other means to check that these are the only two occurrences in the Octave codebase of reversed #include ordering?

Rik <rik5>
Project Administrator
Fri 14 Jan 2022 02:51:13 PM UTC, comment #14: 

Pushed a patch here that moves "octave-config.h" to the top where it is included:
http://hg.savannah.gnu.org/hgweb/octave/rev/b7df783dbe43

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 06:32:33 PM UTC, comment #13: 

In header files, we should always be including "octave-config.h" first, but only if it is really needed for some specific purpose.  I don't think it needs to be included in every .h file in Octave.

All .cc files should begin with

#if defined (HAVE_CONFIG_H)
#  include "config.h"
#endif

John W. Eaton <jwe>
Project Administrator
Thu 13 Jan 2022 06:03:51 PM UTC, comment #12: 

I know with MACOS libraries we have had problems with just slightly different values for certain trig calculations.  By using an expression like "4.0*atan(1.0)" you get a value that is at least self-consistent with the libraries being used.  It also gets around cross-compilation issues where M_PI on the build system might be different than M_PI on the host system.  But, that would be a really bad idea if build and host were different to begin with.

In any case, I think we can just get rid of it if we can guarantee that M_PI is defined.

@markus: According to our guidelines, "octave-config.h" should always be the first header file included.  Next come C and C++ header files, then liboctave header files, then libinterp header files.

Rik <rik5>
Project Administrator
Thu 13 Jan 2022 05:49:45 PM UTC, comment #11: 

In lo-ieee.h, <cmath> is included before "octave-config.h". That should probably be changed anyway (and any other files that include headers before "octave-config.h"). Or is there a good reason why "octave-config.h" is included after other headers in some files?

@jwe: I searched some forums (stackoverflow and alike) for this topic. Some users are commenting that "unforeseeable things" might occur if pi is defined differently in different places. `4.0*atan(1.0)` seems to be used widely to get the exact PI that is used by the library...

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 05:39:53 PM UTC, comment #10: 

At least if it defined as "static const" then it should only be computed once (per translation unit) when Octave starts.  But then every file that needs it would have a separate (static) value.

What makes 4.0*atan(1.0) any better than just defining M_PI with a literal constant if it is not defined by including <cmath>?

John W. Eaton <jwe>
Project Administrator
Thu 13 Jan 2022 05:32:54 PM UTC, comment #9: 

I agree that it is a base assumption of our codebase that we are including GNU extensions so we should make sure that it true at a global level.  Including it in "octave-config.h" seems like a good plan.

Rik <rik5>
Project Administrator
Thu 13 Jan 2022 05:29:17 PM UTC, comment #8: 

Re-reading the POSIX specs:

> An XSI-conforming application shall ensure that the feature test macro _XOPEN_SOURCE is defined with the value 700 before inclusion of any header. This is needed to enable the functionality described in The _POSIX_C_SOURCE Feature Test Macro and to ensure that the XSI option is enabled.


I now think that we should define that macro in our "octave-config.h". Not just in those headers that include <cmath>...

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 05:16:36 PM UTC, comment #7: 

I was mainly concerned about public headers.

I didn't check our build system. But ideally we'd check that Octave is compiled with GNU extensions (something like `-std=gnu++11` if not enabled by default)...

Afaics, `atan` is not `constexpr`. See:
https://en.cppreference.com/w/cpp/header/cmath

I think it is a bug in GCC that it accepts that (invalid) syntax. I believe I came across this before and it is (rightfully) failing with clang++. (But I might be misremembering...)

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 05:10:39 PM UTC, comment #6: 

> `atan` is not `constexpr`. But we could make that a `const double` at least.


I think it is.  I changed the code to use `constexpr` and it compiles correctly.  I believe the compiler can calculate atan at compile time by using the build system's libraries and then it inserts the correct value.

But, if you are going to handle it by using "#define" then we should define _XOPEN_SOURCE in data.cc so that we can always rely on having M_PI and then we can remove the #ifdef/#else construct entirely.

Using grep, it seems like M_PI is used more widely than just graphics.cc.  The list is

corefcn/data.cc
corefcn/gl-render.cc
corefcn/graphics.cc
corefcn/quadcc.cc
util/oct-cmplx.h
numeric/lo-mappers.cc
numeric/lo-specfun.cc
numeric/oct-norm.cc
numeric/randpoisson.cc

Rik <rik5>
Project Administrator
Thu 13 Jan 2022 04:36:25 PM UTC, comment #5: 

See the attached patch.

Currently only tested with an otherwise modified Octave. So, I can't tell atm how it will behave on a "clean" build...

(file #52660)

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 04:23:54 PM UTC, comment #4: 

The correct "appropriate feature test macro" seems to be:

#define _XOPEN_SOURCE 700

See: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_01_02

It looks like this also works on Windows. And it is POSIX conform IIUC.

I'll prepare a patch.

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 04:00:39 PM UTC, comment #3: 

`atan` is not `constexpr`. But we could make that a `const double` at least.

A configure test probably wouldn't suffice. Octave could be (and currently is) built with GNU extensions. In that case, M_PI is defined (also for Windows).
A user or a package build rule could switch to a "strict" dialect e.g. with `mkoctfile -std=c++11 some_file.cc`. In that case, M_PI wouldn't be defined (at least on Windows, maybe also on other platforms).

The POSIX documentation says:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/math.h.html

> Some of the functionality described on this reference page extends the ISO C standard. Applications shall define the appropriate feature test macro (see XSH The Compilation Environment ) to enable the visibility of these symbols in this header.


The math constants on that page are marked with "XSI". So, I guess that applies to them.

Maybe, the "appropriate feature test macro" is `#define _USE_MATH_DEFINES`?
In that case, we could (and should?) define that everywhere where we `#include <cmath>` in a public header.

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 03:47:44 PM UTC, comment #2: 

These constants may not be part of the standard, but they are really widely available.

I don't think we need to try for perfection here.  If the Windows OS has a special exception and doesn't include M_PI then let's just figure out a way to include a definition for M_PI.

Some ideas:

1) Provide a wrapper for the the cmath header in liboctave/wrappers
2) Do a configure test for Windows and define _USE_MATH_DEFINES on that platform
3) Define M_PI ourselves if it does not exist.

On a related note, this code for the pi() function in libinterp/corefcn/data.cc is not the way to do it:

#if defined (M_PI)
  double pi_val = M_PI;
#else
  double pi_val = 4.0 * atan (1.0);
#endif

This requires calculating atan() every single time on platforms, like Windows, which don't have M_PI built-in.  At the very least the second half should be tagged with constexpr (and maybe static too).

Rik <rik5>
Project Administrator
Thu 13 Jan 2022 02:58:44 PM UTC, comment #1: 

This has been an issue since quite some time. Not a regression in Octave 7.

Markus Mützel <mmuetzel>
Project Member
Thu 13 Jan 2022 02:40:23 PM UTC, original submission:  

Math constants are not defined in standard C or C++. They are a GNU/POSIX extension. See: https://en.cppreference.com/w/cpp/header/cmath

At least on Windows, compilation of headers that use any of those math constants fails if a "strict" dialect is selected (e.g. `-std=c++11`).

One header that uses e.g. M_PI is `liboctave/util/oct-cmplx.h`.

C++20 defines `std::numbers::pi` in `<numbers>`:
https://en.cppreference.com/w/cpp/numeric/constants

On Windows, the older C math constants can be forced to be defined by `<cmath>` with `#define _USE_MATH_DEFINES` prior to including this header. That also works if a "strict" C++ dialect is selected.

Should we do that? Or avoid using these non-standard constants in headers entirely? Which alternatives are there?

Markus Mützel <mmuetzel>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arb (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-24 mmuetzel Attached File- => Added bug61841-gnu-extensions.patch, #52719
    2022-01-13 mmuetzel Attached File- => Added bug61812-cmath-posix.patch, #52660
    2022-01-13 mmuetzel StatusNone => In Progress
    2022-01-13 mmuetzel Release7.0.90 => dev

    Back to the top


    Powered by Savane 3.7