Re: RFR: JDK-8199700: SA: Enable jhsdb jtreg tests for Mac OS X
Hi Jini, There are quite a few changes to digest in this - it may have been better to break them up individually: - sudo use - refactor to use ClshdbLauncher - changes to use regex matching Focusing on the main sudo change the assumption is that on OSX you can run sudo without needing to provide a password - correct? That may be the case in mach5 but I'm not sure how others will go running these tests either in their test farms or locally. I'm not sure about the regex changes from contains to matches - won't you need additional wildcards at the start and end of the strings to allow the string to be embedded in a longer string ?? Thanks, David PS. I start vacation in 48 hours :) On 11/07/2018 12:38 PM, Jini George wrote: Gentle reminder ! Thanks, Jini. On 7/10/2018 12:14 AM, Jini George wrote: Requesting reviews for enabling SA tests on OS X for Mach5. https://bugs.openjdk.java.net/browse/JDK-8199700 Webrev: http://cr.openjdk.java.net/~jgeorge/8199700/webrev.00/ The changes are mostly to include the addition of sudo privileges to the SA launchers for OSX if Platform.shouldSAAttach() fails. Some tests (those using clhsdb) have been refactored to use ClhsdbLauncher for ease of maintainence. This also avoids checks for Platform.shouldSAAttach() for corefile related test cases. More details have been provided in JIRA. Thanks, Jini.
Re: RFR: JDK-8199700: SA: Enable jhsdb jtreg tests for Mac OS X
Thank you, David. My answers inline: On 7/11/2018 1:54 PM, David Holmes wrote: Hi Jini, There are quite a few changes to digest in this - it may have been better to break them up individually: - sudo use - refactor to use ClshdbLauncher - changes to use regex matching Focusing on the main sudo change the assumption is that on OSX you can run sudo without needing to provide a password - correct? That may be the case in mach5 but I'm not sure how others will go running these tests either in their test farms or locally. Right -- you would need to provide the password. So it prompts for the password for OSX. (Like how it would have been needed if you had run the test itself with 'sudo'). Examining the /etc/sudoers file to check if no password is needed could have been an option, but that itself would need an sudo, and probably would add unwanted complexity. I'm not sure about the regex changes from contains to matches - won't you need additional wildcards at the start and end of the strings to allow the string to be embedded in a longer string ?? OutputAnalyzer's shouldMatch() uses the find() method of the Matcher class which matches sub-sequences. Thanks, Jini. Thanks, David PS. I start vacation in 48 hours :) On 11/07/2018 12:38 PM, Jini George wrote: Gentle reminder ! Thanks, Jini. On 7/10/2018 12:14 AM, Jini George wrote: Requesting reviews for enabling SA tests on OS X for Mach5. https://bugs.openjdk.java.net/browse/JDK-8199700 Webrev: http://cr.openjdk.java.net/~jgeorge/8199700/webrev.00/ The changes are mostly to include the addition of sudo privileges to the SA launchers for OSX if Platform.shouldSAAttach() fails. Some tests (those using clhsdb) have been refactored to use ClhsdbLauncher for ease of maintainence. This also avoids checks for Platform.shouldSAAttach() for corefile related test cases. More details have been provided in JIRA. Thanks, Jini.
Re: PING: RFR: 8205992: jhsdb cannot attach to Java processes running in Docker containers
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//task to get all LWP IDs and they are kept in a Map in LinuxDebuggerLocal. Also SA_ALTROOT is set to /proc//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
RFR: JDK-8201513: nsk/jvmti/IterateThroughHeap/filter-* are broken
Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8201513 webrev: http://cr.openjdk.java.net/~amenkov/IterateThroughHeap/webrev/ summary: The tests had a error which was fixed during open-sourcing. After that the tests started to fail. Root cause of the failures is wrong verification (positive results are interpreted as negative) --alex
RFR:8207048: jhsdb debugd cannot specify a port number
Hi all, I filed bugzilla for small fix to improvement of `jhsdb debugd` to set a port of UnicastRemoteObject aka sun.jvm.hotspot.debugger.remote.RemoteDebuggerServer by `sun.jvm.hotspot.rmi.debugger.port`. Issue: https://bugs.openjdk.java.net/browse/JDK-8207048 Webrev: http://cr.openjdk.java.net/~ykubota/8207048/webrev.00/ We can set an RMI registry port of debugd server by `sun.jvm.hotspot.rmi.port`, but can not set a port of RemoteObject. So RemoteObject always uses an anonymous port. For security, we should not open ports widely to use debugd, so I want to fix. Could you review it? Thanks, Yuji
Re: RFR JDK-8191948 : jdb error: InvalidTypeException: Can't assign double[][][] to double[][][]
Hi Daniil, It looks good. Thanks, Serguei On 7/11/18 22:23, Daniil Titov wrote: Please review the changes that fix jdb issue with evaluation of multidimensional arrays of primitives. The problem here is that for N-dimensional arrays of the primitives with N greater then 2, JDI fails to find its component type (which is an array of dimension N-1) assuming that it is a boot type. Thanks! Issue: https://bugs.openjdk.java.net/browse/JDK-8191948 Webrev: http://cr.openjdk.java.net/~dtitov/8191948/webrev.01 Best regards, Daniil
Re: PING: RFR: 8205992: jhsdb cannot attach to Java processes running in Docker containers
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 : > 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//task to get all LWP IDs and they are kept in a >>> Map in LinuxDebuggerLocal. >>> >>> Also SA_ALTROOT is set to /proc//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 >>> >
Re: RFR:8207048: jhsdb debugd cannot specify a port number
Hi David, Thank you for comment and updating JBS. I'll create a CSR request after getting comments whether this change is welcomed by community. Thanks, Yuji 2018-07-12 10:21 GMT+09:00 David Holmes : > Hi Yuji, > > I can't comment on the actual change proposed in this enhancement request, > but it will need to have a CSR request created and approved due to the use > of a new system property. > > Thanks, > David > > > > > On 11/07/2018 11:55 PM, KUBOTA Yuji wrote: >> >> Hi all, >> >> I filed bugzilla for small fix to improvement of `jhsdb debugd` to set >> a port of UnicastRemoteObject aka >> sun.jvm.hotspot.debugger.remote.RemoteDebuggerServer by >> `sun.jvm.hotspot.rmi.debugger.port`. >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8207048 >> Webrev: http://cr.openjdk.java.net/~ykubota/8207048/webrev.00/ >> >> We can set an RMI registry port of debugd server by >> `sun.jvm.hotspot.rmi.port`, but can not set a port of RemoteObject. So >> RemoteObject always uses an anonymous port. For security, we should >> not open ports widely to use debugd, so I want to fix. >> >> Could you review it? >> >> Thanks, >> Yuji >> >
Re: PING: RFR: 8205992: jhsdb cannot attach to Java processes running in Docker containers
Looks good to me. Thanks! Jini (Not a Reviewer). On 7/12/2018 10:12 AM, 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 : 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//task to get all LWP IDs and they are kept in a Map in LinuxDebuggerLocal. Also SA_ALTROOT is set to /proc//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
Re: PING: RFR: 8205992: jhsdb cannot attach to Java processes running in Docker containers
Thanks Jini! I'm waiting for Reviewer. Yasumasa 2018-07-12 14:09 GMT+09:00 Jini George : > Looks good to me. > > Thanks! > Jini (Not a Reviewer). > > > On 7/12/2018 10:12 AM, 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 : >>> >>> 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//task to get all LWP IDs and they are kept > in a > Map in LinuxDebuggerLocal. > > Also SA_ALTROOT is set to /proc//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 > >>> >
Re: RFR: JDK-8199700: SA: Enable jhsdb jtreg tests for Mac OS X
On 11/07/2018 8:00 PM, Jini George wrote: Thank you, David. My answers inline: On 7/11/2018 1:54 PM, David Holmes wrote: Hi Jini, There are quite a few changes to digest in this - it may have been better to break them up individually: - sudo use - refactor to use ClshdbLauncher - changes to use regex matching Focusing on the main sudo change the assumption is that on OSX you can run sudo without needing to provide a password - correct? That may be the case in mach5 but I'm not sure how others will go running these tests either in their test farms or locally. Right -- you would need to provide the password. So it prompts for the password for OSX. (Like how it would have been needed if you had run the test itself with 'sudo'). Examining the /etc/sudoers file to check if no password is needed could have been an option, but that itself would need an sudo, and probably would add unwanted complexity. So I'm not sure this change is acceptable when it may cause other testing environments to break. At a minimum I'd want to get the opinions of the SAP folk and anyone else doing regular build/test runs. I'm not sure about the regex changes from contains to matches - won't you need additional wildcards at the start and end of the strings to allow the string to be embedded in a longer string ?? OutputAnalyzer's shouldMatch() uses the find() method of the Matcher class which matches sub-sequences. Ok. Thanks, David Thanks, Jini. Thanks, David PS. I start vacation in 48 hours :) On 11/07/2018 12:38 PM, Jini George wrote: Gentle reminder ! Thanks, Jini. On 7/10/2018 12:14 AM, Jini George wrote: Requesting reviews for enabling SA tests on OS X for Mach5. https://bugs.openjdk.java.net/browse/JDK-8199700 Webrev: http://cr.openjdk.java.net/~jgeorge/8199700/webrev.00/ The changes are mostly to include the addition of sudo privileges to the SA launchers for OSX if Platform.shouldSAAttach() fails. Some tests (those using clhsdb) have been refactored to use ClhsdbLauncher for ease of maintainence. This also avoids checks for Platform.shouldSAAttach() for corefile related test cases. More details have been provided in JIRA. Thanks, Jini.
RFR JDK-8191948 : jdb error: InvalidTypeException: Can't assign double[][][] to double[][][]
Please review the changes that fix jdb issue with evaluation of multidimensional arrays of primitives. The problem here is that for N-dimensional arrays of the primitives with N greater then 2, JDI fails to find its component type (which is an array of dimension N-1) assuming that it is a boot type. Thanks! Issue: https://bugs.openjdk.java.net/browse/JDK-8191948 Webrev: http://cr.openjdk.java.net/~dtitov/8191948/webrev.01 Best regards, Daniil
Re: RFR:8207048: jhsdb debugd cannot specify a port number
Hi Yuji, I can't comment on the actual change proposed in this enhancement request, but it will need to have a CSR request created and approved due to the use of a new system property. Thanks, David On 11/07/2018 11:55 PM, KUBOTA Yuji wrote: Hi all, I filed bugzilla for small fix to improvement of `jhsdb debugd` to set a port of UnicastRemoteObject aka sun.jvm.hotspot.debugger.remote.RemoteDebuggerServer by `sun.jvm.hotspot.rmi.debugger.port`. Issue: https://bugs.openjdk.java.net/browse/JDK-8207048 Webrev: http://cr.openjdk.java.net/~ykubota/8207048/webrev.00/ We can set an RMI registry port of debugd server by `sun.jvm.hotspot.rmi.port`, but can not set a port of RemoteObject. So RemoteObject always uses an anonymous port. For security, we should not open ports widely to use debugd, so I want to fix. Could you review it? Thanks, Yuji
Re: RFR: JDK-8201513: nsk/jvmti/IterateThroughHeap/filter-* are broken
Hi Alex, The fix looks good. Thank you for fixing the typos! Thanks, Serguei On 7/11/18 11:39, Alex Menkov wrote: Hi all, please review a fix for https://bugs.openjdk.java.net/browse/JDK-8201513 webrev: http://cr.openjdk.java.net/~amenkov/IterateThroughHeap/webrev/ summary: The tests had a error which was fixed during open-sourcing. After that the tests started to fail. Root cause of the failures is wrong verification (positive results are interpreted as negative) --alex
Re: RFR (S) 8206960: [Graal] serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor tests fail
Hi Jc, The fix looks good. I'll sponsor a push once it has been reviewed. Thanks, Serguei On 7/11/18 10:04, JC Beyler wrote: Hi all, Could someone review the small-ish webrev for the bug: https://bugs.openjdk.java.net/browse/JDK-8206960 The webrev is here: http://cr.openjdk.java.net/~jcbeyler/8206960/webrev.00/ Basically, the tests were failing for two reasons: - VMEventTest was failing because Graal does not support DisableIntrinsic required by the test, I disabled testing the test with Graal in this case - The other tests were failing because the BCI <-> source code line numbers are not always correct when using Graal via uncommon traps; therefore the tests now check if Graal is being used and, if so, only checks the method names. This allows us to still have tests working with Graal, albeit a bit more coarse. This passes all the HeapMonitor tests with -vmoptions:"-XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI -XX:+TieredCompilation -XX:+UseJVMCICompiler -Djvmci.Compiler=graal" (Except the GCCMS one which is being fixed via the one-liner for JDK-8205643). Let me know what you think, Jc
