PING: RFR: 8205992: jhsdb cannot attach to Java processes running in Docker containers

[Chris Plummer]

Hi Yasumasa,

Changes look and and passed all my testing.

thanks,

Chris

On 7/19/18 10:13 PM, Yasumasa Suenaga wrote:
> Hi Chris,
>
> Thank you for your comment.
> I uploaded new webrev. Could you review again?
>
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.02/
>
> I tested my change on Linux x64, but I cannot check it on other
> platform (includes older Linux).
> However SA tests are included in HotSpot tier 1 tests. Tests on submit
> repo work fine with this change
> (mach5-one-ysuenaga-JDK-8205992-20180720-0305-31840).
>
>
> Thanks,
>
> Yasumasa
>
>
> 2018-07-20 3:26 GMT+09:00 Chris Plummer <chris.plummer at oracle.com>:
>> Hi Yasumasa,
>>
>>    84     // It maps the LWPID in the host to it in the container.
>>
>> "it" -> "the PID"
>>
>>   286     // Get LWPID in the host from the container's LWPID.
>>   287     public int getHostPID(int id) {
>>   288         try {
>>   289             return nspidMap.get(id);
>>   290         } catch (NullPointerException e) {
>>   291             return -1;
>>   292         }
>>   293     }
>>
>> What is the source of the NPE here? Is it because nspidMap was never
>> initialized because the process is not in a container? In that case I think
>> you should be checking for null rather than having an NPE be part of normal
>> execution.
>>
>>    42             int hostPID =
>> ((LinuxDebuggerLocal)debugger).getHostPID(pid);
>>    43             if (hostPID != -1) {
>>    44                 pid = hostPID;
>>    45             }
>>
>> A comment here would be helpful.
>>
>> The rest looks good. I should probably run it through some internal testing.
>> Let me know when you have a final webrev.
>>
>> thanks,
>>
>> Chris
>>
>>
>> On 7/18/18 5:59 AM, Yasumasa Suenaga wrote:
>>> PING:
>>>
>>> Could you review it?
>>>
>>>     JBS:    https://bugs.openjdk.java.net/browse/JDK-8205992
>>>     webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.01/
>>>
>>> This change has been reviewed by Jini.
>>> We need a Reviewer.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2018/07/12 13:42, Yasumasa Suenaga wrote:
>>>> Thanks Jini,
>>>>
>>>> I uploaded new webrev. It contains some comments and removing extra
>>>> space.
>>>>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.01/
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>>
>>>> 2018-07-12 2:32 GMT+09:00 Jini George <jini.george at oracle.com>:
>>>>> Hi Yasumasa,
>>>>>
>>>>> This looks good to me except for one nit. And some more comments would
>>>>> help.
>>>>> For e.g., it would help to say that NSPidMap is to map the host to
>>>>> container
>>>>> lwpids.
>>>>>
>>>>> The nit:
>>>>>
>>>>> *
>>>>>
>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.00/src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.c.sdiff.html
>>>>> Line 253: extra space after the parentheses
>>>>>
>>>>> Thanks,
>>>>> Jini.
>>>>>
>>>>> On 7/4/2018 4:34 AM, Yasumasa Suenaga wrote:
>>>>>>
>>>>>> PING: Could you review it?
>>>>>>
>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8205992
>>>>>>>     webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.00/
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2018/06/28 22:12, Yasumasa Suenaga wrote:
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this change.
>>>>>>>
>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8205992
>>>>>>>     webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.00/
>>>>>>>
>>>>>>> I tried to attach jhsdb to java process in docker container from
>>>>>>> container host, but it couldn't.
>>>>>>> jcmd supports PID namespace in JDK-8193710, but jhsdb hasn't yet.
>>>>>>>
>>>>>>> SA gets LWP ID via thread stack and funcs in libthread_db.so, but they
>>>>>>> returns PIDs in container - they are different from host's PID. So I
>>>>>>> added
>>>>>>> the code to scan /proc/<PID>/task to get all LWP IDs and they are kept
>>>>>>> in a
>>>>>>> Map in LinuxDebuggerLocal.
>>>>>>>
>>>>>>> Also SA_ALTROOT is set to /proc/<PID>/root if SA detects debuggee runs
>>>>>>> in
>>>>>>> container. It helps SA to parse binaries in container.
>>>>>>>
>>>>>>> This change has been pushed to submit repo, and it was failed on OS X
>>>>>>> (mach5-one-ysuenaga-JDK-8205992-20180628-1015-28963).
>>>>>>> But I guess it causes JDK-8205906. This change affects to Linux only.
>>>>>>>
>>>>>>> Could you review it?
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>