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.plum...@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.geo...@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 >>>>>> >>>> > >
