RFR (XS) 8216977: ShowHiddenFrames use in java_lang_StackTraceElement::fill_in appears broken
Harold Seigel
harold.seigel at oracle.com
Thu Aug 29 13:21:10 UTC 2019
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
>>>
>
More information about the hotspot-runtime-dev
mailing list