RFR (S): 8245833: crash_with_sigfpe uses pthread_kill(SIGFPE) on macOS

[gerard ziemski]

On 5/30/20 8:38 AM, David Holmes wrote:
> On 30/05/2020 2:59 am, gerard ziemski wrote:
>> On 5/29/20 11:52 AM, gerard ziemski wrote:
>>> hi David,
>>>
>>> Thank you for the review.
>>>
>>> On 5/28/20 7:03 PM, David Holmes wrote:
>>>> Hi Gerard,
>>>>
>>>> On 29/05/2020 3:34 am, gerard ziemski wrote:
>>>>> hi all,
>>>>>
>>>>> Please review this small and simple fix, that implements 
>>>>> crash_with_sigfpe() in a way that causes an actual crash on macOS, 
>>>>> so it doesn't need to fallback that uses pthread_kill()
>>>>>
>>>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8245833
>>>>> webrev at http://cr.openjdk.java.net/~gziemski/8245833_rev1
>>>>> passes Mach5 hs_tier1,2,3,4,5
>>>>
>>>> Fix looks fine.
>>>>
>>>> So presumably this old code:
>>>>
>>>>    volatile int x = 0;
>>>>    volatile int y = 1/x;
>>>>
>>>> is actually elided by the compiler when we build for macOS?
>>>
>>> It's not exactly elided, since the compiler still generates assembly 
>>> for that code, but I noticed that while normally the compiler would 
>>> complain about the unused "y", in this case it does not, so it 
>>> probably optimizes it without actually performing the division by 
>>> zero, due to some compiler flag we are using (I don't know which one 
>>> makes the difference here), i.e.:
>>>
>>>   volatile int x = 0;
>>>   volatile int y = 1/x;
>>>
>>>   xorl  %eax, %eax
>>>   .loc  37 1751 16              ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1751:16
>>>   movl  %eax, -88(%rbp)
>>>   .loc  37 1752 22              ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1752:22
>>>   movl  -88(%rbp), %ecx
>>>   .loc  37 1752 21 is_stmt 0    ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1752:21
>>>   leal  1(%rcx), %edx
>>>   cmpl  $3, %edx
>>>   cmovael  %eax, %ecx
>>>   .loc  37 1752 16              ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1752:16
>>>   movl  %ecx, -152(%rbp)
>>>
>>> I don't see division instruction here, however for:
>>>
>>>  sigfpe_int = sigfpe_int/sigfpe_int;
>>>
>>>   .loc  37 1751 16              ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1751:16
>>>   movl  _sigfpe_int(%rip), %eax
>>>   .loc  37 1751 26 is_stmt 0    ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1751:26
>>>   cltd
>>>   idivl  _sigfpe_int(%rip)
>>>   .loc  37 1751 14              ## 
>>> open/src/hotspot/share/utilities/vmError.cpp:1751:14
>>>   movl  %eax, _sigfpe_int(%rip)
>>>
>>> we see the "idivl" instruction in the assembly.
>>>
>>> For reference, a simple C test case with standard compiler flags 
>>> produces:
>>>
>>>  volatile int x = 0;
>>>  volatile int y = 1/x;
>>>
>>>  .loc  1 439 16                ## hello/main.cpp:439:16
>>>  movl  $0, -20(%rbp)
>>>  .loc  1 440 22                ## hello/main.cpp:440:22
>>>  movl  -20(%rbp), %ecx
>>>  .loc  1 440 21 is_stmt 0      ## hello/main.cpp:440:21
>>>  movl  $1, %edx
>>>  movl  %eax, -28(%rbp)         ## 4-byte Spill
>>>  movl  %edx, %eax
>>>  cltd
>>>  idivl  %ecx
>>>  .loc  1 440 16                ## hello/main.cpp:440:16
>>>  movl  %eax, -24(%rbp)
>>>  .loc  1 441 3 is_stmt 1       ## hello/main.cpp:441:3
>>>
>>> which also has the "idivl" instruction and also crashes, so it must 
>>> be one of our compiler flags that optimizes the unused variable?
>>
>> It must be more than optimizing an unused variable, because even when 
>> I do use the "y" (print its vale out - it's 0) the code still will 
>> not crash. Some other optimization is at play here...
>
> Thanks for investigating. It is a puzzle. :) But as long as the new 
> code successfully raises SIGFPE the change is good.

JFYI: Using any level of "-O" optimization makes the old code not crash 
- we use "-Os" in the file in question.

Would this be considered a trivial change, or do I need a second review?


cheers