RFR (XS) 8216977: ShowHiddenFrames use in java_lang_StackTraceElement::fill_in appears broken

[coleen.phillimore at oracle.com]

Thanks Harold!
Coleen

On 8/29/19 9:21 AM, Harold Seigel wrote:
> Hi Coleen,
>
> Looks good!  Thanks for cleaning this up.
>
> Harold
>
> On 8/29/2019 8:59 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 8/29/19 1:33 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> On 29/08/2019 1:19 pm, coleen.phillimore at oracle.com wrote:
>>>> On 8/28/19 9:36 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 29/08/2019 6:25 am, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: Return NULL source file and negative line number for 
>>>>>> hidden frames.
>>>>>>
>>>>>> See bug for more details.   Ran hs tier1-3 on linux-x64-debug and 
>>>>>> tier1 on all platforms.
>>>>>
>>>>> This issue made my head spin. I think what you have done has taken 
>>>>> things back to a sane place where ShowHiddenFrames is not being 
>>>>> used to do whacky things unrelated to hidden frames.
>>>>>
>>>>>> open webrev at 
>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8216977.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8216977
>>>>>
>>>>> src/hotspot/share/classfile/javaClasses.inline.hpp
>>>>>
>>>>> Okay - this code was reporting a specially encoded "line number" 
>>>>> that actually contained the BCI, but only if ShowHiddenFrames was 
>>>>> true. Getting rid of this is good because it has nothing to do 
>>>>> with hidden frames and there wasn't even a check that the current 
>>>>> frame was in fact hidden.
>>>>>
>>>>
>>>> Yes, it could have checked if method->is_hidden() give the special 
>>>> line number with bci encoded.   Maybe tools can print something 
>>>> useful with that information. StackTraceElement.toString() doesn't 
>>>> though.
>>>>
>>>>> If we want to report the BCI when no line number table exists then 
>>>>> that should be a specific enhancement to the API.
>>>>
>>>> I agree.
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/classfile/javaClasses.cpp
>>>>>
>>>>> Okay - this code was changing a NULL source file to "unknown" but 
>>>>> only if ShowHiddenFrames is true. Again getting rid of this is 
>>>>> good as it again has nothing to do with hidden frames.
>>>>>
>>>>> If we want to report "unknown" rather than NULL, then we should 
>>>>> make that change independent of ShowHiddenFrames.
>>>>>
>>>>
>>>> Calling it <Unknown> makes it inconsistent with the case where the 
>>>> source file is unknown, which java.lang.StackFrame prints.
>>>>> ---
>>>>>
>>>>> test/hotspot/jtreg/runtime/StackTrace/HiddenFrameTest.java
>>>>>
>>>>> Can you add a comment explaining why String::concat meets the 
>>>>> criteria of "null source file" and "no line number table", as it 
>>>>> is not at all obvious to me what that should be the case. Also is 
>>>>> that true if we disable the String concatenation use of lambda? If 
>>>>> not we should add the necessary -XX:+ flag to the @run line.
>>>>
>>>> I don't know what that flag is.  It might be a property.  I don't 
>>>> think we'd run all the tests with such a flag though so I think 
>>>> this is safe.
>>>
>>> Sorry was confusing different things. You're just using a method 
>>> reference to String::concat which will use lambda expressions under 
>>> the hood. I was thinking about the lambdification of string 
>>> concatenation which is completely different.
>>
>> Oh, yes it is.
>>
>> So I have to give credit to Goetz because I copied a lot of this test 
>> case from the upcoming Helpful NPE test.
>>>
>>>>>
>>>>> For completeness can you check that the NPE only has one frame if 
>>>>> -XX:-ShowHiddenFrames is used.
>>>>
>>>> Sure:
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/2019/8216977.02/webrev/test/hotspot/jtreg/runtime/StackTrace/HiddenFrameTest.java.html 
>>>
>>>
>>>
>>>
>>> Thanks! One nit:
>>>
>>>             String arg = (String)args[0];
>>>
>>> No need for a cast there - args is an array of String.
>>
>> Ok, will fix.
>>
>> thanks!
>> Coleen
>>>
>>> No need for new webrev.
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>