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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Gerard Ziemski-2
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](https://bugs.openjdk.java.net/browse/JDK-8261397)

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

-------------

Commit messages:
 - remove unused FPE_FLT signal processing and replace with generic catch

Changes: https://git.openjdk.java.net/jdk/pull/3175/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3175&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263603
  Stats: 16 lines in 3 files changed: 9 ins; 2 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3175.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3175/head:pull/3175

PR: https://git.openjdk.java.net/jdk/pull/3175
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v2]

Gerard Ziemski-2
> 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](https://bugs.openjdk.java.net/browse/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.

Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:

  add generic catch of FPE_FLT

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3175/files
  - new: https://git.openjdk.java.net/jdk/pull/3175/files/b184aa71..6aed7506

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3175&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3175&range=00-01

  Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3175.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3175/head:pull/3175

PR: https://git.openjdk.java.net/jdk/pull/3175
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v2]

David Holmes-2
On Wed, 24 Mar 2021 16:08:07 GMT, Gerard Ziemski <[hidden email]> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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.
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
>
>   add generic catch of FPE_FLT

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

-------------

PR: https://git.openjdk.java.net/jdk/pull/3175
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v2]

Gerard Ziemski-2
On Thu, 25 Mar 2021 07:19:17 GMT, David Holmes <[hidden email]> 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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3175
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v3]

Gerard Ziemski-2
In reply to this post by Gerard Ziemski-2
> 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](https://bugs.openjdk.java.net/browse/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.

Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:

  change from fatal error to an assert

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3175/files
  - new: https://git.openjdk.java.net/jdk/pull/3175/files/6aed7506..82102386

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3175&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3175&range=01-02

  Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3175.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3175/head:pull/3175

PR: https://git.openjdk.java.net/jdk/pull/3175
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v2]

David Holmes
In reply to this post by Gerard Ziemski-2
Hi Gerard,

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

> On Thu, 25 Mar 2021 07:19:17 GMT, David Holmes <[hidden email]> 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

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3175
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v2]

Gerard Ziemski-2
In reply to this post by Gerard Ziemski-2
On Thu, 25 Mar 2021 15:00:36 GMT, Gerard Ziemski <[hidden email]> 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
>
>> 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.

> _Mailing list message from [David Holmes](mailto:[hidden email]) on [hotspot-runtime-dev](mailto:[hidden email]):_
>
> 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.

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?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3175
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263603: Remove leftovers of "FPE_FLT..." type signals from regular signal handling code [v2]

David Holmes
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
-----

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3175
>