RFR: 8332506: SIGFPE In ObjectSynchronizer::is_async_deflation_needed()

[David Holmes]

On Wed, 18 Dec 2024 15:33:43 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

> This PR solves a division by zero problem, that according to the bug report happened in `ObjectSynchronizer::is_async_deflation_needed()`. As it turns out it really happened in the inlined  `monitors_used_above_threshold()` function. The problematic line looked like this:
> 
> `size_t monitor_usage = (monitors_used * 100LL) / ceiling;`
> 
> Unfortunately the `ceiling` value was increased every time there where too many deflations without any progress. This whould eventually lead to an overflow in the `ceiling` value, and in unlucky circumstances, it would become zero. Thus causing a division by zero crash.
> 
> This PR makes sure not to increase the `ceiling` value if `monitor_usage` is below the `MonitorUsedDeflationThreshold`.
> It also makes sure the ceiling value is never zero, and does not wrap around.
> 
> Tested okay in tier1-5 and `test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java`

src/hotspot/share/runtime/synchronizer.cpp line 1281:

> 1279:     bool status = true;
> 1280: 
> 1281:     // Check if we it's time to adjust the in_use_list_ceiling up, due

Suggestion:

    // Check if it's time to adjust the in_use_list_ceiling up, due

src/hotspot/share/runtime/synchronizer.cpp line 1286:

> 1284:         _no_progress_cnt >= NoAsyncDeflationProgressMax) {
> 1285:       double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
> 1286:       size_t new_ceiling = ceiling + (size_t)((double)ceiling * remainder) + 1;

I can never remember which arithmetic is okay to do for wrap-around and which is UB. To be safe the addition needs to be checked first so that wrap-around in the expression does not occur.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1890920182
PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1890921786