RFR (S) 8208172: SIGSEGV when owner of invokedynamic bootstrap method throws an exception - Symbol::increment_refcount()+0x0

[Ioi Lam]

Hi Coleen,

The fix looks good. I think it's defensive and can be easily back ported 
independent of JDK-8209553 (ExceptionInInitializerError can have a 
default detail message if the cause is given).

Thanks

- Ioi



On 8/21/18 3:44 PM, coleen.phillimore at oracle.com wrote:
>
> Thanks, David.
> Coleen
>
> On 8/20/18 10:25 PM, David Holmes wrote:
>> On 21/08/2018 10:30 AM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 8/20/18 7:56 PM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> Looks good.
>>>>
>>>> On 21/08/2018 6:04 AM, coleen.phillimore at oracle.com wrote:
>>>>> Summary: make table for resolution errors not always expect 
>>>>> non-null message string.
>>>>
>>>> Thanks for fixing this. I'm unsure why we thought there had to be a 
>>>> detail message? AFAIK there is no specified requirement for this. 
>>>> Obviously having a detail message is the most common case.
>>>>
>>>> I'm also not clear why we only hit this in the indy bootstrap case?
>>>
>>> We always create LinkageErrors in the vm with a message, so we 
>>> expected one to always be present.  The case where it's converted to 
>>> ExceptionInInitializerError hadn't been seen, or expected to be 
>>> missing this information.  It's probably possible to write a test to 
>>> have LinkageError =>  EIIE without indy.
>>
>> The obvious direct example to produce EIIE from a static initializer 
>> doesn't induce the problem. Interesting ...
>>
>>>>
>>>> One minor comment:
>>>>
>>>> src/hotspot/share/classfile/resolutionErrors.cpp
>>>>
>>>> void ResolutionErrorEntry::set_message(Symbol* c) {
>>>>     _message = c;
>>>> +   if (_message != NULL) {
>>>>       _message->increment_refcount();
>>>> +   }
>>>>   }
>>>>
>>>> Is there any reason not to move the assignment inside the if clause 
>>>> as well?
>>>>
>>>
>>> The C++ class isn't zero initialized, so we have to initialize the 
>>> message to NULL.
>>
>> And no constructor that does the init either - okay.
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Tested with hs-tier1-2, and all hotspot/jtreg/runtime tests, and 
>>>>> test added.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8208172.01/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8208172
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>
>