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

8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code #3175

Draft
wants to merge 3 commits into
base: master
from

Conversation

@gerard-ziemski
Copy link

@gerard-ziemski gerard-ziemski commented Mar 24, 2021

hello everyone,

Signal handling code is complex as it is, this enhancement removes, incorrect, catching & handling of FPE_FLT types of signals.

It's incorrect, because:

  • We wouldn't want to generate/catch a floating point div by zero instructions, which by default do not cause a crash (only integer div by 0 cause crash)

  • We can't catch them because that would require us to set FPU to raise such signals, which we don't do

  • Even if we wanted to generate FPU signals, those only originate in x87 FPU, which we are not using (we use SSE instructions for floating point arithmetic instead) on x86_64 architecture. Finally the API to turn it on is not even available on macOS at all (it's seems to be available on Linux, but I don't know what it actually does on 64 bit architecture, if anything)

If we were to catch FPE_FLT signal it most likely is a bug in the OS itself, like for example JDK-8261397

In anticipation of such scenarios we have a generic handling of FPE_FLT, but we report it as OS bug that needs to be investigated.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3175/head:pull/3175
$ git checkout pull/3175

Update a local copy of the PR:
$ git checkout pull/3175
$ git pull https://git.openjdk.java.net/jdk pull/3175/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3175

View PR using the GUI difftool:
$ git pr show -t 3175

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3175.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 24, 2021

👋 Welcome back gziemski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 24, 2021

@gerard-ziemski The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@gerard-ziemski gerard-ziemski marked this pull request as ready for review Mar 24, 2021
@openjdk openjdk bot added the rfr label Mar 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

Webrevs

@gerard-ziemski gerard-ziemski marked this pull request as draft Mar 24, 2021
@openjdk openjdk bot removed the rfr label Mar 24, 2021
@gerard-ziemski gerard-ziemski marked this pull request as ready for review Mar 24, 2021
@openjdk openjdk bot added the rfr label Mar 24, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Gerard,

Thanks for tackling this one.

I'm not certain we should make this a fatal condition rather than just printing a warning and throwing the exception. Perhaps use an assert.

Thanks,
David

@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 25, 2021

Hi Gerard,

Thanks for tackling this one.

I'm not certain we should make this a fatal condition rather than just printing a warning and throwing the exception. Perhaps use an assert.

Thanks,
David

Thank you David for the review.

I figured that if we were to get such unexpected signal we would be in unknown territory, but I can change it to assert instead.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 26, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Gerard,

On 26/03/2021 1:03 am, Gerard Ziemski wrote:

On Thu, 25 Mar 2021 07:19:17 GMT, David Holmes <dholmes at openjdk.org> wrote:

Hi Gerard,

Thanks for tackling this one.

I'm not certain we should make this a fatal condition rather than just printing a warning and throwing the exception. Perhaps use an assert.

Thanks,
David

Thank you David for the review.

I figured that if we were to get such unexpected signal we would be in unknown territory, but I can change it to assert instead.

Note that I also suggested that we throw the exception. I don't think we
can just do nothing here as it implies we'd return potential junk to a
FP computation. Though in that case is there actually any point in
making any changes to this code?

While we no longer use the x87 co-processor in hotspot code, I'm
concerned there may be legacy third-party libraries that may still use
it via their own native code, and so potentially enable and trigger
these floating-point signals.

Thanks,
David

@gerard-ziemski
Copy link
Author

@gerard-ziemski gerard-ziemski commented Mar 26, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Gerard,

On 26/03/2021 1:03 am, Gerard Ziemski wrote:

On Thu, 25 Mar 2021 07:19:17 GMT, David Holmes wrote:

Hi Gerard,
Thanks for tackling this one.
I'm not certain we should make this a fatal condition rather than just printing a warning and throwing the exception. Perhaps use an assert.
Thanks,
David

Thank you David for the review.
I figured that if we were to get such unexpected signal we would be in unknown territory, but I can change it to assert instead.

Note that I also suggested that we throw the exception. I don't think we
can just do nothing here as it implies we'd return potential junk to a
FP computation. Though in that case is there actually any point in
making any changes to this code?

While we no longer use the x87 co-processor in hotspot code, I'm
concerned there may be legacy third-party libraries that may still use
it via their own native code, and so potentially enable and trigger
these floating-point signals.

I'm not sure we were ever enabling x87 signals. I seriously doubt we ever wanted to do that (on purpose) or that we had it in fact ever enabled.

3rd parties could/should add their own signal exception handling for x87 FPU if they want it. Those are very domain specific, and if they wanted them they would certainly not want us handling them.

I think this is a very dead code, that unnecessarily complicates our signal handling with zero benefit and I personally would love to see it go away, especially now that we have an understanding of what FPE_FLT signals are. There is enough signal handling code that we do need to support as it is.

Is that a good enough argument, or are you still unconvinced and you don't want to see it fixed?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Gerard,

On 27/03/2021 1:01 am, Gerard Ziemski wrote:

On Thu, 25 Mar 2021 07:19:17 GMT, David Holmes <dholmes at openjdk.org> wrote:

Hi Gerard,
Thanks for tackling this one.
I'm not certain we should make this a fatal condition rather than just printing a warning and throwing the exception. Perhaps use an assert.
Thanks,
David

Thank you David for the review.
I figured that if we were to get such unexpected signal we would be in unknown territory, but I can change it to assert instead.

Note that I also suggested that we throw the exception. I don't think we
can just do nothing here as it implies we'd return potential junk to a
FP computation. Though in that case is there actually any point in
making any changes to this code?

While we no longer use the x87 co-processor in hotspot code, I'm
concerned there may be legacy third-party libraries that may still use
it via their own native code, and so potentially enable and trigger
these floating-point signals.

I'm not sure we were ever enabling x87 signals. I seriously doubt we ever wanted to do that (on purpose) or that we had it in fact ever enabled.

But this isn't just about x87 signals. The use of SSE can also generate
hardware exceptions that can manifest as signals at the OS level.

3rd parties could/should add their own signal exception handling for x87 FPU if they want it. Those are very domain specific, and if they wanted them they would certainly not want us handling them.

The fact we turn a SIGFPE FPE_FLTDIV into a Java ArithmeticException may
be exactly what they want. They may have discovered they don't need to
install their own handlers precisely because the VM handles things how
they expect. Though this begs the question as to why they would enable
signals in the first place ... though with signal chaining things might
again work as hoped.

I think this is a very dead code, that unnecessarily complicates our signal handling with zero benefit and I personally would love to see it go away, especially now that we have an understanding of what FPE_FLT signals are. There is enough signal handling code that we do need to support as it is.

Is that a good enough argument, or are you still unconvinced and you don't want to see it fixed?

Sorry, if this was x87 only then I could accept the dead code argument,
but as it seems applicable to SSE as well then ... it just isn't clear
to me we can get rid of this code without impacting some end users. And
the change to the code hardly makes a difference to
understandability/readability. In fact if macOS did not have the
FPE_FLTINV bug, we would still need the FPE_FLTDIV check that you are
proposing to remove!

Cheers,
David
-----

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 26, 2021

@gerard-ziemski This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@gerard-ziemski gerard-ziemski marked this pull request as draft Apr 26, 2021
@openjdk openjdk bot removed the rfr label Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants