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

Add CMake option to enable the debugging of floating point exceptions #3687

Conversation

michaelgregorius
Copy link
Contributor

@michaelgregorius michaelgregorius commented Jul 7, 2017

Add a new CMake option WANT_DEBUG_FPE which adds the define
LMMS_DEBUG_FPE if activated. Extend lmmsconfig.h.in with regards to that
define. Include information about development options to the CMake
output.

Install a signal handler to trap floating point exceptions when starting
LMMS in case it is compiled using the option described above. The
current implementation of the signal handler prints a stack trace and
then exits the application.

Currently this option is only enabled for Linux builds due to
uncertainty with regards to which of the needed headers are supported by
Windows and Apple.

Edit: @tresf 2018-03-04

To Use

cd build
cmake .. -DWANT_DEBUG_FPE=True -DCMAKE_BUILD_TYPE=Debug

Add a new CMake option WANT_DEBUG_FPE which adds the define
LMMS_DEBUG_FPE if activated. Extend lmmsconfig.h.in with regards to that
define. Include information about development options to the CMake
output.

Install a signal handler to trap floating point exceptions when starting
LMMS in case it is compiled using the option described above. The
current implementation of the signal handler prints a stack trace and
then exits the application.

Currently this option is only enabled for Linux builds due to
uncertainty with regards to which of the needed headers are supported by
Windows and Apple.
@zonkmachine
Copy link
Member

Not for the faint of heart... ;-)

I'm not really cmake literate but I think when you set WANT_DEBUG_FPE=ON you should automatically set CMAKE_BUILD_TYPE=DEBUG as it's not that useful without it.
Or make it a build type, like CMAKE_BUILD_TYPE=DEBUG_FPE.

@tresf
Copy link
Member

tresf commented Jul 8, 2017

you set WANT_DEBUG_FPE=ON you should automatically set CMAKE_BUILD_TYPE=DEBUG as it's not that useful without it.

👍 Just echo a warning if the appropriate debug flags are off and force them on.

Or make it a build type, like CMAKE_BUILD_TYPE=DEBUG_FPE.

That sounds even better. Do best practices permit making a custom CMAKE_BUILD_TYPE?

@michaelgregorius
Copy link
Contributor Author

I don't think that it really is a new build type. It's just an option that can be activated and likely only makes sense for certain builds. How about we just terminate CMake with an error in case it is enabled for non-debug builds? It's disabled by default anyway and in my opinion we only need to make sure that packagers are not able to enable it in release builds that are intended for distribution.

@tresf
Copy link
Member

tresf commented Jul 10, 2017

we only need to make sure that packagers are not able to enable it in release builds that are intended for distribution.

Mass distribution, probably, but who cares? If you want to distribute a binary with additional debug symbols, that should be welcome, no? We really struggle to get usable output from our non-developers because they can't fulfill this "gathering logs" role for us. I'd like to see debug builds for all platforms, I think it's critical to the success of the project as a whole.

@michaelgregorius
Copy link
Contributor Author

@tresf Yes, I was talking about mass distribution, e.g. the packaging for the Ubuntu repositories. I guess we don't want LMMS to exit whenever a random plugin creates NaNs or other floating point exceptions. So we have to make sure that this option cannot be enabled for production builds.

@tresf
Copy link
Member

tresf commented Jul 10, 2017

So we have to make sure that this option cannot be enabled for production builds.

Isn't this the same mutual exclusion problem/solution described here, just reversed? But I don't see cmake flags as something that get accidentally enabled, so I'm not even sure we need to safeguard this.

@michaelgregorius
Copy link
Contributor Author

It seems that I have named the flag in an unfortunate way as it will also do its work in Release mode. It might make sense to call it INSTALL_FP_EXCEPTION_HANDLER instead and to change the help text to something similar, e.g. "Install handler to trap floating point exception (development only!)". What do you think?

@tresf
Copy link
Member

tresf commented Jul 11, 2017

It seems that I have named the flag in an unfortunate way as it will also do its work in Release mode.

