Code Review for JEP 259: Stack-Walking API

[John Rose]

On Nov 11, 2015, at 12:29 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> On 11/11/15 2:32 PM, Mandy Chung wrote:
>>> On Nov 10, 2015, at 6:05 AM, Dmitry Samersoff <dmitry.samersoff at oracle.com> wrote:
>>> 
>>> Mandy,
>>> 
>>> Native part looks good for me.
>>> 
>>> javaClasses.cpp:
>>> 
>>> 1. It might be good to create a helper inline function for
>>> 
>>>  int bci_version = stackFrame->int_field(bci_offset);
>>>  int version     = BackTrace::version_at(bci_version);
>>> 
>> Coleen gave me some suggestion to use injected fields.  This will be changed in the next revision.
>> 
>>> 2. java_lang_StackFrameInfo::fill_methodInfo
>>> 
>>>  CHECK macro left methodInfo partially initialized, not sure it's OK.
>>> 
>> Any exception here will cause StackWalker::walk to fail.  I expect the exception thrown here  would be OOME and other internal errors.  So it should be okay.
>> 
>>> javaClasses.inline.hpp:
>>> 
>>> 78 You can use the same pattern as in assert:
>>> 
>>>    if ((jushort)version != version) version = USHRT_MAX;
>>> 
>>> 117 Is it possible to add comment about +1000000 magic?
>>> 
>> This is existing code refactored from javaClasses.hpp.  I don’t know why it added  +1000000.  For hidden frames, keeping line number to -1 is good to me since it means unavailable.
>> 
>> Coleen - do you know why this is done this way?
> 
> Honestly, I have no idea.  I refactored it into that function preserving the original code.

I added it to make backtraces for internally generated functions more informative.
Since the stack walker will be able to surface BCIs directly, there will be no need for it.

— John