Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port

[Yasumasa Suenaga]

Hi Daniil,

On 2020/03/14 7:05, Daniil Titov wrote:

Hi Yasumasa, Serguei and Alex,

Please review a new version of the webrev that includes the changes Yasumasa 
suggested.


Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher 
is not needed.


The shutdown hook registered in the HotSpotAgent c'tor only works for 
non-servers, so we still need a
the shutdown hook for remote server being added in SALauncher. I changed it to 
use  the lambda expression.

101 public HotSpotAgent() {
  102 // for non-server add shutdown hook to clean-up debugger in case
  103 // of forced exit. For remote server, shutdown hook is added by
  104 // DebugServer.
  105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
  106 new Runnable() {
  107 public void run() {
  108 synchronized (HotSpotAgent.this) {
  109 if (!isServer) {
  110 detach();
  111 }
  112 }
  113 }
  114 }));
  115 }


I missed it, thanks!



Hmm... I think port check (already in use) is not needed because 
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains
`exclusiveAccess.dirs=.` to avoid concurrent execution

As I understand exclusiveAccess.dirs prevents only the tests located in this 
directory from being run simultaneously and other tests could still run in 
parallel with one of these tests.  Thus I would prefer to have the retry 
mechanism in place. I simplified the code using the class variables instead of 
local arrays.


Ok, but I think it might be more simply with TestLibrary.
For example, can you use TestLibrary::getUnusedRandomPort ? It is used in 
test/jdk/java/rmi/testlibrary/RMID.java .


Thanks,

Yasumasa



Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd 
tests) succeeded.

