RFR: 8252657: JVMTI agent is not unloaded when Agent_OnAttach is failed
David Holmes
david.holmes at oracle.com
Wed Dec 16 06:48:49 UTC 2020
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 hotspot-dev
mailing list