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

[coleen.phillimore at oracle.com]

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?
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
>>>>>
>>>
>>