The name is fine. You can create a release with debug symbols. They're not mutually exclusive. From cmake.org:

"CMAKE_BUILD_TYPE - This statically specifies what build type (configuration) will be built in this build tree. Possible values are empty, Debug, Release, --> RelWithDebInfo <-- and MinSizeRel. This variable is only meaningful to single-configuration generators (such as make and Ninja)"

I think it's a good name? :)

@michaelgregorius
Copy link
Contributor Author

@tresf I know RelWithDebInfo builds from work. They are indeed quite useful if you for example want to profile a certain release. However, I don't really understand how this discussion is related to this PR. :)

What this PR adds is a function that is triggered when a floating point exception occurs, i.e. on NaNs, denormals, etc. If this situation occurs a stack trace is printed and the application exits. You can also use it to set break points in case one wants to debug a certain situation where floating points exceptions are triggered. However, as the behavior is quite drastic it should only be used during development which is why you can/have to activate this option in CMake and why it should not be used in official releases.

@tresf
Copy link
Member

tresf commented Jul 11, 2017

However, I don't really understand how this discussion is related to this PR. :)

Perhaps I misunderstood the question. This is what I read...

It seems that I have named the flag in an unfortunate way as it will also do its work in Release mode

I don't see a problem with the name.

It might make sense to call it INSTALL_FP_EXCEPTION_HANDLER instead and to change the help text to something similar

I would advise against using the INSTALL keyword in a CMake variable name, it's misleading. This is a debug flag, so I like the name you've chosen.

change the help text to something similar, e.g. "Install handler to trap floating point exception (development only!)". What do you think?

I think we can document the severity of this in our wiki. I'm not sure why someone would arbitrarily turn on flags when building. It sounds like you're trying to capture the edge-case of a Release shipping with a hard-exception symptom, but this is an optional flag. Let 'em.

@michaelgregorius
Copy link
Contributor Author

So I assume that we can merge this as is.

P.S.: I did not start the discussion to change anything. 😉

@michaelgregorius
Copy link
Contributor Author

Rebased and merged into master with commit 51dd094.

@michaelgregorius michaelgregorius deleted the DebugFloatingPointExceptions branch July 12, 2017 19:55
zonkmachine added a commit to zonkmachine/lmms that referenced this pull request Jul 18, 2017
We perform division with some Envelope and LFO variables so they mustn't
be zero. They are given a minimum value of one frame ( f_cnt_t ).
Since LMMS#3687 we have a compile flag to turn on debugging of floating point
operations. This will currently not let you pass the Envelope/LFO
initiation and this is also fixed by this PR.
zonkmachine added a commit to zonkmachine/lmms that referenced this pull request Jul 19, 2017
We perform division with some Envelope and LFO variables so they mustn't
be zero. They are given a minimum value of one frame ( f_cnt_t ).
Since LMMS#3687 we have a compile flag to turn on debugging of floating point
operations. This will currently not let you pass the Envelope/LFO
initiation and this is also fixed by this PR.
zonkmachine added a commit that referenced this pull request Jul 20, 2017
We perform division with some Envelope and LFO variables so they mustn't
be zero. They are given a minimum value of one frame ( f_cnt_t ).
Since #3687 we have a compile flag to turn on debugging of floating point
operations. This will currently not let you pass the Envelope/LFO
initiation and this is also fixed by this PR.
@zonkmachine zonkmachine mentioned this pull request Oct 2, 2017
tresf added a commit to tresf/lmms that referenced this pull request Mar 5, 2018
Allow LMMS#3687 to work on Mac
@tresf tresf mentioned this pull request Mar 5, 2018
tresf added a commit that referenced this pull request Mar 9, 2018
Allow #3687 to work on Mac
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
We perform division with some Envelope and LFO variables so they mustn't
be zero. They are given a minimum value of one frame ( f_cnt_t ).
Since LMMS#3687 we have a compile flag to turn on debugging of floating point
operations. This will currently not let you pass the Envelope/LFO
initiation and this is also fixed by this PR.
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
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.

None yet

3 participants