[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
[2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
[3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751

Thank you,
Daniil

On 3/6/20, 6:15 PM, "Yasumasa Suenaga"  wrote:

 Hi Daniil,
 
 On 2020/03/07 3:38, Daniil Titov wrote:

 > Hi Yasumasa,
 >
 >   -> checkBasicOptions() is needed? I think you can remove this method 
and embed it in caller.
 > I think that having a piece of code that invokes  a method  named 
"buildAttachArgs" with a copy of the argument map  just for its side-effect ( it 
throws an exception if parameters are incorrect)  and ignores its return might look 
confusing. Thus, I found it more appropriate to wrap it inside a method with more relevant 
name .
 
 Ok, but I prefer to leave comment it.
 
 
 >   > SADebugDTest

 >   >  - Why do you declare portInUse and testResult as array? Their 
length is 1, so I think you don't need to use array.
 > We cannot use primitives there since these local variables are captured 
in lambda expression and are required to be final.
 > The other option is to use some other wrapper for them but I don't see 
any obvious benefits in it comparing to the array.
 
 Hmm... I think port check (already in use) is not needed because test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains `exclusiveAccess.dirs=.` to avoid concurrent execution.

 If you do not think this error check, test code is more simply.
 
 
 > I will include your other suggestion in the new version of the webrev.
 
 Sorry, I have one more comment:
 
 >   - Shutdown hook is very good idea. You can implement more simply if you use lambda expression.
 
 Shutdown hook is already registered in c'tor of HotSpotAgent.

 It works same as shutdownServer(), so I think shutdown hook at SALauncher 
is not needed.
 
 
 Thanks,
 
 Yasumasa
 
 
 > Thanks!

 > Daniil
 >
 > On 3/6/20, 12:30 AM, "Yasumasa Suenaga"  wrote:
 >
 >  Hi Daniil,
 >
 >
 >  - SALauncher.java
 >   - checkBasicOptions() is needed? I think you can remove this 
method and embed it in caller.
 >   - I think registryPort should be checked with 
Integer.parseInt() like others (rmiPort and pid) rather than regex.
 >   - Shutdown hook is very good idea. You can implement more 
simply if you use lambda expression.
 >
 >  - SADebugDTest.java
 >   - Please add bug ID to @bug.
 >   - Why do you declare portInUse and testResult as array? Their 
length is 1, so I think you don't need to use array.
 >
 >
 >  Thanks,
 >
 >  Yasumasa
 >
 >
 >  On 2020/03/06 10:15, Daniil Titov wrote:
 >  > Hi Yasumasa, Serguei and Alex,
 >  >
 >  > Please review a new version of the fix [1] that 

RFR: 8240956: SEGV in DwarfParser::process_dwarf after JDK-8234624

[Yasumasa Suenaga]

Hi all,

Please review this change:

  JBS: https://bugs.openjdk.java.net/browse/JDK-8240956
  webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8240956/webrev.00/

JDK-8234624 introduced DWARF parser in SA for unwinding native frames in jstack 
mixed mode.
However some error has seen intermittently after that.

I investigated the cause of this, I found two concerns:

  A: lack of buffer (.eh_frame section data) range check
  B: Language personality routine and Language Specific Data Area (LSDA) are 
not considered

I addd range check for .eh_frame processing, and ignore personality routine and 
LSDA in this webrev.
Also I added bailout code if DWARF processing is failed due to these concerns.

This change has passed all tests on submit repo 
(mach5-one-ysuenaga-JDK-8240956-20200313-1518-9434671),
also I tested it on my Fedora 31 box and Oracle Linux 7.7 container.


Thanks,

Yasumasa

Re: RFR: 8196751: Add jhsdb option to specify debug server RMI connector port

[Daniil Titov]

Hi Yasumasa, Serguei and Alex,

Please review a new version of the webrev that includes the changes Yasumasa 
suggested.

> Shutdown hook is already registered in c'tor of HotSpotAgent.
>It works same as shutdownServer(), so I think shutdown hook at SALauncher 
> is not needed.

The shutdown hook registered in the HotSpotAgent c'tor only works for 
non-servers, so we still need a 
the shutdown hook for remote server being added in SALauncher. I changed it to 
use  the lambda expression.

101 public HotSpotAgent() {
 102 // for non-server add shutdown hook to clean-up debugger in case
 103 // of forced exit. For remote server, shutdown hook is added by
 104 // DebugServer.
 105 Runtime.getRuntime().addShutdownHook(new java.lang.Thread(
 106 new Runnable() {
 107 public void run() {
 108 synchronized (HotSpotAgent.this) {
 109 if (!isServer) {
 110 detach();
 111 }
 112 }
 113 }
 114 }));
 115 }

>>Hmm... I think port check (already in use) is not needed because 
>> test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains 
>> `exclusiveAccess.dirs=.` to avoid concurrent execution
As I understand exclusiveAccess.dirs prevents only the tests located in this 
directory from being run simultaneously and other tests could still run in 
parallel with one of these tests.  Thus I would prefer to have the retry 
mechanism in place. I simplified the code using the class variables instead of 
local arrays.

Testing: Mach5 tier1-tier3 tests (that include serviceability/sa/sadebugd 
tests) succeeded.

[1] Webrev: http://cr.openjdk.java.net/~dtitov/8196751/webrev.03/
[2] CSR: https://bugs.openjdk.java.net/browse/JDK-8239831
[3] Bug: https://bugs.openjdk.java.net/browse/JDK-8196751 

Thank you,
Daniil

On 3/6/20, 6:15 PM, "Yasumasa Suenaga"  wrote:

Hi Daniil,

On 2020/03/07 3:38, Daniil Titov wrote:
> Hi Yasumasa,
> 
>   -> checkBasicOptions() is needed? I think you can remove this method 
and embed it in caller.
> I think that having a piece of code that invokes  a method  named 
"buildAttachArgs" with a copy of the argument map  just for its side-effect ( 
it throws an exception if parameters are incorrect)  and ignores its return 
might look confusing. Thus, I found it more appropriate to wrap it inside a 
method with more relevant name .

Ok, but I prefer to leave comment it.


>   > SADebugDTest
>   >  - Why do you declare portInUse and testResult as array? Their length 
is 1, so I think you don't need to use array.
> We cannot use primitives there since these local variables are captured 
in lambda expression and are required to be final.
> The other option is to use some other wrapper for them but I don't see 
any obvious benefits in it comparing to the array.

Hmm... I think port check (already in use) is not needed because 
test/hotspot/jtreg/serviceability/sa/sadebugd/TEST.properties contains 
`exclusiveAccess.dirs=.` to avoid concurrent execution.
If you do not think this error check, test code is more simply.


> I will include your other suggestion in the new version of the webrev.

Sorry, I have one more comment:

>   - Shutdown hook is very good idea. You can implement more 
simply if you use lambda expression.

Shutdown hook is already registered in c'tor of HotSpotAgent.
It works same as shutdownServer(), so I think shutdown hook at SALauncher 
is not needed.


Thanks,

Yasumasa


> Thanks!
> Daniil
> 
> On 3/6/20, 12:30 AM, "Yasumasa Suenaga"  wrote:
> 
>  Hi Daniil,
>  
>  
>  - SALauncher.java
>   - checkBasicOptions() is needed? I think you can remove this 
method and embed it in caller.
>   - I think registryPort should be checked with 
Integer.parseInt() like others (rmiPort and pid) rather than regex.
>   - Shutdown hook is very good idea. You can implement more 
simply if you use lambda expression.
>  
>  - SADebugDTest.java
>   - Please add bug ID to @bug.
>   - Why do you declare portInUse and testResult as array? Their 
length is 1, so I think you don't need to use array.
>  
>  
>  Thanks,
>  
>  Yasumasa
>  
>  
>  On 2020/03/06 10:15, Daniil Titov wrote:
>  > Hi Yasumasa, Serguei and Alex,
>  >
>  > Please review a new version of the fix [1] that addresses your 
comments. The new version in addition to RMI connector
>  > port option introduces two more options to specify RMI registry 
port and RMI connector host name. Currently, these
>  > last two settings could be specified using the system properties 
but the system 

Re: RFR: [small, docs] JDK-8240971 Fix CSS styles in some doc comments

[Pavel Rappo]

This is really nice. Incidentally, it also makes

  https://bugs.openjdk.java.net/browse/JDK-8234395

less relevant.

-Pavel

> On 12 Mar 2020, at 20:50, Jonathan Gibbons  
> wrote:
> 
> Please review a simple fix regarding the non-standard use of some CSS in some 
> doc comments.
> 
> From the JBS Description:
> 
> Recently, for the display of javadoc block tags, javadoc changed from using 
> an inconsistent set of CSS class names on the generated 'dt' elements to 
> using a single new name ("notes") on the enclosing 'dl' element.
> 
> There are a few (4) places in the main JDK code where the old-style names 
> were used explicitly in doc comments, in order to emulate the appearance of a 
> list of block tags. These use-sites should be fixed up. They are in the 
> following files:
> 
> open/src/java.base/share/classes/module-info.java
> open/src/java.se/share/classes/module-info.java
> open/src/java.management.rmi/share/classes/module-info.java
> open/src/jdk.jconsole/share/classes/module-info.java
> 
> In addition, these four files used the style attribute to force the font to 
> be used. The font is now set in the standard CSS for "notes", and so the 
> local use of a "style" attribute is no longer necessary.
> 
> -- Jon
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8240971
> Webrev: http://cr.openjdk.java.net/~jjg/8240971/webrev.00/index.html
> API: http://cr.openjdk.java.net/~jjg/8240971/api.00/index.html
> 

Re: RFR: [small, docs] JDK-8240971 Fix CSS styles in some doc comments

[Jonathan Gibbons]

At some point, we should separate JDK-specific definitions from 
javadoc-general definitions, using a separate stylesheet.


-- Jon

On 3/13/20 9:26 AM, Pavel Rappo wrote:

This is really nice. Incidentally, it also makes

   https://bugs.openjdk.java.net/browse/JDK-8234395

less relevant.

-Pavel


On 12 Mar 2020, at 20:50, Jonathan Gibbons  wrote:

Please review a simple fix regarding the non-standard use of some CSS in some 
doc comments.

 From the JBS Description:

Recently, for the display of javadoc block tags, javadoc changed from using an 
inconsistent set of CSS class names on the generated 'dt' elements to using a single new 
name ("notes") on the enclosing 'dl' element.

There are a few (4) places in the main JDK code where the old-style names were 
used explicitly in doc comments, in order to emulate the appearance of a list 
of block tags. These use-sites should be fixed up. They are in the following 
files:

open/src/java.base/share/classes/module-info.java
open/src/java.se/share/classes/module-info.java
open/src/java.management.rmi/share/classes/module-info.java
open/src/jdk.jconsole/share/classes/module-info.java

In addition, these four files used the style attribute to force the font to be used. The font is 
now set in the standard CSS for "notes", and so the local use of a "style" 
attribute is no longer necessary.

-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8240971
Webrev: http://cr.openjdk.java.net/~jjg/8240971/webrev.00/index.html
API: http://cr.openjdk.java.net/~jjg/8240971/api.00/index.html

Re: RFR(L) 8238268: Many SA tests are not running on OSX because they do not attempt to use sudo when available

[Igor Ignatyev]

HI Chris,

overall looks good to me, a few comments though:
1. since you removed vm.hasSAandCanAttach from VMProps, you also need to remove 
it from all TEST.ROOT files which mention it (test/jdk/TEST.ROOT and 
test/hotspot/jtreg/TEST.ROOT) so people won't be confused by undefined property 
and jtreg will be able to properly report invalid usages of it if any.

2. in SATestUtils::canAddPrivileges, could you please add some meaningful 
message to the RuntimeException at L#102?

3. SATestUtils::checkAttachOk method name is somewhat misleading (hence you had 
to put comment every time you used it), I'd recommend you to rename to smth 
like skipIfCannotAttach().

4. in SATestUtils::checkAttachOk's javadoc, it would be better to use @throws 
tag like:
> +/**
> + * Checks if SA Attach is expected to work.
> +.* @throws SkippedException ifSA Attach is not expected to work.
> + */


5. it also might make sense to catch IOException within 
SATestUtils::checkAttachOk and throw it as Error or RuntimeException.

I've briefly looked at all the changed tests and they look good.

Thanks,
-- Igor 


> On Mar 12, 2020, at 11:06 PM, Chris Plummer  wrote:
> 
> Hi Serguei,
> 
> Thanks for the review!
> 
> Can I get one more reviewer please?
> 
> thanks,
> 
> Chris
> 
> On 3/12/20 12:06 AM, serguei.spit...@oracle.com 
>  wrote:
>> Hi Chris,
>> 
>> 
>> On 3/12/20 00:03, Chris Plummer wrote:
>>> Hi Serguei,
>>> 
>>> That check used to be in Platform.shouldSAAttach(), which essentially was 
>>> moved to SATestUtils.checkAttachOk() and reworked some. It was necessary in 
>>> Platform.shouldSAAttach() since that was used to evaluation 
>>> vm.hasSAandCanAttach (which is now gone). When I moved everything to 
>>> SATestUtils.checkAttachOk(), I recall thinking it wasn't really necessary 
>>> since all tests that call it should have @require vm.hasSA, but left it in 
>>> anyway just to be extra safe. I'm still inclined to just leave it in, but 
>>> would not be opposed to removing it.
>> 
>> I agree, it is more safe to keep it, at list for now.
>> 
>> 
>> Thanks,
>> Serguei
>> 
>>> thanks,
>>> 
>>> Chris
>>> 
>>> On 3/11/20 11:20 PM, serguei.spit...@oracle.com 
>>>  wrote:
 Hi Chris,
 
 I've made another pass today.
 It looks good to me.
 
 I have just one minor questions.
 
 There is some overlap between the requires vm.hasSA check and 
 checkAttachOk:
 +public static  void checkAttachOk() throws IOException {
 +if (!Platform.hasSA()) {
 +throw new SkippedException("SA not supported.");
 +}
 In the former case, the test is not run but in the latter the 
 SkippedException is thrown.
 As I see, all tests with the checkAttachOk call use requires vm.hasSA as 
 well.
 It can be that the first check "if (!Platform.hasSA())" in the 
 checkAttachOk is redundant.
 It is okay and more safe in general but generates little confusion.
 I'm okay if you don't do anything with this but wanted to know your view.
 
 Thanks,
 Serguei
 
 
 On 3/10/20 18:57, Chris Plummer wrote:
> On 3/10/20 6:07 PM, serguei.spit...@oracle.com 
>  wrote:
>> Hi Chris,
>> 
>> Overall, this looks as a right direction to me while it is not easy to 
>> verify all the details.
> Yes, there are a lot of tests with quite a few different types of 
> changes. I did a lot of testing and verified that when the tests pass, 
> they pass for the right reasons (really ran the test, skipped due to lack 
> of privileges, or skipped due to running signed on OSX 10.14 or later). I 
> also verified locally running as root, running with a cached sudo, and 
> running without sudo.
>> I'll make another pass tomorrow. 
> Thanks!
>> 
>> A couple of quick nits so far:
>> 
>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html
>>  
>> 
>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html
>>  
>> 
>> http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html
>>  
>> 
>> 

RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump

[Schmelter, Ralf]

Hi,

I have updated the webrev: 
http://cr.openjdk.java.net/~rschmelter/webrevs/8237354/webrev.1/

It has the following significant changes:

- The jcmd now uses two separate flags. The -gz flag is now a boolean flag 
which toggles the compression  on/off. And the new -gz-level flag can be used 
to change the compression level. If tried to change the jlong flag coding to 
allow the old behavior (only one flag, which acts both as a boolean flag and a 
jlong flag), but decided against it, since it changes the semantic of a jlong 
flag. And I don't expect the -gz-level flag to be used all that much.

- I no longer use my own threads. Instead I use the WorkGang returned from 
CollectedHeap:: get_safepoint_workers(). This works fine, apart from Shenandoah 
GC, which runs into assertions when calling the CollectedHeap::object_iterate() 
method from a worker thread. I'm not sure if the assertion is too strong, but 
since the GC is currently experimental, I switch back to single threading in 
this case (as would be the case for serial GC or epsilon GC). Using the worker 
threads removes the problems the original code had regarding destruction of the 
monitor used.

- The reported number of bytes is now the one written to disk.

Best regards,
Ralf

-Original Message-
From: Ioi Lam  
Sent: Dienstag, 25. Februar 2020 18:03
To: Langer, Christoph ; Schmelter, Ralf 
; Yasumasa Suenaga ; 
serguei.spit...@oracle.com; hotspot-runtime-...@openjdk.java.net runtime 

Cc: serviceability-dev@openjdk.java.net
Subject: Re: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump

Hi Christoph,

This sounds fair. I will remove my objection :-)

Thanks
- Ioi

RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents

[Reingruber, Richard]

Hi Martin,

thanks a lot for reviewing and the feedback. I'll dig into the details as soon 
as possible. Looking forward to it :)

Thanks, Richard.

-Original Message-
From: Doerr, Martin  
Sent: Donnerstag, 12. März 2020 17:28
To: Reingruber, Richard ; 'Robbin Ehn' 
; Lindenmaier, Goetz ; David 
Holmes ; Vladimir Kozlov (vladimir.koz...@oracle.com) 
; serviceability-dev@openjdk.java.net; 
hotspot-compiler-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L) 8227745: Enable Escape Analysis for Better Performance in 
the Presence of JVMTI Agents

Hi Richard,


I managed to find time for a (almost) complete review of webrev.4. (I'll review 
the tests separately.)

First of all, the change seems to be in pretty good quality for its significant 
complexity. I couldn't find any real bugs. But I'd like to propose minor 
improvements.
I'm convinced that it's mature because we did substantial testing.

I like the new functionality for object deoptimization. It can possibly be 
reused for future escape analysis based optimizations. So I appreciate having 
it available in the code base.
In addition to that, your change makes the JVMTI implementation better 
integrated into the VM.


Now to the details:


src/hotspot/share/c1/c1_IR.hpp
describe_scope parameters. Ok.


src/hotspot/share/ci/ciEnv.cpp
src/hotspot/share/ci/ciEnv.hpp
Fix for JvmtiExport::can_walk_any_space() capability. Ok.


src/hotspot/share/code/compiledMethod.cpp
Nice cleanup!


src/hotspot/share/code/debugInfoRec.cpp
src/hotspot/share/code/debugInfoRec.hpp
Additional parmeters. (Remark: I think "non_global_escape_in_scope" would read 
better than "not_global_escape_in_scope", but your version is consistent with 
existing code, so no change request from my side.) Ok.


src/hotspot/share/code/nmethod.cpp
Nice cleanup!


src/hotspot/share/code/pcDesc.hpp
Additional parameters. Ok.


src/hotspot/share/code/scopeDesc.cpp
src/hotspot/share/code/scopeDesc.hpp
Improved implementation + additional parameters. Ok.


src/hotspot/share/compiler/compileBroker.cpp
src/hotspot/share/compiler/compileBroker.hpp
Extra thread for DeoptimizeObjectsALot. (Remark: I would have put it into a 
follow up change together with the test in order to make this webrev smaller, 
but since it is included, I'm reviewing everything at once. Not a big deal.) Ok.


src/hotspot/share/jvmci/jvmciCodeInstaller.cpp
Additional parameters. Ok.


src/hotspot/share/opto/c2compiler.cpp
Make do_escape_analysis independent of JVMCI capabilities. Nice!


src/hotspot/share/opto/callnode.hpp
Additional fields for MachSafePointNodes. Ok.


src/hotspot/share/opto/escape.cpp
Annotation for MachSafePointNodes. Your added functionality looks correct.
But I'd prefer to move the bulky code out of the large function.
I suggest to factor out something like has_not_global_escape and 
has_arg_escape. So the code could look like this:
  SafePointNode* sfn = sfn_worklist.at(next);
  sfn->set_not_global_escape_in_scope(has_not_global_escape(sfn));
  if (sfn->is_CallJava()) {
CallJavaNode* call = sfn->as_CallJava();
call->set_arg_escape(has_arg_escape(call));
  }
This would also allow us to get rid of the found_..._escape_in_args variables 
making the loops better readable.

It's kind of ugly to use strcmp to recognize uncommon trap, but that seems to 
be the way to do it (there are more such places). So it's ok.


src/hotspot/share/opto/machnode.hpp
Additional fields for MachSafePointNodes. Ok.


src/hotspot/share/opto/macro.cpp
Allow elimination of non-escaping allocations. Ok.


src/hotspot/share/opto/matcher.cpp
src/hotspot/share/opto/output.cpp
Copy attribute / pass parameters. Ok.


src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp
Nice cleanup!


src/hotspot/share/prims/jvmtiEnv.cpp
src/hotspot/share/prims/jvmtiEnvBase.cpp
Escape barriers + deoptimize objects for target thread. Good.


src/hotspot/share/prims/jvmtiImpl.cpp
src/hotspot/share/prims/jvmtiImpl.hpp
The sequence is pretty complex:
VM_GetOrSetLocal element initialization executes EscapeBarrier code which 
suspends the target thread (extra VM Operation).
VM_GetOrSetLocal::doit_prologue performs object deoptimization (by VM Thread to 
prepare VM Operation with frame deoptimization).
VM_GetOrSetLocal destructor implicitly calls EscapeBarrier destructor which 
resumes the target thread.
But I don't have any improvement proposal. Performance is probably not a 
concern, here. So it's ok.

VM_GetOrSetLocal::deoptimize_objects deoptimizes the top frame if it has 
non-globally escaping objects and other frames if they have arg escaping ones. 
Good.


src/hotspot/share/prims/jvmtiTagMap.cpp
Escape barriers + deoptimize objects for all threads. Ok.


src/hotspot/share/prims/whitebox.cpp
Added WB_IsFrameDeoptimized to API. Ok.


src/hotspot/share/runtime/deoptimization.cpp
Object deoptimization. I have more comments and proposals, here.
First of all, handling recursive and waiting locks in 

Re: RFR(L) 8238268: Many SA tests are not running on OSX because they do not attempt to use sudo when available

[Chris Plummer]

  
  
Hi Serguei,
  
  Thanks for the review!
  
  Can I get one more reviewer please?
  
  thanks,
  
  Chris
  
  On 3/12/20 12:06 AM, serguei.spit...@oracle.com wrote:


  
  Hi Chris,


On 3/12/20 00:03, Chris Plummer wrote:
  
  
Hi Serguei,
  
  That check used to be in Platform.shouldSAAttach(), which
  essentially was moved to SATestUtils.checkAttachOk() and
  reworked some. It was necessary in Platform.shouldSAAttach()
  since that was used to evaluation vm.hasSAandCanAttach (which
  is now gone). When I moved everything to
  SATestUtils.checkAttachOk(), I recall thinking it wasn't
  really necessary since all tests that call it should have
  @require vm.hasSA, but left it in anyway just to be extra
  safe. I'm still inclined to just leave it in, but would not be
  opposed to removing it.

  
  
  I agree, it is more safe to keep it, at list for now.
  
  
  Thanks,
  Serguei
  
  
 thanks,
  
  Chris
  
  On 3/11/20 11:20 PM, serguei.spit...@oracle.com wrote:


  Hi Chris,

I've made another pass today.
It looks good to me.

I have just one minor questions.

There is some overlap between the requires
  vm.hasSA check and checkAttachOk:
+public static  void checkAttachOk() throws IOException {
+if (!Platform.hasSA()) {
+throw new SkippedException("SA not supported.");
+}
In the former case, the test is not
run but in the latter the SkippedException is thrown.
As I see, all tests with the checkAttachOk
  call use requires vm.hasSA as
  well.
  It can be that the first check
  "if (!Platform.hasSA())" in the checkAttachOk is redundant.
It is okay and
more safe in general but generates little confusion.
I'm okay if you don't do anything with this but wanted
to know your view.

Thanks,
Serguei


On 3/10/20 18:57, Chris Plummer wrote:
  
  
On 3/10/20 6:07 PM, serguei.spit...@oracle.com
  wrote:


  Hi Chris,

Overall, this looks as a right direction to me while it
is not easy to verify all the details.
  

Yes, there are a lot of tests with quite a few different
types of changes. I did a lot of testing and verified that
when the tests pass, they pass for the right reasons (really
ran the test, skipped due to lack of privileges, or skipped
due to running signed on OSX 10.14 or later). I also
verified locally running as root, running with a cached
sudo, and running without sudo.

   I'll make another pass
tomorrow. 
  

Thanks!

   
A couple of quick nits so far:

http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestCpoolForInvokeDynamic.java.udiff.html
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestDefaultMethods.java.udiff.html
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java.udiff.html
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java.udiff.html
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJhsdbJstackMixed.java.udiff.html
http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/hotspot/jtreg/serviceability/sa/TestRevPtrsForInvokeDynamic.java.udiff.html
 import jdk.test.lib.Utils;
-import jdk.test.lib.Asserts;
+import jdk.test.lib.SA.SATestUtils;
Need to swap these exports.


  

Ok

   http://cr.openjdk.java.net/~cjplummer/8238268/webrev.00/test/lib/jdk/test/lib/SA/SATestUtils.java.frames.html
  48 if (SATestUtils.needsPrivileges()) {
  49 cmdStringList = SATestUtils.addPrivileges(cmdStringList);

The method calls are local, so the class name can be
omitted in the method names: