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

[David Holmes]

Hi Gerard,

On 2/06/2020 2:29 am, gerard ziemski wrote:
> 
> 
> 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?

I think it is trivial.

Thanks,
David

> 
> cheers