RFR (S) 8216308: StackTraceElement::fill_in can use injected Class source-file

[David Holmes]

Hi Coleen,

On 15/01/2019 9:23 am, coleen.phillimore at oracle.com wrote:
> On 1/14/19 5:19 PM, David Holmes wrote:
>> On 14/01/2019 11:49 pm, coleen.phillimore at oracle.com wrote:
>>> On 1/14/19 8:23 AM, coleen.phillimore at oracle.com wrote:
>>>> On 1/14/19 8:06 AM, David Holmes wrote:
>>>>> On 14/01/2019 10:32 pm, Aleksey Shipilev wrote:
>>>>>> On 1/12/19 1:43 AM, David Holmes wrote:
>>>>>>>>> I'm also very unclear about how the redefinition case is 
>>>>>>>>> currently handled. It seems that we will
>>>>>>>>> normally intern NULL (and presumably get a NULL or empty-string 
>>>>>>>>> oop?) unless ShowHiddenFrames is
>>>>>>>>> set, in which case we use the unknown_class_name() - regardless 
>>>>>>>>> of whether the frame is actually
>>>>>>>>> hidden or not! This seems broken to me. (Separate bug to fix 
>>>>>>>>> that is okay if it is indeed broken.)
>>>>>>>>
>>>>>>>> This looks like a bug, but I'm not sure what ShowHiddenFrames is 
>>>>>>>> supposed to do here, or how it
>>>>>>>> got there.  I think if Aleksey removed that with this patch it 
>>>>>>>> would be fine with me.
>>>>>>>
>>>>>>> I think use of ShowHiddenFrames here is completely broken. But a 
>>>>>>> seperate bug and some suitable
>>>>>>> archaeology is needed to fix it the right way.
>>>>>>
>>>>>> Okay, are we in agreement that current patch does not break 
>>>>>> anything new? If so, let's push the
>>>>>
>>>>> Okay I agree it doesn't break anything new though I'd be happier if 
>>>>> the Backtrace::get_line_number issue was fixed. Otherwise it needs 
>>>>> a follow up bug too - I'm starting to think it makes no sense to 
>>>>> allow redefinition to occur within a method like this! And this is 
>>>>> a distinct issue from ShowHiddenFrames.
>>>>
>>>> There's no practical way to disable redefinition here and it's not 
>>>> something that happens.  You can file a bug for it if you like, but 
>>>> it's not something worth fixing.
>>>
>>> Looking again at the original code.  If you have a redefinition at 
>>> StringTable::intern() the line number from the methodHandle (Method*) 
>>> is correct.  In this case it's the "old" method.  It's where the 
>>> original method had the exception, which is what you want to print.
>>
>> Then why do we have the -1 handling for the initial redefinition case? 
>> It doesn't make sense to me that the logic is basically:
>>
>> if method was redefined
>>   set line number = -1
>>
>> < do other stuff that might lead to method redefinition >
>>
>> set line number with value read from possibly redefined method
>>
>> ---
>>
>> Surely the line number has to be the same, in terms of being valid or 
>> not, regardless of whether the redefinition occurred before this code 
>> or during it?
> 
>    if (method() == NULL || !version_matches(method(), version)) {
>      // The method was redefined, accurate line number information isn't 
> available
>      java_lang_StackTraceElement::set_fileName(element(), NULL);
>      java_lang_StackTraceElement::set_lineNumber(element(), -1);
>    } else {
> 
> This case handles both the case that the Method* that we got from 
> holder->method_idnum() is null (deleted) or doesn't match the version 
> that we saved in the backtrace.  method_idnum() will return the new 
> Method* which might have different line numbers if there was redefinition.
> 
> When you get to the 'else' case, if a redefinition happens, the Method* 
> is the old method and its line numbers will match the one that was saved 
> in the backtrace, because we checked the version.
> 
> I don't think the 'else' case needs to worry about redefinition. The 
> comment in Backtrace::get_source_file_name doesn't make sense anymore 
> and we've already checked the version in the 'if' above, so we only need 
> to worry about NULL for some other reason.  Again, if a redefinition 
> happens during the StringTable::intern, that's ok.  We already have a 
> Symbol with the source file name matching the version.
> 
> Does this help?

Yes - many thanks - I get it now.

David
-----

> Coleen
>>
>> Thanks,
>> David
>>
>>
>>>
>>> Coleen
>>>
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>
>>>>>> current patch in its current form, and then follow up on 
>>>>>> ShowHiddenFrames in a separate issue. This
>>>>>> would also make current patch simply backportable to 11.
>>>>>>
>>>>>> Current patch (no changes since last time):
>>>>>>    http://cr.openjdk.java.net/~shade/8216308/webrev.01/
>>>>>>
>>>>>> -Aleksey
>>>>>>
>>>>
>>>
>