review request for JDK-8013461 There is a symbol AsyncGetCallTrace in libjvm.symbols that does not exist in minimal/libjvm.a when DEBUG_LEVEL == release

Thu May 23 07:52:53 PDT 2013

David, thanks for finding that!

joe

On 05/22/2013 08:09 PM, David Holmes wrote:
> Joe,
>
> On 23/05/2013 5:27 AM, Joseph Provino wrote:
>> Is there a consensus what is in the webrev is okay?
>
> Yes I am now okay with it. I found this in 4889433:
>
> "AsyncGetCallTrace() requires the CLASS_LOAD event
> to be enabled because that causes jmethodID's to be created for the
> methods at class load time. This increases the likelyhood of being
> able to get the jmethodID for a frame during a stackwalk. Since
> AsyncGetCallTrace() cannot block, it uses a special routine to the
> get the jmethodID that does not block if the jmethodID is not available
> immediately. So while CLASS_LOAD events are not strictly required, the
> data gathered during the stackwalk would be pretty useless."
>
> So reporting the ticks_no_class_load value seems quite reasonable and 
> the clients of this interface should be expecting it.
>
> Thanks,
> David
>
>> The change is to include forte.cpp in the minimal jvm but to
>> conditionalize the code so that only AsyncGetCallTrace()
>> is defined with the minimal jvm.
>>
>> Webrev is here: http://cr.openjdk.java.net/~jprovino/8013461/webrev.01
>>
>>      JDK-8013461 https://jbs.oracle.com/bugs/browse/JDK-8013461
>>      There is a symbol AsyncGetCallTrace in libjvm.symbols that does not
>> exist in
>>      minimal/libjvm.a when DEBUG_LEVEL == release
>>
>> joe
>>
>> On 05/21/2013 04:52 PM, JOSEPH PROVINO wrote:
>>>
>>> On 5/21/2013 4:00 PM, Oleg Mazurov wrote:
>>>> Though formally not part of the Solaris Studio team any more here is
>>>> my opinion based on my recollection of how I implemented interaction
>>>> with the JVM via AsyncGetCallTrace.
>>>> It's looked up using dlsym. If the symbol is not there Java callstack
>>>> collection is shut down. I understand in your case even JVMTI is not
>>>> there so the dlsym call will not be made.
>>>> From that perspective there is no difference whether the symbol is
>>>> present and returns an error code or not present at all.
>>>
>>> Oleg, then it sounds like what we have will work.
>>>
>>> Thanks for the quick reply.
>>>
>>> joe
>>>
>>>>
>>>>     -- Oleg
>>>>
>>>> On 5/21/2013 12:19 PM, JOSEPH PROVINO wrote:
>>>>>
>>>>> On 5/21/2013 3:16 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> On 5/21/13 11:26 AM, JOSEPH PROVINO wrote:
>>>>>>>
>>>>>>> On 5/21/2013 2:23 PM, Staffan Larsen wrote:
>>>>>>>> On 21 maj 2013, at 17:35, JOSEPH PROVINO
>>>>>>>> <joseph.provino at oracle.com> wrote:
>>>>>>>>
>>>>>>>>> On 5/21/2013 3:06 AM, David Holmes wrote:
>>>>>>>>>> Hi Staffan,
>>>>>>>>>>
>>>>>>>>>> On 21/05/2013 4:49 PM, Staffan Larsen wrote:
>>>>>>>>>>> On 21 maj 2013, at 04:34, David Holmes
>>>>>>>>>>> <David.Holmes at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> <added servicability>
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Joe,
>>>>>>>>>>>>
>>>>>>>>>>>> As I have previously stated you copied the struct definitions
>>>>>>>>>>>> instead of moving them outside the ifdef.
>>>>>>>>>>>>
>>>>>>>>>>>> Serviceability folk: we are particularly interested in
>>>>>>>>>>>> whether the use of ticks_no_class_load is deemed appropriate
>>>>>>>>>>>> in this situation. Who will be consuming this value?
>>>>>>>>>>> Since you have opted for the simple fix of having an exported
>>>>>>>>>>> but non-functional AsyncGetCallTrace instead of actually
>>>>>>>>>>> removing the symbol from the symbol files (which is the
>>>>>>>>>>> proposed solution in the bug report),
>>>>>>>>>> That would be a simpler solution semantically but the only way
>>>>>>>>>> I can see to do that is to use a text replacement mechanism in
>>>>>>>>>> the build files - as is done for the dynamic vtable symbols. I
>>>>>>>>>> find that less appealing than simply exporting an interface
>>>>>>>>>> that is configured to report an error (which is essentially
>>>>>>>>>> what all the optional interfaces do under the minimal VM).
>>>>>>>>>>
>>>>>>>>>>> I would like you to include a comment about this in the
>>>>>>>>>>> source. Right now it's very unclear why there is an exported
>>>>>>>>>>> function that only returns an error.
>>>>>>>>>>>
>>>>>>>>>>> As to the appropriate return value, I don't know. The only
>>>>>>>>>>> caller should be the Sun Studio profiler,
>>>>>>> Does anyone know where to find instructions on how to run the
>>>>>>> collector which would get the error return value?
>>>>>>>>>>>   and I'm not sure how it will handle this case if ever run.
>>>>>>>>>>> The possible return values aren't very well documented.
>>>>>>>>>> I guess we need to try and run it to find out.
>>>>>>>>> Okay, do either of you feel strongly about how this should be
>>>>>>>>> fixed -- return an error or remove the symbol?
>>>>>>>> No, I don't feel strongly either way, but a comment in the code
>>>>>>>> would be nice.
>>>>>>> How much effort should I put into finding out what Sun Studio
>>>>>>> profiler does when it gets -1?
>>>>>>
>>>>>> Let's ask the Solaris Studio guys directly.
>>>>>> I'm adding Oleg to the mailing list.
>>>>>>
>>>>>> Oleg,
>>>>>>
>>>>>> Could you, please, share your view on this problem?
>>>>>
>>>>> In particular what will the Sun Studio Profiler collector do if it
>>>>> gets the error
>>>>>
>>>>> trace->num_frames = ticks_no_class_load; // -1
>>>>>
>>>>> Thanks.
>>>>>
>>>>> joe
>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> joe
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Staffan
>>>>>>>>
>>>>>>>>
>>>>>>>>> joe
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>> /Staffan
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> David
>>>>>>>>>>>>
>>>>>>>>>>>> On 21/05/2013 5:10 AM, JOSEPH PROVINO wrote:
>>>>>>>>>>>>> The change is to include forte.cpp in the minimal jvm but to
>>>>>>>>>>>>> conditionalize the code so that
>>>>>>>>>>>>> only AsyncGetCallTrace() is defined with the minimal jvm.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Webrev is here:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~jprovino/8013461/webrev.00/
>>>>>>>>>>>>>
>>>>>>>>>>>>>   * JDK-8013461
>>>>>>>>>>>>> <https://jbs.oracle.com/bugs/browse/JDK-8013461>There is
>>>>>>>>>>>>>     a symbol AsyncGetCallTrace in libjvm.symbols that does
>>>>>>>>>>>>> not exist in
>>>>>>>>>>>>>     minimal/libjvm.a when DEBUG_LEVEL == release
>>>>>>>>>>>>> <https://jbs.oracle.com/bugs/browse/JDK-8013461>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>
>>>>>>>>>>>>> joe
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>