RFR: 8252657: JVMTI agent is not unloaded when Agent_OnAttach is failed
David Holmes
david.holmes at oracle.com
Fri Dec 18 04:00:27 UTC 2020
On 16/12/2020 5:36 pm, Yasumasa Suenaga wrote:
> Hi David,
>
>> I'm not sure you mean exactly by DLL unloading mechanism is not hooked.
>
> I want to say we do not hook `FreeLibrary()` on Windows and `dlclose()`
> on Linux.
> Can we describe ""this function may be called - " at here?
I think it is clear this refers to the VM doing the unloading not
somebody issuing direct dlclose/FreeLibrary commands.
David
>
> Cheers,
> Yasumasa
>
>
> On 2020/12/16 15:48, David Holmes wrote:
>> On 16/12/2020 4:33 pm, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>>> "This function will be called by the VM when the library is about to
>>>> be unloaded. The library will be unloaded (unless it is statically
>>>> linked into the executable) and this function will be called if some
>>>> platform specific mechanism causes the unload (an unload mechanism
>>>> is not specified in this document) or the library is (in effect)
>>>> unloaded by the termination of the VM. VM termination includes
>>>> normal termination and VM failure, including start-up failure, but
>>>> not, of course, uncontrolled shutdown. An implementation may also
>>>> choose to not call this function if the
>>>> Agent_OnAttach/Agent_OnAttach_L function reported an error (returned
>>>> a non-zero value)."
>>>
>>> Thank you so much!
>>> According to this, we can fix this bug simply. (We can unload agent
>>> library without `Agent_OnUnload()` call)
>>
>> Yes I think it is reasonable to allow this. But of course there must
>> be a general consensus.
>>
>>> However I think following sentence should be removed because DLL
>>> unloading mechanism is not hooked, and also `Agent_OnUnload()`
>>> wouldn't be called by dll unloading - it depends on VM termination /
>>> failure / non-zero value from entry points.
>>>
>>> "this function will be called if some platform specific mechanism
>>> causes the unload (an unload mechanism is not specified in this
>>> document)"
>>
>> I'm not sure you mean exactly by DLL unloading mechanism is not
>> hooked. But that sentence has to remain because it makes it clear that
>> the decision as to when unloading occurs is up to an implementation
>> and is not defined by the specification.
>>
>> Cheers,
>> David
>> -----
>>
>>>
>>> If you are OK, I will update CSR.
>>>
>>>
>>> Cheers,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/12/16 11:09, David Holmes wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Sorry for the delay getting back to this.
>>>>
>>>> On 1/12/2020 4:41 pm, Yasumasa Suenaga wrote:
>>>>> Hi David,
>>>>>
>>>>> On 2020/12/01 14:59, David Holmes wrote:
>>>>>> Looking at the original webrev from September:
>>>>>>
>>>>>> https://cr.openjdk.java.net/~ysuenaga/JDK-8252657/webrev.00/src/hotspot/share/prims/jvmtiExport.cpp.cdiff.html
>>>>>>
>>>>>>
>>>>>> The suggestion to unload the library if Agent_OnAttach fails seems
>>>>>> quite reasonable - it is what happens if no Agent_OnAttach
>>>>>> function can be found in the agent.
>>>>>>
>>>>>> But the specification is lacking because it simply states any such
>>>>>> error is "ignored" - which is an oversimplification and I think
>>>>>> was really intended to contrast with the abort behaviour if
>>>>>> Agent_OnLoad fails. In addition the specification indicates that
>>>>>> Agent_OnUnload is called any time the agent library is unloaded,
>>>>>> except for "uncontrolled shutdown". This unfortunately suggests
>>>>>> that before unloading the library after OnAttach fails, we also
>>>>>> call OnUnload. That seems wrong and in fact the VM does not do
>>>>>> that - and noone seems to have complained.
>>>>>>
>>>>>> Also note the specification states an agent "must export a
>>>>>> start-up function ..." but doesn't say what happens if it doesn't
>>>>>> do so.
>>>>>>
>>>>>> My gut feeling for what the specification should say here is that
>>>>>> if the start-up function does not exist, or the call to
>>>>>> Agent_OnAttach reports an error, then the agent library is
>>>>>> immediately unloaded with no attempt to call the agent shutdown
>>>>>> function (Agent_OnUnload).
>>>>>
>>>>> That's what I wanted to suggest!
>>>>> I understand we need to consider following case in this issue, is
>>>>> it right?
>>>>>
>>>>> Agent_OnLoad / Agent_OnLoad_L does not exist:
>>>>> - Agent_OnUnload is not called
>>>>> - DLL is not unloaded (JVM will abort)
>>>>>
>>>>> Agent_OnLoad / Agent_OnLoad_L failed:
>>>>> - Agent_OnUnload is not called
>>>>> - DLL is not unloaded (JVM will abort)
>>>>
>>>> These cases are already well covered.
>>>>
>>>>> Agent_OnAttach does not exist:
>>>>> - Agent_OnUnload is not called
>>>>> - DLL is unloaded
>>>>>
>>>>> Agent_OnAttach_L does not exist:
>>>>> - Agent_OnUnload is not called
>>>>> - DLL is not unloaded (static link)
>>>>
>>>> These cases are kind of implicitly covered, but could be clarified.
>>>>
>>>>> Agent_OnAttach failed:
>>>>> - Agent_OnUnload is not called
>>>>> - DLL is unloaded
>>>>>
>>>>> Agent_OnAttach_L faled:
>>>>> - Agent_OnUnload is not called
>>>>> - DLL is not unloaded (static link)
>>>>
>>>> These are the problematic cases.
>>>>
>>>>>
>>>>>> But with my "compatibility glasses" on this may be too strong to
>>>>>> retrofit to the specification as other VMs may behave differently.
>>>>>> So as I stated in the CSR request we probably want to allow the
>>>>>> current hotspot behaviour but not mandate it (unless we check with
>>>>>> all the other VM implementations that such a specification is okay
>>>>>> with them).
>>>>>
>>>>> Ok, for example, can we change the spec as following?
>>>>> (Of course, this is not all)
>>>>>
>>>>> ```
>>>>> diff --git a/src/hotspot/share/prims/jvmti.xml
>>>>> b/src/hotspot/share/prims/jvmti.xml
>>>>> index 44553b8065f..57c87b1a71b 100644
>>>>> --- a/src/hotspot/share/prims/jvmti.xml
>>>>> +++ b/src/hotspot/share/prims/jvmti.xml
>>>>> @@ -688,7 +688,8 @@ Agent_OnUnload_L(JavaVM *vm)</example>
>>>>> mechanism causes the unload (an unload mechanism is not
>>>>> specified in this document)
>>>>> or the library is (in effect) unloaded by the termination of
>>>>> the VM whether through
>>>>> normal termination or VM failure, including start-up failure.
>>>>> - Uncontrolled shutdown is, of course, an exception to this rule.
>>>>> + Uncontrolled shutdown is, of course, an exception to this
>>>>> rule, and also it might not be called
>>>>> + when start-up function does not exist or fails (returns
>>>>> non-zero value).
>>>>
>>>> I think this is what was originally proposed and as I said then I
>>>> don't like burying this detail inside the reference to uncontrolled
>>>> shutdown. We should make this much more explicit which requires some
>>>> reworking of the existing far-too-long sentence.
>>>>
>>>> "This function will be called by the VM when the library is about to
>>>> be unloaded. The library will be unloaded (unless it is statically
>>>> linked into the executable) and this function will be called if some
>>>> platform specific mechanism causes the unload (an unload mechanism
>>>> is not specified in this document) or the library is (in effect)
>>>> unloaded by the termination of the VM. VM termination includes
>>>> normal termination and VM failure, including start-up failure, but
>>>> not, of course, uncontrolled shutdown. An implementation may also
>>>> choose to not call this function if the
>>>> Agent_OnAttach/Agent_OnAttach_L function reported an error (returned
>>>> a non-zero value)."
>>>>
>>>> Cheers,
>>>> David
>>>> -----
>>>>
>>>>> Note the distinction between this function and the
>>>>> <eventlink id="VMDeath">VM Death event</eventlink>: for the
>>>>> VM Death event
>>>>> to be sent, the VM must have run at least to the point of
>>>>> initialization and a valid
>>>>> ```
>>>>>
>>>>>> I agree that the more general issue of re-loading an agent is a
>>>>>> separate issue.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>> On 1/12/2020 3:21 pm, David Holmes wrote:
>>>>>>> On 1/12/2020 3:19 pm, David Holmes wrote:
>>>>>>>> On 1/12/2020 2:46 pm, Yasumasa Suenaga wrote:
>>>>>>>>> Hi Chris, David,
>>>>>>>>>
>>>>>>>>> Currently Agent_OnUnload() is not called when Agent_OnLoad() is
>>>>>>>>> failed - JVM will abort.
>>>>>>>>> Should we re-think this behavior?
>>>>>>>>
>>>>>>>> We should we rethink that? It is probably one of the clearest
>>>>>>>> parts of the spec. If Agent_Onload fails that is considered a
>>>>>>>> fatal error - end of story.
>>>>>>>
>>>>>>> I meant, of course, "Why should we rethink that?".
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>> The issue is with Agent_onAttach and how its failure should, or
>>>>>>>> should not, impact Agent_OnUnload.
>>>>>>>>
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>> https://github.com/YaSuenag/jvmti-examples/tree/master/helloworld
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>> $ java -agentpath:/path/to/libhelloworld.so=error --version
>>>>>>>>> Hello World from Agent_OnLoad()
>>>>>>>>> options = error
>>>>>>>>> Error occurred during initialization of VM
>>>>>>>>> agent library failed to init: /path/to/libhelloworld.so
>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020/12/01 11:44, David Holmes wrote:
>>>>>>>>>> On 1/12/2020 11:45 am, Chris Plummer wrote:
>>>>>>>>>>> On Fri, 2 Oct 2020 07:27:43 GMT, Yasumasa Suenaga
>>>>>>>>>>> <ysuenaga at openjdk.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>> * Q3: What has to be done for statically linked agent?
>>>>>>>>>>>>
>>>>>>>>>>>> JVMTI spec says "unless it is statically linked into the
>>>>>>>>>>>> executable", so I think we can ignore about
>>>>>>>>>>>> Agent_OnUnload_L() in this PR.
>>>>>>>>>>>
>>>>>>>>>>> I don't think that makes sense. If you call it for
>>>>>>>>>>> dynamically linked then you need to call it for statically
>>>>>>>>>>> linked also.
>>>>>>>>>>
>>>>>>>>>> Agreed. Even though you can't physically unload the statically
>>>>>>>>>> linked library, if it is logically unloaded by some mechanism,
>>>>>>>>>> then Agent_OnUnload_L is supposed to be called.
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>>> -------------
>>>>>>>>>>>
>>>>>>>>>>> PR: https://git.openjdk.java.net/jdk/pull/19
>>>>>>>>>>>
More information about the serviceability-dev
mailing list