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

[David Holmes]

On Fri, 20 Dec 2024 19:38:44 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> What's more clear to some people, might be more unclear to some other people (like me).
>> 
>> To me it's clear that we have an overflow after adding two unsigned numbers A and B, if the result is less than any of the numbers A and B. A quick search on the interwebs also confirms this. So, in this case if `new_ceiling < old_ceiling` we have an overflow.
>> 
>> Looking at the suggested:
>> 
>> 
>> size_t delta = (size_t)(ceiling * remainder) + 1;
>> if (ceiling > SIZE_MAX - delta) { // overflow
>> 
>> 
>> I see "if ceiling is larger than the largest possible number minus some delta (which might be very small), we have and overflow" and I think, this might be true... Not saying you are wrong, but it's definitely not clear to me by just looking at it.
>> 
>> Definitely agree that we should fix the math that calculates the `new_ceiling` later.
>
> Both expressions: the one in the patch and the suggested expression for overflow test require study and neither are faster to understand. Both require comment so it's really a preference of which math expression you like the best. Either is fine to me because they both look correct to me and with a comment I don't have to reexamine them.
> 
> As for the minimal fix, the suggested patch does two things that are related and both desirable to have as a fix.  Moving the ceiling calculation to only be adjusted if the monitors used are above MonitorDeflationThreshold, and fixing the overflow resolve both causes of the bug.   I think we want both of these. The less minimal patch of determining how to adjust the ceiling and what to adjust it to, with what values and potentially removing some of the command line arguments should be longer term work.

The transformation using delta is the idiomatic way to convert potentially overflowing (and possibly UB) expressions into a conditional operation that avoids overflow. As Coleen states neither form is generally obvious to all readers.

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

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