[14] RFR(S): 8225653: Provide more information when hitting SIGILL from HaltNode

[Vladimir Ivanov]

> I updated the webrev choosing the option to use MacroAssembler::stop() 
> instead which requires less code changes:
> http://cr.openjdk.java.net/~chagedorn/8225653/webrev.01/

I suggest to make assert->fatal changes as a separate RFE.

There are different places where it is assumed 
MacroAssembler::debug32/64 may return. I'd like those to be cleaned up, 
but it would pollute the patch.

IMO printing error message on the console is OK for now (hs_err file 
will contain generic error info).

Otherwise, looks good.

Some nitpicking:
   "Halt node: Never-taken loop exit reached"

I question "Halt node" prefix utility. It definitely makes the message 
self-descriptive on error, but IMO it adds more noise in the source 
code. So, I'd prefer to leave it out.

Best regards,
Vladimir Ivanov

> 
> Best regards,
> Christian
> 
> On 05.09.19 18:04, Christian Hagedorn wrote:
>> Hi Vladimir
>>
>> Thank you for your review!
>>
>> On 05.09.19 16:11, Vladimir Ivanov wrote:
>>> What do you think about reusing MacroAssembler::stop()? Otherwise, 
>>> you have to add MacroAssembler::halt for x86-32 as well.
>>>
>>
>> As we have discussed, this is another option to use 
>> MacroAssembler::stop() by changing the assert in 
>> MacroAssembler::debug32/64 into a guarantee() or fatal() to also crash 
>> with a message in a product build. This change should be possible as 
>> debug32/64 is currently only used in debug mode. I think choosing 
>> fatal() would be better because the halt reason will then show up in 
>> the hs_err file.
>>
>> Which option is preferable?
>>
>> Best regards,
>> Christian
>>
>>> On 04/09/2019 10:43, Christian Hagedorn wrote:
>>>> Hi
>>>>
>>>> Please review the following patch:
>>>> https://bugs.openjdk.java.net/browse/JDK-8225653
>>>> http://cr.openjdk.java.net/~chagedorn/8225653/webrev.00/
>>>>
>>>> This enhancement provides more information about executing a 
>>>> HaltNode on x86. Up to now, an illegal instruction was emitted for a 
>>>> HaltNode which resulted in a crash with SIGILL at runtime if 
>>>> executed. This made it hard to determine if the crash was actually 
>>>> caused by a HaltNode or not without doing a closer analysis.
>>>>
>>>> This patch adds a string to each HaltNode stating the purpose of it. 
>>>> Code generation for a HaltNode adds a runtime call which executes 
>>>> fatal() with the newly provided HaltNode string. The VM then crashes 
>>>> with this additional information which should make a further 
>>>> analysis easier. I filed an RFE [1] to also provide this information 
>>>> for architectures other than x86.
>>>>
>>>> The assert at [2] is not necessary anymore since there is a new 
>>>> DEFINE_CLASS_ID entry for HaltNode with ID 15. ClassMask_Halt now 
>>>> equals USHORT_MAX, making the assert always true. It might be a good 
>>>> idea to file an RFE to change the current encoding as the next entry 
>>>> with ID 16 will exceed the 16 bit limit.
>>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>> Christian
>>>>
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8230552
>>>> [2] 
>>>> http://hg.openjdk.java.net/jdk/jdk/file/d8f22418ca99/src/hotspot/share/opto/node.hpp#l750 
>>>>