Re: Integrated: 8282428: ProblemList jdk/jfr/jvm/TestWaste.java

[Mikael Vidstedt]

On Sun, 27 Feb 2022 03:37:11 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to ProblemList jdk/jfr/jvm/TestWaste.java.

Marked as reviewed by mikael (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7631

Re: RFR: Merge jdk17 [v2]

[Mikael Vidstedt]

On Mon, 2 Aug 2021 23:53:59 GMT, Jesper Wilhelmsson  
wrote:

>> Forwardport JDK 17 -> JDK 18
>
> Jesper Wilhelmsson has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Revert "8271150: Remove EA from JDK 17 version string starting with Initial 
> RC promotion on Aug 5, 2021(B34)"
>   
>   This reverts commit f8fb5713074b8960f5530d7aca954f84d57c1f30.

Marked as reviewed by mikael (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4964

Re: [jdk17] Integrated: 8270109: ProblemList 4 SA tests on macOS-aarch64

[Mikael Vidstedt]

On Thu, 8 Jul 2021 19:17:53 GMT, Daniel D. Daugherty  wrote:

> A trivial fix to ProblemList 4 SA tests on macOS-aarch64

Marked as reviewed by mikael (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/234

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Mikael Vidstedt]

On Wed, 3 Feb 2021 20:08:28 GMT, Mikael Vidstedt  wrote:

>>> I wonder if this is the right choice
>>> ...
>>> ```
>>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>>> ...
>>> ```
>>> 
>> 
>> The transition in OopStorageParIterPerf was made for gtest setup to execute 
>> in WXWrite context. For tests themselves, defining macro set WXWrite.
>> 
>> I've simplified the scheme and now we switch to WXWrite once at the gtest 
>> launcher. So this transition was dropped.
>> 
>> I've also refreshed my memory and tried to switch to WXWrite as close as 
>> possible to each place where we'll be writing executable memory. There are a 
>> lot of such places! As you correctly noted, code cache contains objects, not 
>> plain data. For example, CodeCache memory management structures, 
>> CompiledMethod, ...  are there, so we need more WXWrite switches than we 
>> have in the current approach. I had a comparable amount of them just to run 
>> -version, but certainly not enough to run tier1 tests.
>> 
>> Following your advice, I don't require a known "from" state anymore. So a 
>> few W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
>> function, which expects to be called from the native code. I had to switch 
>> to WXExec just only to satisfy the expectations. After the update, we don't 
>> need this anymore.
>> 
>> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
>> functions are not marked as entries for some reason, although they are 
>> called directly from e.g. interpreter. I added W^X management to such 
>> functions.
>> 
>> Thank you!
>
> Out of curiosity, have you looked at the performance of the W^X state 
> transition? In particular I'm wondering if the cost is effectively negligible 
> so doing it unconditionally on JVM entry is a no-brainer and just 
> easier/cleaner than the alternatives, or if there are reasons to look at only 
> doing the transition if/when needed (perhaps do it lazily and revert back to 
> X when leaving the JVM?).

You read my mind, Andrew. Unless, of course, it's optimized to leverage the 
fact that it's thread-specific..

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Mikael Vidstedt]

On Wed, 3 Feb 2021 20:05:29 GMT, Anton Kozlov  wrote:

>> Thank you all for your comments regarding W^X implementation. I've made a 
>> change that reduces the footprint of the implementation, also addressing 
>> most of the comments. I'll revisit them individually to make sure nothing is 
>> forgotten.
>> 
>> The basic principle has not changed: when we execute JVM code (owned by 
>> libjvm.so, starting from JVM entry function), we switch to Write state. When 
>> we leave JVM to execute generated or JNI code, we switch to Executable 
>> state. I would like to highlight that JVM code does not mean the VM state of 
>> the java thread. After @stefank's suggestion, I could also drop a few W^X 
>> state switches, so now it should be more clear that switches are tied to JVM 
>> entry functions.
>
>> I wonder if this is the right choice
>> ...
>> ```
>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>> ...
>> ```
>> 
> 
> The transition in OopStorageParIterPerf was made for gtest setup to execute 
> in WXWrite context. For tests themselves, defining macro set WXWrite.
> 
> I've simplified the scheme and now we switch to WXWrite once at the gtest 
> launcher. So this transition was dropped.
> 
> I've also refreshed my memory and tried to switch to WXWrite as close as 
> possible to each place where we'll be writing executable memory. There are a 
> lot of such places! As you correctly noted, code cache contains objects, not 
> plain data. For example, CodeCache memory management structures, 
> CompiledMethod, ...  are there, so we need more WXWrite switches than we have 
> in the current approach. I had a comparable amount of them just to run 
> -version, but certainly not enough to run tier1 tests.
> 
> Following your advice, I don't require a known "from" state anymore. So a few 
> W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
> function, which expects to be called from the native code. I had to switch to 
> WXExec just only to satisfy the expectations. After the update, we don't need 
> this anymore.
> 
> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
> functions are not marked as entries for some reason, although they are called 
> directly from e.g. interpreter. I added W^X management to such functions.

Out of curiosity, have you looked at the performance of the W^X state 
transition? In particular I'm wondering if the cost is effectively negligible 
so doing it unconditionally on JVM entry is a no-brainer and just 
easier/cleaner than the alternatives, or if there are reasons to look at only 
doing the transition if/when needed (perhaps do it lazily and revert back to X 
when leaving the JVM?).

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR(XS): 8245521: Remove STACK_BIAS

[Mikael Vidstedt]

David/Matthias/Vladimir, thanks for the reviews! Change pushed.

Cheers,
Mikael

> On May 26, 2020, at 12:01 PM, Vladimir Kozlov  
> wrote:
> 
> On 5/21/20 9:11 PM, David Holmes wrote:
>> Hi Mikael,
>> Looks good.
> 
> +1
> 
>> I assume the change to GraalHotSpotVMConfig.java is to allow it to work with 
>> older VMs?
> 
> Yes. stackBias will be set to 0 if STACK_BIAS is not present. Otherwise it 
> will be set to STACK_BIAS value.
> 
> Thanks,
> Vladimir
> 
>> Thanks,
>> David
>> On 22/05/2020 1:36 pm, Mikael Vidstedt wrote:
>>> 
>>> Please review this small change which removes the STACK_BIAS constant and 
>>> its uses:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245521
>>> webrev: 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/
>>> 
>>> Background (from JBS):
>>> 
>>> With Solaris/SPARC removed the STACK_BIAS definition in 
>>> src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can 
>>> be removed.
>>> 
>>> 
>>> Testing:
>>> 
>>> Tier1
>>> 
>>> Cheers,
>>> Mikael
>>> 

RFR(XS): 8245521: Remove STACK_BIAS

[Mikael Vidstedt]

Please review this small change which removes the STACK_BIAS constant and its 
uses:

JBS: https://bugs.openjdk.java.net/browse/JDK-8245521
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8245521/webrev.00/open/webrev/

Background (from JBS):

With Solaris/SPARC removed the STACK_BIAS definition in 
src/hotspot/share/utilities/globalDefinitions.hpp is now always 0 and can be 
removed.


Testing:

Tier1

Cheers,
Mikael

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

> On May 8, 2020, at 12:48 PM, Daniel D. Daugherty 
>  wrote:
> 
> On 5/7/20 1:35 AM, Mikael Vidstedt wrote:
>> New webrev here:
>> 
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
> 
> This pretty much says it all:
> 
> > Summary of changes:90904 lines changed: 8 ins; 90725 del; 171 mod; 
> > 103780 unchg
> 
> 
> My review is focused on looking at the changes and not looking for missed
> changes. I figure there's enough work here just looking at the changes to
> keep me occupied for a while and enough people have posted comments about
> finding other things to be examined, etc...
> 
> Unlike my normal reviews, I won't be listing all the touched files;
> (there's _only_ 427 of them...)
> 
> Don't forget to make a copyright year update pass before you push.

Yup - I have added it in 10 different places on my TODO list to maximize the 
likelihood of me remembering it :)

> 
> src/hotspot/os/posix/os_posix.hpp
> L174
> old L175 #ifndef SOLARIS
> L176
>nit - on most of this style of deletion you also got rid of
>one of the blank lines, but not here.

Oops, will fix.

> src/hotspot/share/utilities/dtrace.hpp
> old L42: #elif defined(__APPLE__)
> old L44: #include 
> old L45: #else
> new L32: #include 
>  was previous included only for __APPLE__ and it
> is now there for every platform. Any particular reason?

No particular reason other than "it looks cleaner". I guess we could see if the 
include can be removed altogether.

> Thumbs up!

Thanks for the review!!

Cheers,
Mikael

> 
>> incremental: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/
>> 
>> Remaining items:
>> 
>> * File follow-up to remove STACK_BIAS
>> 
>> * File follow-ups to change/update/remove flags and/or flag documentation: 
>> UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots, 
>> ArrayAllocatorMallocLimit, ThreadPriorityPolicy
>> 
>> * File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”, 
>> “sunos”, “sun studio”, “s compiler bug”, “niagara”, …)
>> 
>> 
>> Please let me know if there’s something I have missed!
>> 
>> Cheers,
>> Mikael
>> 
>>> On May 3, 2020, at 10:12 PM, Mikael Vidstedt  
>>> wrote:
>>> 
>>> 
>>> Please review this change which implements part of JEP 381:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>> webrev: 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>>> 
>>> 
>>> Note: When reviewing this, please be aware that this exercise was 
>>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>>> individual changes carefully. You may want to get that coffee cup filled up 
>>> (or whatever keeps you awake)!
>>> 
>>> 
>>> Background:
>>> 
>>> Because of the size of the total patch and wide range of areas touched, 
>>> this patch is one out of in total six partial patches which together make 
>>> up the necessary changes to remove the Solaris and SPARC ports. The other 
>>> patches are being sent out for review to mailing lists appropriate for the 
>>> respective areas the touch. An email will be sent to jdk-dev summarizing 
>>> all the patches/reviews. To be clear: this patch is *not* in itself 
>>> complete and stand-alone - all of the (six) patches are needed to form a 
>>> complete patch. Some changes in this patch may look wrong or incomplete 
>>> unless also looking at the corresponding changes in other areas.
>>> 
>>> For convenience, I’m including a link below[1] to the full webrev, but in 
>>> case you have comments on changes in other areas, outside of the files 
>>> included in this thread, please provide those comments directly in the 
>>> thread on the appropriate mailing list for that area if possible.
>>> 
>>> In case it helps, the changes were effectively produced by searching for 
>>> and updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, 
>>> etc. More information about the areas impacted can be found in the JEP 
>>> itself.
>>> 
>>> A big thank you to Igor Ignatyev for helping make the changes to the 
>>> hotspot tests!
>>> 
>>> Also, I have a short list of follow-ups which I’m going to look at 
>>> separately from this JEP/patch, mainly related to command line 
>>> options/flags which are no longer relevant and should be 
>>> deprecated/obsoleted/removed.
>>> 
>>> Testing:
>>> 
>>> A slightly earlier version of this change successfully passed tier1-8, as 
>>> well as client tier1-2. Additional testing will be done after the first 
>>> round of reviews has been completed.
>>> 
>>> Cheers,
>>> Mikael
>>> 
>>> [1] 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>>> 
> 

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (serviceability)

[Mikael Vidstedt]

> On May 6, 2020, at 3:19 PM, serguei.spit...@oracle.com wrote:
> 
> 
> 
> On 5/6/20 11:12, serguei.spit...@oracle.com 
> <mailto:serguei.spit...@oracle.com> wrote:
>> On 5/6/20 10:58, coleen.phillim...@oracle.com 
>> <mailto:coleen.phillim...@oracle.com> wrote:
>>> 
>>> 
>>> On 5/6/20 1:04 PM, serguei.spit...@oracle.com 
>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>> On 5/6/20 03:40, coleen.phillim...@oracle.com 
>>>> <mailto:coleen.phillim...@oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> On 5/6/20 2:09 AM, serguei.spit...@oracle.com 
>>>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>>>> On 5/5/20 17:04, Mikael Vidstedt wrote:
>>>>>>>> On May 5, 2020, at 4:48 PM, serguei.spit...@oracle.com 
>>>>>>>> <mailto:serguei.spit...@oracle.com> wrote:
>>>>>>>> 
>>>>>>>> Hi Mikael,
>>>>>>>> 
>>>>>>>> The fixes in webrev look good to me.
>>>>>>>> 
>>>>>>>> I've just noticed a couple of more serviceability related things can 
>>>>>>>> be missed.
>>>>>>>> (Not sure if they are included into different chunk of fixes.)
>>>>>>>> 
>>>>>>>> One is libjvm_db.so which is for Solaris Pstack support:
>>>>>>>>   make/hotspot/lib/CompileDtraceLibraries.gmk:# Note that 
>>>>>>>> libjvm_db.c has tests for COMPILER2, but this was never set by
>>>>>>>>   make/hotspot/lib/CompileDtraceLibraries.gmk: LIBJVM_DB_OUTPUTDIR := 
>>>>>>>> $(JVM_VARIANT_OUTPUTDIR)/libjvm_db
>>>>>>>>   make/hotspot/lib/CompileDtraceLibraries.gmk:NAME := jvm_db, \
>>>>>>>>   make/hotspot/lib/CompileDtraceLibraries.gmk:SRC := 
>>>>>>>> $(TOPDIR)/src/java.base/solaris/native/libjvm_db, \
>>>>>>>> 
>>>>>>>> Another is DTrace support which also includes jhelper.d (support for 
>>>>>>>> DTrace jstack action which is for Solaris only):
>>>>>>>>   make/hotspot/gensrc/GensrcDtrace.gmk
>>>>>>>>   make/hotspot/lib/JvmDtraceObjects.gmk
>>>>>>>> It also impacts some other make files.
>>>>>>> I believe you’ll find that these changes were included in the build 
>>>>>>> system patch:
>>>>>>> 
>>>>>>> https://mail.openjdk.java.net/pipermail/build-dev/2020-May/027342.html 
>>>>>>> <https://mail.openjdk.java.net/pipermail/build-dev/2020-May/027342.html>
>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/
>>>>>>>  
>>>>>>> <http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/>
>>>>>>> 
>>>>>>> Let me know if I missed something.
>>>>>> 
>>>>>> The file make/hotspot/src/native/dtrace/generateJvmOffsets.cpp is for 
>>>>>> Solaris only, and so, can be removed.
>>>>>> It is for libjvm_db.so (provider for Solaris Pstack) and jhelper.d 
>>>>>> (provider for Solaris DTrace jstack action).
>>>>>> The jstack action (prints mixed java+native stack traces) was never 
>>>>>> implemented other than for Solaris.
>>>>> 
>>>>> I wonder if this can be used to implement the same thing on linux and if 
>>>>> we can keep this?  Thoughts?
>>>> 
>>>> I was also thinking about it. And DTrace kind of exists on Mac OS as well.
>>>> Yes, it can be used. But It will require the jstack action implementation 
>>>> and the jhelper use on the DTrace side.
>>> 
>>>> The same is true for the libjvm_db.so. It could be used in the Linux 
>>>> Pstack utility to print mixed stack traces.
>>>> (I see some pstack projects like this: https://github.com/peadar/pstack 
>>>> <https://github.com/peadar/pstack> )
>>> Yes, I was thinking specifically of the pstack utility and not DTrace, just 
>>> to get Java frames in ptrace.  Could libjvm_db.so be used for that?
>> 
>> Yes, it can be.
>> The work needs to be done on the Linux side to get use of the libjvm_db.so.
>> We worked with the Solaris team in the past to make the Pstack to print java 
>> frames (including virtual ones).
>> The libjvm_db API came out from discussions with them.
>> The interface is pretty simple:
>> The Pstack does one initialization call and then in a loop requests the java 
>> frame strings from the register context (pc, sp and fp).
>> Each time the libjvm_db returns corrected register state to help with the 
>> stack walking steps.
>> It is kind of assisted stack walking.
> 
> We had a local chat with Mikael and Coleen on this.
> The conclusion is that we’ll remove it for now and restore it later if needed.
> This folder has to be also removed with this:
>   src/java.base/solaris/native/libjvm_db

Thank you Serguei & Coleen for the discussion! As we concluded the code will 
still be there in the history in case we need inspiration, leaving it in 
(without actually being used) will just lead to inevitable bit-rot.

Cheers,
Mikael

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (serviceability)

[Mikael Vidstedt]

To summarize the status:

I have not made any changes to the serviceability patch itself, but I did 
remove generateJvmOffsets.cpp as part of the build system patch. I believe the 
following items remain:

* File follow-up(s) to update comments

* File follow-up to rename the #ifdef guards in 
src/jdk/jdwp.agent/*/native/libjdwp/path_md.h

* File follow-up to remove "#undef CS” in 
src/hotspot/share/prims/methodHandles.cpp

* File follow-up to investigate what to do about dtrace going forward (on linux 
& mac)

Please let me know if I missed anything!

Cheers,
Mikael

> On May 3, 2020, at 10:12 PM, Mikael Vidstedt  
> wrote:
> 
> 
> Please review this change which implements part of JEP 381:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/serviceability/open/webrev/
> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
> 
> 
> Note: When reviewing this, please be aware that this exercise was *extremely* 
> mind-numbing, so I appreciate your help reviewing all the individual changes 
> carefully. You may want to get that coffee cup filled up (or whatever keeps 
> you awake)!
> 
> 
> Background:
> 
> Because of the size of the total patch and wide range of areas touched, this 
> patch is one out of in total six partial patches which together make up the 
> necessary changes to remove the Solaris and SPARC ports. The other patches 
> are being sent out for review to mailing lists appropriate for the respective 
> areas the touch. An email will be sent to jdk-dev summarizing all the 
> patches/reviews. To be clear: this patch is *not* in itself complete and 
> stand-alone - all of the (six) patches are needed to form a complete patch. 
> Some changes in this patch may look wrong or incomplete unless also looking 
> at the corresponding changes in other areas.
> 
> For convenience, I’m including a link below[1] to the full webrev, but in 
> case you have comments on changes in other areas, outside of the files 
> included in this thread, please provide those comments directly in the thread 
> on the appropriate mailing list for that area if possible.
> 
> In case it helps, the changes were effectively produced by searching for and 
> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
> More information about the areas impacted can be found in the JEP itself.
> 
> 
> Testing:
> 
> A slightly earlier version of this change successfully passed tier1-8, as 
> well as client tier1-2. Additional testing will be done after the first round 
> of reviews has been completed.
> 
> Cheers,
> Mikael
> 
> [1] 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
> 

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

New webrev here:

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot/open/webrev/
incremental: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/hotspot.incr/open/webrev/

Remaining items:

* File follow-up to remove STACK_BIAS

* File follow-ups to change/update/remove flags and/or flag documentation: 
UseLWPSynchronization, BranchOnRegister, LIRFillDelaySlots, 
ArrayAllocatorMallocLimit, ThreadPriorityPolicy

* File follow-up(s) to update comments ("solaris", “sparc”, “solstudio”, 
“sunos”, “sun studio”, “s compiler bug”, “niagara”, …)


Please let me know if there’s something I have missed!

Cheers,
Mikael

> On May 3, 2020, at 10:12 PM, Mikael Vidstedt  
> wrote:
> 
> 
> Please review this change which implements part of JEP 381:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
> 
> 
> Note: When reviewing this, please be aware that this exercise was *extremely* 
> mind-numbing, so I appreciate your help reviewing all the individual changes 
> carefully. You may want to get that coffee cup filled up (or whatever keeps 
> you awake)!
> 
> 
> Background:
> 
> Because of the size of the total patch and wide range of areas touched, this 
> patch is one out of in total six partial patches which together make up the 
> necessary changes to remove the Solaris and SPARC ports. The other patches 
> are being sent out for review to mailing lists appropriate for the respective 
> areas the touch. An email will be sent to jdk-dev summarizing all the 
> patches/reviews. To be clear: this patch is *not* in itself complete and 
> stand-alone - all of the (six) patches are needed to form a complete patch. 
> Some changes in this patch may look wrong or incomplete unless also looking 
> at the corresponding changes in other areas.
> 
> For convenience, I’m including a link below[1] to the full webrev, but in 
> case you have comments on changes in other areas, outside of the files 
> included in this thread, please provide those comments directly in the thread 
> on the appropriate mailing list for that area if possible.
> 
> In case it helps, the changes were effectively produced by searching for and 
> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
> More information about the areas impacted can be found in the JEP itself.
> 
> A big thank you to Igor Ignatyev for helping make the changes to the hotspot 
> tests!
> 
> Also, I have a short list of follow-ups which I’m going to look at separately 
> from this JEP/patch, mainly related to command line options/flags which are 
> no longer relevant and should be deprecated/obsoleted/removed.
> 
> Testing:
> 
> A slightly earlier version of this change successfully passed tier1-8, as 
> well as client tier1-2. Additional testing will be done after the first round 
> of reviews has been completed.
> 
> Cheers,
> Mikael
> 
> [1] 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
> 

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

Igor, thank you for the review, and again for helping make the test changes in 
the first place! :)

I hope Vladimir’s reply clarifies how we’re planning on handling the Graal 
related changes.

Cheers,
Mikael

> On May 4, 2020, at 2:29 PM, Igor Ignatyev  wrote:
> 
> Hi Mikael,
> 
> the changes in /test/ look good to me.
> 
> I have a question regarding src/jdk.internal.vm.compiler/*, aren't these 
> files part of graal-compiler and hence will be brought back by the next graal 
> update?
> 
> Thanks,
> -- Igor 
> 
>> On May 3, 2020, at 10:12 PM, Mikael Vidstedt  
>> wrote:
>> 
>> 
>> Please review this change which implements part of JEP 381:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>> 
>> 
>> Note: When reviewing this, please be aware that this exercise was 
>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>> individual changes carefully. You may want to get that coffee cup filled up 
>> (or whatever keeps you awake)!
>> 
>> 
>> Background:
>> 
>> Because of the size of the total patch and wide range of areas touched, this 
>> patch is one out of in total six partial patches which together make up the 
>> necessary changes to remove the Solaris and SPARC ports. The other patches 
>> are being sent out for review to mailing lists appropriate for the 
>> respective areas the touch. An email will be sent to jdk-dev summarizing all 
>> the patches/reviews. To be clear: this patch is *not* in itself complete and 
>> stand-alone - all of the (six) patches are needed to form a complete patch. 
>> Some changes in this patch may look wrong or incomplete unless also looking 
>> at the corresponding changes in other areas.
>> 
>> For convenience, I’m including a link below[1] to the full webrev, but in 
>> case you have comments on changes in other areas, outside of the files 
>> included in this thread, please provide those comments directly in the 
>> thread on the appropriate mailing list for that area if possible.
>> 
>> In case it helps, the changes were effectively produced by searching for and 
>> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
>> More information about the areas impacted can be found in the JEP itself.
>> 
>> A big thank you to Igor Ignatyev for helping make the changes to the hotspot 
>> tests!
>> 
>> Also, I have a short list of follow-ups which I’m going to look at 
>> separately from this JEP/patch, mainly related to command line options/flags 
>> which are no longer relevant and should be deprecated/obsoleted/removed.
>> 
>> Testing:
>> 
>> A slightly earlier version of this change successfully passed tier1-8, as 
>> well as client tier1-2. Additional testing will be done after the first 
>> round of reviews has been completed.
>> 
>> Cheers,
>> Mikael
>> 
>> [1] 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>> 
> 

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

Vladimir, thank you for the review!

Note that based on Stefan’s comments I have removed the decryptSuffix variable 
in 
src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
 in the upcoming webrev.

Cheers,
Mikael

> On May 4, 2020, at 12:01 PM, Vladimir Kozlov  
> wrote:
> 
> JIT, AOT, JVMCI and Graal changes seem fine to me.
> 
> It would be interesting to see shared code execution coverage change.  There 
> are places where we use flags and setting instead of #ifdef SPARC which may 
> not be executed now or executed partially. We may simplify such code too.
> 
> Thanks,
> Vladimir
> 
> On 5/3/20 10:12 PM, Mikael Vidstedt wrote:
>> Please review this change which implements part of JEP 381:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>> Note: When reviewing this, please be aware that this exercise was 
>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>> individual changes carefully. You may want to get that coffee cup filled up 
>> (or whatever keeps you awake)!
>> Background:
>> Because of the size of the total patch and wide range of areas touched, this 
>> patch is one out of in total six partial patches which together make up the 
>> necessary changes to remove the Solaris and SPARC ports. The other patches 
>> are being sent out for review to mailing lists appropriate for the 
>> respective areas the touch. An email will be sent to jdk-dev summarizing all 
>> the patches/reviews. To be clear: this patch is *not* in itself complete and 
>> stand-alone - all of the (six) patches are needed to form a complete patch. 
>> Some changes in this patch may look wrong or incomplete unless also looking 
>> at the corresponding changes in other areas.
>> For convenience, I’m including a link below[1] to the full webrev, but in 
>> case you have comments on changes in other areas, outside of the files 
>> included in this thread, please provide those comments directly in the 
>> thread on the appropriate mailing list for that area if possible.
>> In case it helps, the changes were effectively produced by searching for and 
>> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
>> More information about the areas impacted can be found in the JEP itself.
>> A big thank you to Igor Ignatyev for helping make the changes to the hotspot 
>> tests!
>> Also, I have a short list of follow-ups which I’m going to look at 
>> separately from this JEP/patch, mainly related to command line options/flags 
>> which are no longer relevant and should be deprecated/obsoleted/removed.
>> Testing:
>> A slightly earlier version of this change successfully passed tier1-8, as 
>> well as client tier1-2. Additional testing will be done after the first 
>> round of reviews has been completed.
>> Cheers,
>> Mikael
>> [1] 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

Kim, thank you for the review! Comments inline..


> On May 4, 2020, at 3:47 AM, Kim Barrett  wrote:
> 
>> On May 4, 2020, at 1:12 AM, Mikael Vidstedt  
>> wrote:
>> 
>> 
>> Please review this change which implements part of JEP 381:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
> 
> I've only looked at the src/hotspot changes so far. I've not
> duplicated comments already made by Stefan.
> 
> Looks good, other than a few very minor issues, some of which might
> already be covered by planned followup RFEs.
> 
> --
> 
> I think with sparc removal, c1's pack64/unpack64 stuff is no longer
> used.  So I think that can be removed from c1_LIR.[ch]pp too.

Good catch. Fixed.

> --
> src/hotspot/share/opto/generateOptoStub.cpp
> 225   // Clear last_Java_pc and (optionally)_flags
> 
> The sparc-specific clearing of "flags" is gone.

Fixed.

> --
> src/hotspot/share/runtime/deoptimization.cpp
> 1086   *((jlong *) check_alignment_get_addr(obj, index, 8)) = (jlong) 
> *((jlong *) );
> 
> [pre-existing]
> The rhs cast to jlong is unnecessary, since it's dereferencing a jlong*.

When I first updated the code I actually remove the cast, but it just ended up 
looking asymmetrical so I chose to leave it there. Let me know if you feel 
strongly that it should go. (I don’t like these casts in general).

> --
> src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp
> 236 JVMFlag::Error CompilerThreadPriorityConstraintFunc(intx value, bool 
> verbose) {
> 237   return JVMFlag::SUCCESS;
> 238 }
> 
> After SOLARIS code removal we no longer need this constraint function.

Fixed. (I had that on my follow-up list, but included it in the upcoming 
webrev.)

> --
> src/hotspot/share/runtime/globals.hpp
> 2392   experimental(size_t, ArrayAllocatorMallocLimit,
>\
> 2393   (size_t)-1,
>\
> 
> Combine these lines.

Fixed.

> --
> src/hotspot/share/utilities/dtrace.hpp
> 
> Shuold just eliminate all traces of HS_DTRACE_WORKAROUND_TAIL_CALL_BUG.

Fixed - more code removed!

Cheers,
Mikael

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

> On May 4, 2020, at 2:11 AM, Thomas Schatzl  wrote:
> 
> Hi,
> 
> On 04.05.20 10:28, Stefan Karlsson wrote:
>> Hi Mikael,
>> On 2020-05-04 07:12, Mikael Vidstedt wrote:
>>> 
>>> Please review this change which implements part of JEP 381:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>> webrev: 
>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
>>>  
>> I went over this patch and collected some comments:
>> src/hotspot/share/adlc/output_c.cpp
>> src/hotspot/share/adlc/output_h.cpp
>> Awkward code layout after change to.
>> src/hotspot/share/c1/c1_Runtime1.cpp
>> src/hotspot/share/classfile/classListParser.cpp
>> src/hotspot/share/memory/arena.hpp
>> src/hotspot/share/opto/chaitin.cpp
>> test/hotspot/jtreg/gc/TestCardTablePageCommits.jav >
> > Surrounding comments still refers to Sparc and/or Solaris.
> >
> > There are even more places if you search in the rest of the HotSpot
> > source. Are we leaving those for a separate cleanup pass?
> 
> In addition to "sparc", "solaris", also "solstudio"/"Sun Studio"/"SS compiler 
> bug"/"niagara" yield some search (=grep) results.
> 
> Some of these locations look like additional RFEs.

Ah good! I found and fixed a few additional places based on those strings, but 
would like to handling the remaining comment updates as RFEs.

Thank you for having a look!

Cheers,
Mikael

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

Thank you for the review! Comments inline..

> On May 4, 2020, at 1:28 AM, Stefan Karlsson  
> wrote:
> 
> Hi Mikael,
> 
> On 2020-05-04 07:12, Mikael Vidstedt wrote:
>> Please review this change which implements part of JEP 381:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
> 
> I went over this patch and collected some comments:
> 
> src/hotspot/share/adlc/output_c.cpp
> src/hotspot/share/adlc/output_h.cpp
> 
> Awkward code layout after change to.

Indeed - fixed!

> src/hotspot/share/c1/c1_Runtime1.cpp
> src/hotspot/share/classfile/classListParser.cpp
> src/hotspot/share/memory/arena.hpp
> src/hotspot/share/opto/chaitin.cpp
> test/hotspot/jtreg/gc/TestCardTablePageCommits.java
> 
> Surrounding comments still refers to Sparc and/or Solaris.
> 
> There are even more places if you search in the rest of the HotSpot source. 
> Are we leaving those for a separate cleanup pass?

Correct - I deliberately avoided changing comments that were not immediately 
“obvious” how to address and/or that were pre-existing issues, since it’s not 
necessarily wrong for a comment to refer to Solaris or SPARC even after these 
changes. I would prefer to do that as follow-ups. Fair?

> src/hotspot/share/gc/g1/g1HeapRegionAttr.hpp
> 
> Remove comment:
>  // We use different types to represent the state value depending on platform 
> as
>  // some have issues loading parts of words.

Fixed.

> src/hotspot/share/gc/shared/memset_with_concurrent_readers.hpp
> 
> Fuse the declaration and definition, now that we only have one 
> implementation. Maybe even remove function/file at some point.

Fixed (fused).

> src/hotspot/share/utilities/globalDefinitions.hpp
> 
> Now that STACK_BIAS is always 0, should we remove its usages? Follow-up RFE?

Yes, this is one of the things I have on my list to file a follow-up for.

> src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
> 
> Maybe remove decryptSuffix?

Fixed.

> src/utils/hsdis/Makefile
> 
> Is this really correct?
> 
> Shouldn't:
> ARCH1=$(CPU:x86_64=amd64)
> ARCH2=$(ARCH1:i686=i386)
> ARCH=$(ARCH2:sparc64=sparcv9)
> 
> be changed to:
> ARCH1=$(CPU:x86_64=amd64)
> ARCH=$(ARCH1:i686=i386)
> 
> so that we have ARCH defined?

Very good catch! This Makefile could use some indentation love or just a plain 
rewrite.. In either case I fixed the ARCH definition and tested it to make sure 
the end result seemed to do the right thing (and AFAICT it does).

> Other than that this looks good to me.

Thank you!

Cheers,
Mikael

> 
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>> Note: When reviewing this, please be aware that this exercise was 
>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>> individual changes carefully. You may want to get that coffee cup filled up 
>> (or whatever keeps you awake)!
>> Background:
>> Because of the size of the total patch and wide range of areas touched, this 
>> patch is one out of in total six partial patches which together make up the 
>> necessary changes to remove the Solaris and SPARC ports. The other patches 
>> are being sent out for review to mailing lists appropriate for the 
>> respective areas the touch. An email will be sent to jdk-dev summarizing all 
>> the patches/reviews. To be clear: this patch is *not* in itself complete and 
>> stand-alone - all of the (six) patches are needed to form a complete patch. 
>> Some changes in this patch may look wrong or incomplete unless also looking 
>> at the corresponding changes in other areas.
>> For convenience, I’m including a link below[1] to the full webrev, but in 
>> case you have comments on changes in other areas, outside of the files 
>> included in this thread, please provide those comments directly in the 
>> thread on the appropriate mailing list for that area if possible.
>> In case it helps, the changes were effectively produced by searching for and 
>> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
>> More information about the areas impacted can be found in the JEP itself.
>> A big thank you to Igor Ignatyev for helping make the changes to the hotspot 
>> tests!
>> Also, I have a short list of follow-ups which I’m going to look at 
>> separately from this JEP/patch, mainly related to command line options/flags 
>> which are no longer relevant and should be deprecated/obsoleted/removed.
>> Testing:
>> A slightly earlier version of this change successfully passed tier1-8, as 
>> well as client tier1-2. Additional testing will be done after the first 
>> round of reviews has been completed.
>> Cheers,
>> Mikael
>> [1] 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (serviceability)

[Mikael Vidstedt]

> On May 5, 2020, at 4:48 PM, serguei.spit...@oracle.com wrote:
> 
> Hi Mikael,
> 
> The fixes in webrev look good to me.
> 
> I've just noticed a couple of more serviceability related things can be 
> missed.
> (Not sure if they are included into different chunk of fixes.)
> 
> One is libjvm_db.so which is for Solaris Pstack support:
>   make/hotspot/lib/CompileDtraceLibraries.gmk:# Note that libjvm_db.c has 
> tests for COMPILER2, but this was never set by
>   make/hotspot/lib/CompileDtraceLibraries.gmk: LIBJVM_DB_OUTPUTDIR := 
> $(JVM_VARIANT_OUTPUTDIR)/libjvm_db
>   make/hotspot/lib/CompileDtraceLibraries.gmk:NAME := jvm_db, \
>   make/hotspot/lib/CompileDtraceLibraries.gmk:SRC := 
> $(TOPDIR)/src/java.base/solaris/native/libjvm_db, \
> 
> Another is DTrace support which also includes jhelper.d (support for DTrace 
> jstack action which is for Solaris only):
>   make/hotspot/gensrc/GensrcDtrace.gmk
>   make/hotspot/lib/JvmDtraceObjects.gmk
> It also impacts some other make files.

I believe you’ll find that these changes were included in the build system 
patch:

https://mail.openjdk.java.net/pipermail/build-dev/2020-May/027342.html
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/

Let me know if I missed something.

> Also, these lines can be just removed:
>   open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#ifndef 
> _JAVASOFT_SOLARIS_PATH_MD_H_
>   open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#define 
> _JAVASOFT_SOLARIS_PATH_MD_H_
>   open/src/jdk.jdwp.agent/unix/native/libjdwp/path_md.h:#endif /* 
> !_JAVASOFT_SOLARIS_PATH_MD_H_ */

Remove or rather rename? The corresponding Windows header also has the guard, 
any particular reason why it should be *removed*?

Cheers,
Mikael

> 
> Thanks,
> Serguei
> 
> 
> On 5/5/20 14:34, Mikael Vidstedt wrote:
>> All good points! I deliberately chose *not* to update comments where it 
>> wasn’t immediately 100% obvious exactly how to update them. For example, in 
>> many cases I found that the comments are already incomplete or stale, and 
>> for each such case we’ll want to consider how exactly to update the comment 
>> (remove/switch to Unix/etc.). I would prefer to handle this as follow-up(s), 
>> separate from the JEP. Does that sounds reasonable?
>> 
>> Apart from the comments - do the changes look good? If so, can I count you 
>> as a reviewer?
>> 
>> Cheers,
>> Mikael
>> 
>> 
>>> On May 4, 2020, at 12:20 AM, serguei.spit...@oracle.com wrote:
>>> 
>>> HI Mikael,
>>> 
>>> Some quick comments.
>>> 
>>> Some extra references to Solaris/solaris, SunOS or SPARC are listed below:
>>> 
>>> src/java.instrument/unix/native/libinstrument/FileSystemSupport_md.c (2 
>>> refs to Solaris/solaris)
>>> src/java.management/share/classes/javax/management/loading/MLet.java (refs 
>>> to Solaris, SPARC/sparc and SunOS)
>>> src/jdk.management.agent/unix/classes/jdk/internal/agent/FileSystemImpl.java
>>>  (ref to Solaris)
>>> 
>>> src/hotspot/share/prims/forte.cpp:// Solaris SPARC Compiler1 needs an 
>>> additional check on the grandparent
>>> src/hotspot/share/prims/forte.cpp:// on Linux X86, Solaris SPARC and 
>>> Solaris X86.
>>> src/hotspot/share/prims/jvmti.xml:On the Solaris Operating 
>>> Environment, an agent library is a shared
>>> src/hotspot/share/prims/jvmti.xml: LD_LIBRARY_PATH under the 
>>> Solaris operating
>>> src/hotspot/share/prims/jvmti.xml:  example, in the Java 2 SDK a 
>>> CTRL-Break on Win32 and a CTRL-\ on Solaris
>>> src/hotspot/share/prims/methodHandles.cpp:#undef CS  // Solaris builds 
>>> complain
>>> 
>>> 
>>> Thanks,
>>> Serguei
>>> 
>>> 
>>> On 5/3/20 22:12, Mikael Vidstedt wrote:
>>>> Please review this change which implements part of JEP 381:
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>>> webrev: 
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/serviceability/open/webrev/
>>>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>>>> 
>>>> 
>>>> Note: When reviewing this, please be aware that this exercise was 
>>>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>>>> individual changes carefully. You may want to get that coffee cup filled 
>>>> up (or whatever keeps you awake)!
>>>> 
>>>> 
>>>> Background:
>>>> 
>

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (serviceability)

[Mikael Vidstedt]

All good points! I deliberately chose *not* to update comments where it wasn’t 
immediately 100% obvious exactly how to update them. For example, in many cases 
I found that the comments are already incomplete or stale, and for each such 
case we’ll want to consider how exactly to update the comment (remove/switch to 
Unix/etc.). I would prefer to handle this as follow-up(s), separate from the 
JEP. Does that sounds reasonable?

Apart from the comments - do the changes look good? If so, can I count you as a 
reviewer?

Cheers,
Mikael


> On May 4, 2020, at 12:20 AM, serguei.spit...@oracle.com wrote:
> 
> HI Mikael,
> 
> Some quick comments.
> 
> Some extra references to Solaris/solaris, SunOS or SPARC are listed below:
> 
> src/java.instrument/unix/native/libinstrument/FileSystemSupport_md.c (2 refs 
> to Solaris/solaris)
> src/java.management/share/classes/javax/management/loading/MLet.java (refs to 
> Solaris, SPARC/sparc and SunOS)
> src/jdk.management.agent/unix/classes/jdk/internal/agent/FileSystemImpl.java 
> (ref to Solaris)
> 
> src/hotspot/share/prims/forte.cpp:// Solaris SPARC Compiler1 needs an 
> additional check on the grandparent
> src/hotspot/share/prims/forte.cpp:// on Linux X86, Solaris SPARC and Solaris 
> X86.
> src/hotspot/share/prims/jvmti.xml:On the Solaris Operating 
> Environment, an agent library is a shared
> src/hotspot/share/prims/jvmti.xml: LD_LIBRARY_PATH under the 
> Solaris operating
> src/hotspot/share/prims/jvmti.xml:  example, in the Java 2 SDK a 
> CTRL-Break on Win32 and a CTRL-\ on Solaris
> src/hotspot/share/prims/methodHandles.cpp:#undef CS  // Solaris builds 
> complain
> 
> 
> Thanks,
> Serguei
> 
> 
> On 5/3/20 22:12, Mikael Vidstedt wrote:
>> Please review this change which implements part of JEP 381:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/serviceability/open/webrev/
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>> 
>> 
>> Note: When reviewing this, please be aware that this exercise was 
>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>> individual changes carefully. You may want to get that coffee cup filled up 
>> (or whatever keeps you awake)!
>> 
>> 
>> Background:
>> 
>> Because of the size of the total patch and wide range of areas touched, this 
>> patch is one out of in total six partial patches which together make up the 
>> necessary changes to remove the Solaris and SPARC ports. The other patches 
>> are being sent out for review to mailing lists appropriate for the 
>> respective areas the touch. An email will be sent to jdk-dev summarizing all 
>> the patches/reviews. To be clear: this patch is *not* in itself complete and 
>> stand-alone - all of the (six) patches are needed to form a complete patch. 
>> Some changes in this patch may look wrong or incomplete unless also looking 
>> at the corresponding changes in other areas.
>> 
>> For convenience, I’m including a link below[1] to the full webrev, but in 
>> case you have comments on changes in other areas, outside of the files 
>> included in this thread, please provide those comments directly in the 
>> thread on the appropriate mailing list for that area if possible.
>> 
>> In case it helps, the changes were effectively produced by searching for and 
>> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
>> More information about the areas impacted can be found in the JEP itself.
>> 
>> 
>> Testing:
>> 
>> A slightly earlier version of this change successfully passed tier1-8, as 
>> well as client tier1-2. Additional testing will be done after the first 
>> round of reviews has been completed.
>> 
>> Cheers,
>> Mikael
>> 
>> [1] 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
>> 
> 

RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Mikael Vidstedt]

Please review this change which implements part of JEP 381:

JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/
JEP: https://bugs.openjdk.java.net/browse/JDK-8241787


Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


Background:

Because of the size of the total patch and wide range of areas touched, this 
patch is one out of in total six partial patches which together make up the 
necessary changes to remove the Solaris and SPARC ports. The other patches are 
being sent out for review to mailing lists appropriate for the respective areas 
the touch. An email will be sent to jdk-dev summarizing all the 
patches/reviews. To be clear: this patch is *not* in itself complete and 
stand-alone - all of the (six) patches are needed to form a complete patch. 
Some changes in this patch may look wrong or incomplete unless also looking at 
the corresponding changes in other areas.

For convenience, I’m including a link below[1] to the full webrev, but in case 
you have comments on changes in other areas, outside of the files included in 
this thread, please provide those comments directly in the thread on the 
appropriate mailing list for that area if possible.

In case it helps, the changes were effectively produced by searching for and 
updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
More information about the areas impacted can be found in the JEP itself.

A big thank you to Igor Ignatyev for helping make the changes to the hotspot 
tests!

Also, I have a short list of follow-ups which I’m going to look at separately 
from this JEP/patch, mainly related to command line options/flags which are no 
longer relevant and should be deprecated/obsoleted/removed.

Testing:

A slightly earlier version of this change successfully passed tier1-8, as well 
as client tier1-2. Additional testing will be done after the first round of 
reviews has been completed.

Cheers,
Mikael

[1] 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (serviceability)

[Mikael Vidstedt]

Please review this change which implements part of JEP 381:

JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/serviceability/open/webrev/
JEP: https://bugs.openjdk.java.net/browse/JDK-8241787


Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


Background:

Because of the size of the total patch and wide range of areas touched, this 
patch is one out of in total six partial patches which together make up the 
necessary changes to remove the Solaris and SPARC ports. The other patches are 
being sent out for review to mailing lists appropriate for the respective areas 
the touch. An email will be sent to jdk-dev summarizing all the 
patches/reviews. To be clear: this patch is *not* in itself complete and 
stand-alone - all of the (six) patches are needed to form a complete patch. 
Some changes in this patch may look wrong or incomplete unless also looking at 
the corresponding changes in other areas.

For convenience, I’m including a link below[1] to the full webrev, but in case 
you have comments on changes in other areas, outside of the files included in 
this thread, please provide those comments directly in the thread on the 
appropriate mailing list for that area if possible.

In case it helps, the changes were effectively produced by searching for and 
updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
More information about the areas impacted can be found in the JEP itself.


Testing:

A slightly earlier version of this change successfully passed tier1-8, as well 
as client tier1-2. Additional testing will be done after the first round of 
reviews has been completed.

Cheers,
Mikael

[1] 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

Re: RFR(T) : 8243946 : serviceability/sa tests fail after JDK-8243928

[Mikael Vidstedt]

Looks good, thanks for the quick turnaround!

Cheers,
Mikael

> On Apr 27, 2020, at 10:02 PM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
>> 2 lines changed: 0 ins; 0 del; 2 mod;
> 
> Hi all,
> 
> (so now it's my time to apology for inconvenience)
> 
> could you please review this small follow-up fix/partial revert of 
> 8243928[1]? serviceability/sa/TestCpoolForInvokeDynamic.java  and 
> TestDefaultMethods.java test use non-exported API so they can't be run in 
> driver mode (b/c jtreg use vanilla JVM for driver code, meaning even exports 
> from @modules tags are ignored)
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8243946
> testing: test/hotspot/jtreg/serviceability (in progress)
> webrev: http://cr.openjdk.java.net/~iignatyev//8243946/webrev.00
> 
> [1]  https://bugs.openjdk.java.net/browse/JDK-8243928
> 
> Thanks,
> -- Igor

Re: RFR(XS) 8241335: ProblemList serviceability/sa/ClhsdbPstack.java due to JDK-8240956

[Mikael Vidstedt]

Looks good, thanks for doing this!

Cheers,
Mikael

> On Mar 19, 2020, at 8:25 PM, Chris Plummer  wrote:
> 
> Hello,
> 
> Please review the following:
> 
> diff --git a/test/hotspot/jtreg/ProblemList.txt 
> b/test/hotspot/jtreg/ProblemList.txt
> --- a/test/hotspot/jtreg/ProblemList.txt
> +++ b/test/hotspot/jtreg/ProblemList.txt
> @@ -115,7 +115,7 @@
>  serviceability/sa/ClhsdbPrintAll.java 8193639 solaris-all
>  serviceability/sa/ClhsdbPrintAs.java 8193639 solaris-all
>  serviceability/sa/ClhsdbPrintStatics.java 8193639 solaris-all
> -serviceability/sa/ClhsdbPstack.java 8193639 solaris-all
> +serviceability/sa/ClhsdbPstack.java 8193639,8240956 solaris-all,linux-all
>  serviceability/sa/ClhsdbRegionDetailsScanOopsForG1.java 8193639 solaris-all
>  serviceability/sa/ClhsdbScanOops.java 8193639,8235220,8230731 
> solaris-all,linux-x64,macosx-x64,windows-x64
>  serviceability/sa/ClhsdbSource.java 8193639 solaris-all
> 
> I'm still waiting for a tier1 run to make sure the test isn't run on linux. 
> I'll push once that is done if I've had a review by then.
> 
> thanks,
> 
> Chris
> 

Re: RFR(T): 8240132: ProblemList com/sun/jdi/InvokeHangTest.java

[Mikael Vidstedt]

Looks good, thanks for doing it!

Cheers,
Mikael

> On Feb 26, 2020, at 2:49 PM, Daniel D. Daugherty 
>  wrote:
> 
> Greetings,
> 
> I'm trying to reduce the noise in the jdk/jdk CI. I'm ProblemListing
> com/sun/jdi/InvokeHangTest.java on Linux due to this bug:
> 
> JDK-8218463 com/sun/jdi/InvokeHangTest.java fail "java.lang.Exception: 
> InvokeHangTest: failed; bkpts = 64 "
> https://bugs.openjdk.java.net/browse/JDK-8218463
> 
> I'm using the following subtask to do the ProblemListing:
> 
> JDK-8240132 ProblemList com/sun/jdi/InvokeHangTest.java
> https://bugs.openjdk.java.net/browse/JDK-8240132
> 
> Here's the context diff:
> 
> $ hg diff
> diff -r f67951f722a4 test/jdk/ProblemList.txt
> --- a/test/jdk/ProblemList.txtWed Feb 26 21:24:02 2020 +0100
> +++ b/test/jdk/ProblemList.txtWed Feb 26 17:45:04 2020 -0500
> @@ -915,6 +915,8 @@
> 
>  com/sun/jdi/NashornPopFrameTest.java 8225620 generic-all
> 
> +com/sun/jdi/InvokeHangTest.java 8218463 linux-all
> +
>  
> 
>  # jdk_time
> 
> Thanks, in advance, for any comments, questions or suggestions.
> 
> Dan

Re: RFR (S) 8224796: C code is not compiled correctly due to undefined "i386"

[Mikael Vidstedt]

Looks good.

The if-define-to-code ratio in LinuxDebuggerLocal.c is impressively high, in a 
bad way. An enhancement to cover cleaning it up sure wouldn’t hurt.

Cheers,
Mikael

> On May 28, 2019, at 12:39 AM, Aleksey Shipilev  wrote:
> 
> On 5/27/19 12:36 PM, Aleksey Shipilev wrote:
>> Bug:
>>  https://bugs.openjdk.java.net/browse/JDK-8224796
>> 
>> x86_32 tier1 tests time out (actually, fail) because of this.
>> 
>> In short, recent change to compile C code with --std=c99 got "i386" 
>> undefined. Build system still
>> sets "i586", and we should really use that. I don't want to regress stuff 
>> much by globally replacing
>> i386->i586, so the new code handles *both* defines. This is what "handle 
>> both x86_64 and amd64"
>> block in LinuxDebuggerLocal.c already does.
>> 
>> I have not seen the failures on Mac due to ps_core.c, but it is better to be 
>> safe there as well.
>> 
>> Fix:
>>  http://cr.openjdk.java.net/~shade/8224796/webrev.01/
> 
> David had reviewed. More reviews, please?
> 
> -- 
> Thanks,
> -Aleksey
> 

Re: RFR (XS) 8213246: Fix typo in vmTestbase failuire to failure

[Mikael Vidstedt]

Looks good.

Cheers,
Mikael - not a Reviewer

> On Nov 2, 2018, at 4:05 PM, JC Beyler  wrote:
> 
> Hi all,
> 
> Could I get a review for a trivial typo fix:
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8213246/webrev.00/ 
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8213246 
> 
> 
> Thanks,
> Jc

Re: RFR (S) 8210842: Handle JNIGlobalRefLocker.cpp

[Mikael Vidstedt]

Sorry for being late to the game. Can I request a helpful comment in 
SafeJNIEnv.hpp describing what its purpose is?

Also, I don’t necessarily have a better suggestion (and don’t consider this 
blocking), but Is there another word than “Safe” to describe this wrapper? 
“Checked”? ExceptionChecking? Just something to consider.

Cheers,
Mikael

> On Sep 26, 2018, at 8:52 PM, JC Beyler  wrote:
> 
> Ping on this, anybody have time to do a review and give a LGTM? Or David if 
> you have time and will to provide an explicit LGTM :)
> 
> Thanks,
> Jc
> 
> On Mon, Sep 24, 2018 at 9:18 AM JC Beyler  > wrote:
> Hi David,
> 
> Sounds good to me, Alex gave me one LGTM, so it seems I'm waiting for an 
> explicit LGTM from you or from someone else on this list to do a review.
> 
> Thanks again for your help,
> Jc
> 
> On Sun, Sep 23, 2018 at 9:22 AM David Holmes  > wrote:
> Hi Jc,
> 
> I don't think it is an issue for this to go ahead. If the static 
> analysis tool has an issue then we can either find out how to teach it 
> about this code structure, or else flag the issues as false positives. 
> I'd be relying on one of the serviceability team here to handle that if 
> the problem arises.
> 
> Thanks,
> David
> 
> On 23/09/2018 12:17 PM, JC Beyler wrote:
> > Hi David,
> > 
> > No worries with the time to answer; I appreciate the review!
> > 
> > That's a fair point. Is it possible to "describe" to the static analysis 
> > tool to "trust" calls made in the SafeJNIEnv class?
> > 
> > Otherwise, do you have any idea on how to go forward? For what it's 
> > worth, this current webrev does not try to replace exception checking 
> > with the SafeJNIEnv (it actually adds it), so for now the 
> > question/solution could be delayed. I could continue working on this in 
> > the scope of both the nsk/gc/lock/jniref code base and the NSK_VERIFIER 
> > macro removal and we can look at how to tackle the cases where the tests 
> > are actually calling exception checking (I know my heapmonitor does for 
> > example).
> > 
> > Let me know what you think and thanks for the review,
> > Jc
> > 
> > 
> > On Sun, Sep 23, 2018 at 6:48 AM David Holmes  >  
> > >> wrote:
> > 
> > Hi Jc,
> > 
> > Sorry for the delay on getting back to this but I'm travelling at the
> > moment.
> > 
> > This looks quite neat. Thanks also to Alex for all the suggestions.
> > 
> > My only remaining concern is that static analysis tools may not like
> > this because they may not be able to determine that we won't make
> > subsequent JNI calls when an exception happens in the first. That's not
> > a reason not to do this of course, just flagging that we may have to do
> > something to deal with that problem.
> > 
> > Thanks,
> > David
> > 
> > On 20/09/2018 11:56 AM, JC Beyler wrote:
> >  > Hi Alex,
> >  >
> >  > Done here, thanks for the review:
> >  >
> >  > Webrev: http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.03/ 
> > 
> >  > >
> >  >  > >
> >  > Bug: https://bugs.openjdk.java.net/browse/JDK-8210842 
> > 
> >  >
> >  > Thanks again!
> >  > Jc
> >  >
> >  >
> >  > On Wed, Sep 19, 2018 at 5:13 PM Alex Menkov
> > mailto:alexey.men...@oracle.com> 
> > >
> >  > 
> >  > wrote:
> >  >
> >  > Hi Jc,
> >  >
> >  > Looks good to me.
> >  > A minor note:
> >  > - I'd move ErrorHandler typedef to SafeJNIEnv class to avoid
> > global
> >  > namespece pollution (ErrorHandler is too generic name).
> >  >
> >  > --alex
> >  >
> >  > On 09/19/2018 15:56, JC Beyler wrote:
> >  >  > Hi Alex,
> >  >  >
> >  >  > I've updated the webrev to:
> >  >  > Webrev:
> > http://cr.openjdk.java.net/~jcbeyler/8210842/webrev.02/ 
> > 
> >  > >
> >  >  > >
> >  >  >  > 

Re: RFR: (trivial) 8210486: Unused code in check_nest_attributes function

[Mikael Vidstedt]

Looks good. Thanks for fixing!

Cheers,
Mikael


> On Sep 7, 2018, at 1:51 AM, David Holmes  wrote:
> 
> Thanks to Mikael V. for finding this. :)
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210486
> Webrev: http://cr.openjdk.java.net/~dholmes/8210486/webrev/
> 
> Trivial code cleanup of unused variable:
> 
> --- old/src/hotspot/share/prims/jvmtiRedefineClasses.cpp  2018-09-07 
> 04:39:40.369608675 -0400
> +++ new/src/hotspot/share/prims/jvmtiRedefineClasses.cpp  2018-09-07 
> 04:39:38.797517530 -0400
> @@ -699,7 +699,6 @@
>   // Check whether the class NestHost attribute has been changed.
>   Thread* thread = Thread::current();
>   ResourceMark rm(thread);
> -  JvmtiThreadState *state = JvmtiThreadState::state_for((JavaThread*)thread);
>   u2 the_nest_host_idx = the_class->nest_host_index();
>   u2 scr_nest_host_idx = scratch_class->nest_host_index();
> 
> Testing:
>  - jdk/com/sun/jdi
>  - jdk/java/lang/instrument
>  - hotspot/runtime/Redefine*
>  - hotspot/runtime/appcds/redefineClass
>  - tiers 1-3 (mach5)
> 
> Thanks,
> David
> 
> 

Re: RFR(XXS) : 8210108 : sun/tools/jstatd test build failures after JDK-8210022

[Mikael Vidstedt]

Looks good, thanks for fixing!

Cheers,
Mikael

> On Aug 28, 2018, at 11:57 AM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev//8210108/webrev.00/index.html
>> 3 lines changed: 1 ins; 1 del; 1 mod; 
> 
> 
> Hi all,
> 
> could you please review this small and trivial follow-up fix?
> 
> unfortunately, test/jdk/sun/tools/jstatd/JstatdTest now uses two different 
> OutputAnalyzer classes, jdk.test.lib.process.OutputAnalyzer which it gets 
> from jdk.test.lib.process.ProcessThread, and jdk.testlibrary.OutputAnalyzer 
> from jdk.testlibrary.ProcessTools. as JstatdTest uses j.t.OutputAnalyzer more 
> often, the fix restores import statement and uses FQDN where 
> j.t.l.process.OutputAnalyzer is used.
> 
> // we will get rid of this FQDN, as soon as jdk.testlibrary.ProcessTools is 
> "merged" w/ jdk.t.l.process.ProcessTools, which is tracked by 8210112.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8210108
> webrev: http://cr.openjdk.java.net/~iignatyev//8210108/webrev.00/index.html
> testing: sun/tools/jstatd tests
> 
> Thanks,
> -- Igor

Re: RFR: JDK-8209608: Problem list com/sun/jdi/BreakpointWithFullGC.java

[Mikael Vidstedt]

Looks good.

Cheers,
Mikael

> On Aug 16, 2018, at 3:20 PM, Alex Menkov  wrote:
> 
> Hi all,
> 
> please review small change to problemlist BreakpointWithFullGC.java
> until JDK-8209605 is fixed.
> 
> #
> diff -r 4e9667589c43 test/jdk/ProblemList.txt
> --- a/test/jdk/ProblemList.txt  Thu Aug 16 17:29:22 2018 -0400
> +++ b/test/jdk/ProblemList.txt  Thu Aug 16 15:14:43 2018 -0700
> @@ -836,6 +836,8 @@
> 
> com/sun/jdi/BasicJDWPConnectionTest.java 8195703 generic-all
> 
> +com/sun/jdi/BreakpointWithFullGC.java   8209605 
> generic-all
> +
> com/sun/jdi/RedefineImplementor.sh 8004127 generic-all
> 
> com/sun/jdi/JdbExprTest.sh 8203393 solaris-all
> #
> 
> --alex

Re: RFR - JDK-8209517: com/sun/jdi/BreakpointWithFullGC.java fails with timeout

[Mikael Vidstedt]

Thanks for fixing this Alex! I’ve skimmed through the changes and AFAICT it all 
looks good.

One *very* minor thing (feel free to completely ignore this) - there’s a mix of 
“debugee’ (one ‘g’) and “debuggee” (two ‘g’:s). :)

Cheers,
Mikael

> On Aug 15, 2018, at 3:42 PM, Alex Menkov  wrote:
> 
> Hi all,
> 
> please review a fix for
> https://bugs.openjdk.java.net/browse/JDK-8209517
> webrev:
> http://cr.openjdk.java.net/~amenkov/sh2java/step1_regression/webrev/
> 
> Cause of the BreakpointWithFullGC failures is a mess of jdb and debuggee 
> outputs (the test runs debuggee by using CommandLineLaunch connector, so jdb 
> redirects debuggee stdout to its own stdout).
> To solve it test framework was updated to launch debuggee first (redirecting 
> its output) and then connecting jdb to existing process.
> The approach allow to drop "simple prompt" logic (jdb mode when debugee is 
> not yet running).
> 
> Mach5 passed 400 runs ("4 std platforms" x "--test-repeat 100") without any 
> issues.
> 
> --alex

RFR(XS): 8207217: Problem list java/lang/management/ThreadMXBean/AllThreadIds.java

[Mikael Vidstedt]

Please review this change which problem lists the frequently failing 
java/lang/management/ThreadMXBean/AllThreadIds.java test until the issue[1] has 
been fixed:

Bug: https://bugs.openjdk.java.net/browse/JDK-8207217 

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8207217/webrev.00/open/webrev/ 


Cheers,
Mikael

[1] https://bugs.openjdk.java.net/browse/JDK-8131745 

Re: (11) RFR (XS): 8205966: [testbug] New Nestmates JDI test times out with Xcomp on sparc

[Mikael Vidstedt]

Looks good. Nice speedup!

Cheers,
Mikael


> On Jul 5, 2018, at 10:21 PM, David Holmes  wrote:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8205966
> webrev: http://cr.openjdk.java.net/~dholmes/8205966/webrev/
> 
> One of the @run variants was taking around 15x longer to execute. That 
> variant uses the InMemoryJavaCompiler which involves a lot of classes and 
> code execution. The test was enabling method entry event generation for all 
> of main, resulting in the massive slowdown.
> 
> The fix is to add a new breakpoint() function that gets called after the 
> in-memory compilation setup is done, and we initially run the test to that 
> point before enabling the events.
> 
> The problem @run now only takes 2x the other tests and so should avoid the 
> timeouts.
> 
> Testing: mach5 tier4 solaris-sparc
> mach5 tier 1-3
> 
> Thanks,
> David

Re: RFR: 8036132 Tab characters in test/com/sun/jdi files

[Mikael Vidstedt]

I randomly chose a file and ended up looking at 
GetLocalVariables4Test.sh where the indentation of all the lines in 
main() appear to be off - the tab should have been translated to 8 
spaces in this case, not 4?


Cheers,
Mikael

On 2014-03-03 05:30, Staffan Larsen wrote:

Just changing a lot of tabs into a lot more spaces. Easiest to view in the 
patch file.

webrev: http://cr.openjdk.java.net/~sla/8036132/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8036132

Thanks,
/Staffan

Re: RFR: 8036132 Tab characters in test/com/sun/jdi files

[Mikael Vidstedt]

Looks good.

Cheers,
Mikael

On 2014-03-03 23:24, Staffan Larsen wrote:

Missed one more place in CatchPatternTest.sh: 
http://cr.openjdk.java.net/~sla/8036132/webrev.02/

/Staffan

On 4 mar 2014, at 08:09, Staffan Larsen staffan.lar...@oracle.com wrote:


Thanks for noticing! The same mistake was in all the inlined Java code.

new webrev: http://cr.openjdk.java.net/~sla/8036132/webrev.01/

/Staffan

On 3 mar 2014, at 21:04, Mikael Vidstedt mikael.vidst...@oracle.com wrote:


I randomly chose a file and ended up looking at GetLocalVariables4Test.sh where 
the indentation of all the lines in main() appear to be off - the tab should 
have been translated to 8 spaces in this case, not 4?

Cheers,
Mikael

On 2014-03-03 05:30, Staffan Larsen wrote:

Just changing a lot of tabs into a lot more spaces. Easiest to view in the 
patch file.

webrev: http://cr.openjdk.java.net/~sla/8036132/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8036132

Thanks,
/Staffan

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-23 01:19, Alan Bateman wrote:

On 19/02/2014 18:22, Mikael Vidstedt wrote:

:

The documented grammar in the comment only mentions SPACE and the 
code below doesn't make any references to \t. As a matter of fact, it 
only checks for one single, mandatory SPACE after the colon (enforced 
at line 535-536) and doesn't care to remove any space characters at 
the end of the value. The while loop only deals with continuations. 
If additional spaces do exist they will as far as I can tell be part 
of the value. Are they trimmed later? I'm assuming it would be nice 
to have both parsers (parse_manifest  JarFacade) behave the same way?


Here's what it would look like to only check for space, but still eat 
any additional spaces which doesn't match what 
parse_manifest/parse_nv_pair does:


http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.01/webrev/

Sorry for the delay getting back to you on this (I've been busy with 
other things).


I checked the JAR File Specification, which is turn references RFC 822 
as the inspiration for the name-value pairs. The SPACE token is just 
ASCII SP. So I agree it's just ASCII SP that needs to be handled here, 
not LWSP-char which includes ASCII HT.


Looking at JDK-6274276 then the trimming was done to avoid 
hard-to-diagnose problems with leading/trailing spaces. It's possible 
that this is inconsistent with other areas where JAR file attributes 
are used. I would suggest leaving it as is for now as this is 
potentially changing behavior in several areas.


That sounds reasonable. I'll keep the webrev.01 approach - only check 
for and trim ASCII SP.


Thanks for the review!

Cheers,
Mikael

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-19 09:07, Alan Bateman wrote:

On 18/02/2014 19:45, Mikael Vidstedt wrote:


That makes sense, and in fact parse_manifest.c does not even appear 
to allow for \t, so I'm more and more starting to think that a 
reasonable implementation in this context would be:


static int isNormalSpace(int c) { return c == ' '; }

In which case it probably shouldn't even be a separate function to 
start with. I would like to get a second opinion on the implications 
of only checking for ' ' (0x20) though.


If we want to allow both ' ' and \t we should probably call the 
function isblankAscii.
Thanks again for taking this. On \t then if it's nor handled by the 
parsing code then isNormalSpace should be fine.


Since I'm not exactly an expert on the code in question I would 
certainly appreciate it if somebody could verify me on that. I'm looking 
at parse_nv_pair (lines 430-542) in:


http://hg.openjdk.java.net/jdk9/dev/jdk/file/c766ec3e4877/src/share/bin/parse_manifest.c

The documented grammar in the comment only mentions SPACE and the code 
below doesn't make any references to \t. As a matter of fact, it only 
checks for one single, mandatory SPACE after the colon (enforced at line 
535-536) and doesn't care to remove any space characters at the end of 
the value. The while loop only deals with continuations. If additional 
spaces do exist they will as far as I can tell be part of the value. Are 
they trimmed later? I'm assuming it would be nice to have both parsers 
(parse_manifest  JarFacade) behave the same way?


Here's what it would look like to only check for space, but still eat 
any additional spaces which doesn't match what 
parse_manifest/parse_nv_pair does:


http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.01/webrev/

Cheers,
Mikael

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-18 00:33, Alan Bateman wrote:

On 18/02/2014 03:59, Mikael Vidstedt wrote:


How about:

http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.00/webrev/

Cheers,
Mikael

I checked the java.lang.instrument spec and for the Boot-Class-Path 
attribute then it doesn't say any more than space. It might be worth 
checking the manifest parsing code (parse_manfiest.c) to see how 
continuations are handled as I suspect \r and \n can't appear in the 
attribute value (in which case the check might really only need to be 
for space and \t.


That makes sense, and in fact parse_manifest.c does not even appear to 
allow for \t, so I'm more and more starting to think that a reasonable 
implementation in this context would be:


static int isNormalSpace(int c) { return c == ' '; }

In which case it probably shouldn't even be a separate function to start 
with. I would like to get a second opinion on the implications of only 
checking for ' ' (0x20) though.


If we want to allow both ' ' and \t we should probably call the function 
isblankAscii.


Otherwise replacing isspace is good and your isspaceAscii is likely to 
match the libc isspace (at runtime). This code isn't performance 
sensitive but maybe check space first would be a bit better. Also the 
library native code using 4 space indent rather than hotspot's 2.


Will fix indentation. I seriously doubt that the performance difference 
warrants the more complicated code.


I created JDK-8035054 a few days ago to track this. Thanks for taking 
it as I am busy with a number of other things at the moment.


Always for you, sir! ;)

/Mikael

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-17 07:08, Alan Bateman wrote:

On 17/02/2014 05:51, Mikael Vidstedt wrote:


I'm inclined to agree with this. Since the code depends on a specific 
behavior of isspace which does not match what the system provided 
function does I too think it would be more robust to implement our 
own version of it.
I completely agree that changing this code to use its own isspace is 
the right thing, it just seems a bit much for a drive-by fixed to gcc 
warnings. Do either of you want to take it?


How about:

http://cr.openjdk.java.net/~mikael/webrevs/isspace/webrev.00/webrev/

Cheers,
Mikael

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

I'm inclined to agree with this. Since the code depends on a specific behavior 
of isspace which does not match what the system provided function does I too 
think it would be more robust to implement our own version of it.

Cheers,
Mikael

 On Feb 16, 2014, at 14:20, Martin Buchholz marti...@google.com wrote:
 
 Those locale-dependent APIs - more trouble than they're worth.  More often 
 than not, you want a locale-independent version.
 
 So just define your own is_ASCII_space etc. like everybody else has done and 
 move on.
 
 
 On Sun, Feb 16, 2014 at 9:20 AM, Alan Bateman alan.bate...@oracle.com 
 wrote:
 On 13/02/2014 21:14, Mikael Vidstedt wrote:
 :
 
 The change in question appears to come from 
 https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the bug 
 gives enough additional information. My speculation (and it's really just a 
 speculation) is that it's not related to isspace per-se, but to something 
 else which gets defined/redefined/undefined by including ctype.h. I guess 
 it would be good to know if we have tests which cover the thing the comment 
 is alluding to (non-ascii in Premain-Class).
 Thanks for pointing this out. I looked at it again and the issue is that 
 isspace is a macro and depends on the locale. By not including ctype.h then 
 it means we get linked to the libc function instead. One approach is to 
 include ctype.h and then #undef isspace, another is to define function 
 prototype ourselves. I think the latter is a little bit better because it 
 would avoid accidental usage of other local sensitive char classifiers. 
 Attached is the patch that I propose. I have deliberate moved to to after 
 other includes so we get a chance to #undef in the event that it gets 
 included by something else.
 
 On tests then PremainClassTest.java is good enough to find this on Solaris.
 
 -Alan
 
 
 diff --git a/src/share/instrument/JarFacade.c 
 b/src/share/instrument/JarFacade.c
 --- a/src/share/instrument/JarFacade.c
 +++ b/src/share/instrument/JarFacade.c
 @@ -23,17 +23,20 @@
   * questions.
   */
 
 -#ifdef _WIN32
 -/*
 - * Win* needs this include. However, Linux and Solaris do not.
 - * Having this include on Solaris SPARC breaks having non US-ASCII
 - * characters in the value of the Premain-Class attribute.
 - */
 -#include ctype.h
 -#endif /* _WIN32 */
  #include string.h
  #include stdlib.h
 -#include ctype.h
 +
 +/**
 + * ctype.h is required on Windows. For other platforms we use a function
 + * prototype to ensure that we use the libc isspace function rather than
 + * the isspace macro (due to isspace being locale sensitive)
 + */
 +#ifdef _WIN32
 +  #include ctype.h
 +#else
 +  #undef isspace
 +  extern int isspace(int c);
 +#endif /* _WIN32 */
 
  #include jni.h
  #include manifest_info.h
 

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

Alan,

I made the change to JarFacade.c myself last week, only to then see the comment 
a few lines above where you added the new include. It seems to indicate that 
including ctype.h on Solaris/SPARC is a bad idea. I have no idea if the comment 
is still relevant, but that may be worth understanding first.

Cheers,
Mikael

 On Feb 13, 2014, at 5:18, Alan Bateman alan.bate...@oracle.com wrote:
 
 
 The number of native code warnings in the build is annoying so this is 
 another drive-by fix that eliminates a few of them in the serviceability and 
 security areas. The webrev with the changes is here:
 
 http://cr.openjdk.java.net/~alanb/8034856+8034857/webrev/
 
 In the pkcs11 code the issue is the function prototypes for the throwXXX 
 functions aren't included. This is fixed by including pkcs11wrapper.h but 
 that exposes another issue with the header file includes that needed to be 
 fixed.
 
 In JarFacade the issue is that it uses isspace but doesn't include the ctype.h
 
 For LinuxOperatingSystem.c then there are 12 warnings related to fscanf 
 usages where the format specifier is %lld and the code wants to read into a 
 uint64_t. I've changed the format specifier to%SCNd64 so that it matches 
 uint64_t and should be okay on both 32 and 64-bit.
 
 Thanks,
 Alan.

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-13 10:23, Alan Bateman wrote:

On 13/02/2014 17:56, Mikael Vidstedt wrote:

Alan,

I made the change to JarFacade.c myself last week, only to then see 
the comment a few lines above where you added the new include. It 
seems to indicate that including ctype.h on Solaris/SPARC is a bad 
idea. I have no idea if the comment is still relevant, but that may 
be worth understanding first.



Do you have cycles to look into it? As the code is using isspace 
already then it's not clear (unless there are different versions). 
Before pushing the changes then I ran the tests on all platforms 
(including Solaris) and the j.l.i tests include a number of tests 
exercise these manifest attributes with a non-US characters.


The change in question appears to come from 
https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the 
bug gives enough additional information. My speculation (and it's really 
just a speculation) is that it's not related to isspace per-se, but to 
something else which gets defined/redefined/undefined by including 
ctype.h. I guess it would be good to know if we have tests which cover 
the thing the comment is alluding to (non-ascii in Premain-Class).


As an aside, the native code warnings coming from the jdk repository 
are really annoying so this is the reason for the drive-by fixes when 
I get a few minutes. I think others are doing the same.


Absolutely support this work! As a matter of fact I have a couple of 
change in a sandbox I should send out for review.


Cheers,
Mikael

Re: code review (round 0) request for VS2010 IDE fix (8016601)

[Mikael Vidstedt]

Dan,

I have not reviewed the actual changes, but FWIW I have verified that 
applying the patch does solve the linker error you mention. Thanks a lot 
for fixing!


Cheers,
Mikael

On 2013-08-02 15:36, Daniel D. Daugherty wrote:

Greetings,

I have have a proposed fix for the following bug:

8016601 Unable to build hsx24 on Windows using project creator and
Visual Studio
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8016601
https://jbs.oracle.com/bugs/browse/JDK-8016601

Here are the HSX-25 webrev URLs:

OpenJDK: http://cr.openjdk.java.net/~dcubed/8016601-webrev/0-hsx25/
Internal: http://javaweb.us.oracle.com/~ddaugher/8016601-webrev/0-hsx25/

Testing:
- JPRT windows_i586 and windows_x64 build and test
- local windows_i586 cmd line builds for:
  {OpenJDK, Oracle} x {Client, Server} VM x {product, debug, fastdebug}
- local windows_i586 VS2010 builds for
  {OpenJDK, Oracle} x {Client, Server, Tiered} VM x {product, debug, 
fastdebug}

- local windows_x64 VS2010 builds for
  {OpenJDK, Oracle} x {Client, Server, Tiered} VM x {product, debug, 
fastdebug}

  Thanks to Ron for doing the windows_x64 testing

Gory details are below. As always, comments, questions and
suggestions are welome.

Dan


Gory Details:

Build fixes:
- VS2010 IDE builds are working again; fixes this failure mode:

1D:\hotspot_src\hsx\24\hotspot_14_jun\build\vs-i486\compiler2\debug\objectCountEventSender.obj 
: warning LNK4042: object specified more than once; extras ignored
1D:\hotspot_src\hsx\24\hotspot_14_jun\build\vs-i486\compiler2\debug\errorReporter.obj 
: warning LNK4042: object specified more than once; extras ignored
1 Creating library 
D:\hotspot_src\hsx\24\hotspot_14_jun\build\vs-i486\compiler2\debug\jvm.lib 
and object 
D:\hotspot_src\hsx\24\hotspot_14_jun\build\vs-i486\compiler2\debug\jvm.exp
1jfrRequestables.obj : error LNK2019: unresolved external symbol 
public: static void __cdecl 
ObjectCountEventSender::disable_requestable_event(void) 
(?disable_requestable_event@ObjectCountEventSender@@SAXXZ) referenced 
in function public: virtual void __thiscall 
VM_GC_SendObjectCountEvent::doit(void) 
(?doit@VM_GC_SendObjectCountEvent@@UAEXXZ)
1jfrRequestables.obj : error LNK2019: unresolved external symbol 
public: static void __cdecl 
ObjectCountEventSender::enable_requestable_event(void) 
(?enable_requestable_event@ObjectCountEventSender@@SAXXZ) referenced 
in function public: virtual void __thiscall 
VM_GC_SendObjectCountEvent::doit(void) 
(?doit@VM_GC_SendObjectCountEvent@@UAEXXZ)
1D:\hotspot_src\hsx\24\hotspot_14_jun\build\vs-i486\compiler2\debug\jvm.dll 
: fatal error LNK1120: 2 unresolved externals


- The ProjectCreator tool is modified to support two new options:
  '-relativeAltSrcInclude' and '-altRelativeInclude'. Here's an
  example use of the new options:

  -relativeAltSrcInclude src\closed
  -altRelativeInclude share\vm

  which means config the project with some alternate source files in
  src\closed\share\vm that will replace the corresponding files in
  src\share\vm.

  For example, src\closed\share\vm\utilities\errorReporter.cpp replaces
  src\share\vm\utilities\errorReporter.cpp. In the VS2010 IDE, you'll
  still be able to see src\share\vm\utilities\errorReporter.cpp in the
  project source browser, but the icon will indicate that the file is
  excluded from the project.

  The ProjectCreator tool's file tree walking logic is modified to keep
  track of each alternate source file that is found. If a corresponding
  regular source file is found, then the regular source file is
  excluded from the project in favor of the alternate source version.

- VS2010 cmd line build no longer issue the following linker warnings:

link.exe @C:\Users\lfoltan\AppData\Local\Temp\nm9B65.tmp
errorReporter.obj : warning LNK4042: object specified more than 
once; extras ignored
objectCountEventSender.obj : warning LNK4042: object specified 
more than once; extras ignored


Misc cleanups:

- removed more core config support from various makefiles and scripts;
  the core config is vestigal and was mostly removed years ago; the
  core config is not the same as the minimalvm config.
- removed extra references to ${ALTSRC}/share/vm/jfr objects
- added some AltSrc versus OpenJDK identification to messages where
  files are auto-generated
- added some missing copyright headers

Re: RFR(XS): 8009287 Uninitialised variable in hotspot/agent/src/os/linux/ps_core.c

[Mikael Vidstedt]

Looks good!

Cheers,
Mikael

On 2013-03-04 23:24, Staffan Larsen wrote:

A very small fix for a warning.

webrev: http://cr.openjdk.java.net/~sla/8009287/webrev.00/

Thanks,
/Staffan

Re: RFR (XS): 8007901 SA: Don't read flag values as constants

[Mikael Vidstedt]

Nasty, so normally the code picks up the value from the VMIntConstant 
vmstructs array which is the hardcoded default value, but with this fix 
it's picked up from the command line options database which reflects the 
actual current value. That makes sense and looks good.


Cheers,
Mikael

On 2013-02-09 11:06, Staffan Larsen wrote:

Please review this small patch to avoid reading flag values in SA as constants. 
Reading them as constants  means SA will only see the default value for these 
flags. Instead the infrastructure in SA to read command line flags should be 
used. In addition the value if EnableInvokeDynamic is never used, so I removed 
that from SA.

webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/
bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible)

Thanks,
/Staffan

Re: RR(S): 8007536 Incorrect copyright header in JDP files

[Mikael Vidstedt]

Looks good.

Cheers,
Mikael

On 2013-02-06 06:05, Dmitry Samersoff wrote:

Hi Everyone,

Please, review this, copyright-only fix

http://cr.openjdk.java.net/~dsamersoff/8007536.JDP-COPYRIGHT/webrev.01/jdk/

-Dmitry

Re: RFR (XS): 8005592: ClassLoaderDataGraph::_unloading incorrectly defined as nonstatic in vmStructs

[Mikael Vidstedt]

Thanks Karen, I have verified that the SA does not look for the 
_unloading field. I will remove it!


Cheers,
Mikael

On 2013-01-11 05:11, Karen Kinnear wrote:

Mikael,

I like the assertion that you added.
I believe that this field isn't used by SA - so if you could double-check that 
in SA - if that is the case, please
just remove it from vmStructs before checking in.

thanks,
Karen

On Dec 28, 2012, at 5:24 PM, Mikael Vidstedt wrote:


Please review the following change.

Background:

The _unloading field is a static field in ClassLoaderDataGraph (in 
classLoaderData.hpp) and should therefore be defined using static_field, as 
opposed to nonstatic_field, in vmStructs.

Apart from changing from nonstatic_field to static_field I also added an assert 
in the CHECK_NONSTATIC_VM_STRUCT_ENTRY macro to make sure any field offsets are 
within the bounds of the corresponding structs. The assert triggers for 
_unloading before the change to static_field.

The change passes JPRT.

http://cr.openjdk.java.net/~mikael/8005592/webrev.00/

Thanks,
Mikael

Re: RFR (S): 8004747: Remove last_entry from VM_STRUCT macros

[Mikael Vidstedt]

David,

I did two things (both on linux/x86_64 only):

1. I compared the pre-processor output manually. Unfortunately there are 
line numbers sprinkled around in the pre-processed file which makes 
comparing the output a bit more complex than it should be. Modulo the 
line numbers and some white space changes the outputs matche.
2. I wrote a small test which mimics what the SA does - it looks up the 
four exported structures dynamically and reads the data. Apart from the 
address field in the VMStructEntry entries the outputs almost [1] 
match. The address field is expected to vary since it reflects the 
actual placement in memory of various static variables.


Are there other experiments you can think of?

Cheers,
Mikael

[1] Using this tool I actually found one bug in the table - one entry 
(ClassLoaderDataGraph::_unloading) is declared as nonstatic when it is 
indeed static. I'll file a separate bug for that.


On 12/11/2012 8:16 PM, David Holmes wrote:

Hi Mikael,

This looks okay to me - I like the simplification. However as I don't 
perform mental macro substitution particularly well (okay, not at all 
;-)) I was wondering if there was a simple validation test where we 
could compare the pre-processed output before and after?


Thanks,
David

On 11/12/2012 7:01 AM, Mikael Vidstedt wrote:


Please review the following change:

Webrev: http://cr.openjdk.java.net/~mikael/8004747/webrev.00/

Background:

This patch does 3 things (depending on how you count it):

1. Move the call to generating the last entry for the VM structures
database

The vmStructs functionality is a macro based way of creating a database
of information about VM internal structures, types and constants.

The database for structures is defined in runtime/vmStructs.cpp in an
array called localHotSpotVMStructs. The content of the array is
generated with calls to a few different macros (VM_STRUCTS,
VM_STRUCTS_PARALLELGC, VM_STRUCTS_CMS, VM_STRUCTS_G1, VM_STRUCTS_CPU and
VM_STRUCTS_OS_CPU respectively). Common for all these macros is the fact
that the last argument passed in is another macro
(GENERATE_VM_STRUCT_LAST_ENTRY) which is a generator for the end
marker/last entry in the database; a special entry which can be easily
located by an external consumer of the information.

Even though the end marker generator macro
(GENERATE_VM_STRUCT_LAST_ENTRY) is passed to all the VM_STRUCT* macros,
the actual end marker must be generated once and only once. The way this
is currently handled is that only the last VM_STRUCT macro, which
happens to be VM_STRUCTS_OS_CPU, actually makes use of its last argument
and adds it to the end of the array.

Not only is this fairly complex to understand, it's also more fragile
than it needs to be.

This change removes the last argument for all the VM_STRUCT macros and
instead explicitly inserts the end marker at the end of the
localHotSpotVMStructs array.

The same VM_STRUCTS macros are being used in VMStructs::init to do type
checking. Instead of passing the end marker generator into the macros
the last parameter is instead CHECK_SENTINEL, which is defined to expand
to nothing, which means it can be safely removed.

2. Move the end marker generating for the VM types, VM int constants and
VM long constants databases

Repeat the exact above story, but replace anything called Structs/STRUCT
with Types/TYPES, IntConstants/INT_CONSTANTS and
LongConstants/LONG_CONSTANTS respectively.

3. Minor prettification

In addition to the above the change also removes some superfluous
backslashes from a few of the VM_STRUCT* macro calls.

Thanks,
Mikael

RFR (XS): 8005592: ClassLoaderDataGraph::_unloading incorrectly defined as nonstatic in vmStructs

[Mikael Vidstedt]

Please review the following change.

Background:

The _unloading field is a static field in ClassLoaderDataGraph (in 
classLoaderData.hpp) and should therefore be defined using static_field, 
as opposed to nonstatic_field, in vmStructs.


Apart from changing from nonstatic_field to static_field I also added an 
assert in the CHECK_NONSTATIC_VM_STRUCT_ENTRY macro to make sure any 
field offsets are within the bounds of the corresponding structs. The 
assert triggers for _unloading before the change to static_field.


The change passes JPRT.

http://cr.openjdk.java.net/~mikael/8005592/webrev.00/

Thanks,
Mikael

Re: RFR (S): 8004747: Remove last_entry from VM_STRUCT macros

[Mikael Vidstedt]

Thanks David!

Still need a 2nd reviewer!

Cheers,
Mikael

On 12/28/2012 3:34 PM, David Holmes wrote:

Mikael,

That all sounds fine to me.

Thanks,
David

On 29/12/2012 3:48 AM, Mikael Vidstedt wrote:


David,

I did two things (both on linux/x86_64 only):

1. I compared the pre-processor output manually. Unfortunately there are
line numbers sprinkled around in the pre-processed file which makes
comparing the output a bit more complex than it should be. Modulo the
line numbers and some white space changes the outputs matche.
2. I wrote a small test which mimics what the SA does - it looks up the
four exported structures dynamically and reads the data. Apart from the
address field in the VMStructEntry entries the outputs almost [1]
match. The address field is expected to vary since it reflects the
actual placement in memory of various static variables.

Are there other experiments you can think of?

Cheers,
Mikael

[1] Using this tool I actually found one bug in the table - one entry
(ClassLoaderDataGraph::_unloading) is declared as nonstatic when it is
indeed static. I'll file a separate bug for that.

On 12/11/2012 8:16 PM, David Holmes wrote:

Hi Mikael,

This looks okay to me - I like the simplification. However as I don't
perform mental macro substitution particularly well (okay, not at all
;-)) I was wondering if there was a simple validation test where we
could compare the pre-processed output before and after?

Thanks,
David

On 11/12/2012 7:01 AM, Mikael Vidstedt wrote:


Please review the following change:

Webrev: http://cr.openjdk.java.net/~mikael/8004747/webrev.00/

Background:

This patch does 3 things (depending on how you count it):

1. Move the call to generating the last entry for the VM structures
database

The vmStructs functionality is a macro based way of creating a 
database

of information about VM internal structures, types and constants.

The database for structures is defined in runtime/vmStructs.cpp in an
array called localHotSpotVMStructs. The content of the array is
generated with calls to a few different macros (VM_STRUCTS,
VM_STRUCTS_PARALLELGC, VM_STRUCTS_CMS, VM_STRUCTS_G1, 
VM_STRUCTS_CPU and
VM_STRUCTS_OS_CPU respectively). Common for all these macros is the 
fact

that the last argument passed in is another macro
(GENERATE_VM_STRUCT_LAST_ENTRY) which is a generator for the end
marker/last entry in the database; a special entry which can be easily
located by an external consumer of the information.

Even though the end marker generator macro
(GENERATE_VM_STRUCT_LAST_ENTRY) is passed to all the VM_STRUCT* 
macros,
the actual end marker must be generated once and only once. The way 
this

is currently handled is that only the last VM_STRUCT macro, which
happens to be VM_STRUCTS_OS_CPU, actually makes use of its last 
argument

and adds it to the end of the array.

Not only is this fairly complex to understand, it's also more fragile
than it needs to be.

This change removes the last argument for all the VM_STRUCT macros and
instead explicitly inserts the end marker at the end of the
localHotSpotVMStructs array.

The same VM_STRUCTS macros are being used in VMStructs::init to do 
type

checking. Instead of passing the end marker generator into the macros
the last parameter is instead CHECK_SENTINEL, which is defined to 
expand

to nothing, which means it can be safely removed.

2. Move the end marker generating for the VM types, VM int 
constants and

VM long constants databases

Repeat the exact above story, but replace anything called 
Structs/STRUCT

with Types/TYPES, IntConstants/INT_CONSTANTS and
LongConstants/LONG_CONSTANTS respectively.

3. Minor prettification

In addition to the above the change also removes some superfluous
backslashes from a few of the VM_STRUCT* macro calls.

Thanks,
Mikael

RFR (S): 8004747: Remove last_entry from VM_STRUCT macros

[Mikael Vidstedt]

Please review the following change:

Webrev: http://cr.openjdk.java.net/~mikael/8004747/webrev.00/

Background:

This patch does 3 things (depending on how you count it):

1. Move the call to generating the last entry for the VM structures 
database


The vmStructs functionality is a macro based way of creating a database 
of information about VM internal structures, types and constants.


The database for structures is defined in runtime/vmStructs.cpp in an 
array called localHotSpotVMStructs. The content of the array is 
generated with calls to a few different macros (VM_STRUCTS, 
VM_STRUCTS_PARALLELGC, VM_STRUCTS_CMS, VM_STRUCTS_G1, VM_STRUCTS_CPU and 
VM_STRUCTS_OS_CPU respectively). Common for all these macros is the fact 
that the last argument passed in is another macro 
(GENERATE_VM_STRUCT_LAST_ENTRY) which is a generator for the end 
marker/last entry in the database; a special entry which can be easily 
located by an external consumer of the information.


Even though the end marker generator macro 
(GENERATE_VM_STRUCT_LAST_ENTRY) is passed to all the VM_STRUCT* macros, 
the actual end marker must be generated once and only once. The way this 
is currently handled is that only the last VM_STRUCT macro, which 
happens to be VM_STRUCTS_OS_CPU, actually makes use of its last argument 
and adds it to the end of the array.


Not only is this fairly complex to understand, it's also more fragile 
than it needs to be.


This change removes the last argument for all the VM_STRUCT macros and 
instead explicitly inserts the end marker at the end of the 
localHotSpotVMStructs array.


The same VM_STRUCTS macros are being used in VMStructs::init to do type 
checking. Instead of passing the end marker generator into the macros 
the last parameter is instead CHECK_SENTINEL, which is defined to expand 
to nothing, which means it can be safely removed.


2. Move the end marker generating for the VM types, VM int constants and 
VM long constants databases


Repeat the exact above story, but replace anything called Structs/STRUCT 
with Types/TYPES, IntConstants/INT_CONSTANTS and 
LongConstants/LONG_CONSTANTS respectively.


3. Minor prettification

In addition to the above the change also removes some superfluous 
backslashes from a few of the VM_STRUCT* macro calls.


Thanks,
Mikael

Re: RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

Serguei,

I'm confused. When I look at the latest webrev 
(http://cr.openjdk.java.net/~mikael/8003879/webrev.02/) I believe the 
indentation is correct - correct being defined as aligned on the first 
letter of the type name. Please clarify what you expect if you do not agree.


Thanks,
Mikael

On 11/26/2012 12:21 PM, serguei.spit...@oracle.com wrote:


Looks good.
However, it'd be nice to fix the indentation commented by David (use 
frames to notice it):


*vmStructs_cms.hpp*:
68declare_type(AFLBinaryTreeDictionary, 
FreeBlockDictionaryFreeChunk)

*jni.cpp:*
2110declare_type(MetablockTreeDictionary, 
FreeBlockDictionaryMetablock)

Thanks,
Serguei


On 11/22/12 5:04 PM, Mikael Vidstedt wrote:


On 2012-11-22 16:35, David Holmes wrote:

Hi Mikael,

I prefer this as an internal test too. Though I don't understand why 
execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - this 
is an enhancement to the VM interface not the JNI interface isn't 
it? (Separate issue of course).


I think we'll find that when we get a few more tests in there it's 
time to look at breaking it out into a separate file completely or do 
it in a completely different way, but that's for another day. I do 
hope it's soon though.


BTW the weird indentation makes more sense viewing frames rather 
than diffs :)


Right :)



Looks good to go.


Thanks! Eagerly awaiting a second review!

/Mikael



David

On 23/11/2012 10:16 AM, Mikael Vidstedt wrote:


I gave it some more thought and decided to try what it would look like
having the test be part of the ExecuteInternalVMTests family 
instead. I

like this one better personally, but comments are appreciated!

http://cr.openjdk.java.net/~mikael/8003879/webrev.02

Cheers,
Mikael

On 2012-11-22 14:25, Mikael Vidstedt wrote:


New webrev available here:

http://cr.openjdk.java.net/~mikael/8003879/webrev.01

Cheers,
Mikael

On 2012-11-22 14:19, Mikael Vidstedt wrote:

On 2012-11-21 20:17, David Holmes wrote:

Hi Mikael,

On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:


Please review the below change.

The change for 7045397 introduced a couple of duplicate entries 
in the

vmStructs::localHotSpotVMTypes array. This shows up when using the
jmap
tool in a rather ugly way:

snip

Other than an indentation problem (which I don't think you
introduced) this fixup seems fine.


For some reason that's the standard for declaring types in that long
list - aligning on the first letter of the type name. I followed 
suit.





In addition to removing the two duplicated entries I also added a
simple, naive runtime test to walk through and make sure no 
type is
repeated. The VMStructs::init only called in debug_only so 
there's no

startup overhead in product, but it may be better to turn the test
into
a unit test and only running it as part of ExecuteInternalVMTests.
Feedback appreciated!


I have a few comments on this part:

- Array indices should be int's not size_t (ie signed not unsigned)


Will fix.


- I can't clearly see how the localHotSpotVMTypes array is declared
or filled but I assume there is guaranteed to be a sentinel 
entry at

the end so that we don't index past the end?


Yeah. I'm not sure you want to know. It took me a few minutes to
figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY
macro which generates the end marker is passed to VM_TYPES,
VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is 
allowed to

actually allowed to call the macro and actually generate the marker,
meaning only the implementation of VM_TYPES_OS_CPU actually has 
the

call to last_entry(). Not sure why the end marker isn't just
explicitly added at the end instead, but I think I'll queue that up
for later.

All in all, as far as I can tell there is one and only one end 
marker

and it is generated as part of the expansion of VM_TYPES_OS_CPU.


- assert(0, ...) should be assert(false, ...) (as per style guide
[1] ;-) )


Will fix.

That all said I'm not sure this test belongs there, but I don't 
feel

strongly either way.


Me neither :)

Cheers,
Mikael



Cheers,
David

[1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide



http://cr.openjdk.java.net/~mikael/8003879/webrev.00/

Cheers,
Mikael

Re: RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

No worries, thanks for clarifying and for the review(s)!

Cheers,
Mikael

On 11/26/2012 3:00 PM, serguei.spit...@oracle.com wrote:

Mikael,

Sorry for the confusion. I see what you mean.
I missed that you made it consistent with the current style in these 
files.

Agree, that is the best you can do.

Thanks,
Serguei

On 11/26/12 2:37 PM, Mikael Vidstedt wrote:


Serguei,

I'm confused. When I look at the latest webrev 
(http://cr.openjdk.java.net/~mikael/8003879/webrev.02/) I believe the 
indentation is correct - correct being defined as aligned on the 
first letter of the type name. Please clarify what you expect if you 
do not agree.


Thanks,
Mikael

On 11/26/2012 12:21 PM, serguei.spit...@oracle.com wrote:


Looks good.
However, it'd be nice to fix the indentation commented by David (use 
frames to notice it):


*vmStructs_cms.hpp*:
68declare_type(AFLBinaryTreeDictionary, 
FreeBlockDictionaryFreeChunk)

*jni.cpp:*
2110declare_type(MetablockTreeDictionary, 
FreeBlockDictionaryMetablock)

Thanks,
Serguei


On 11/22/12 5:04 PM, Mikael Vidstedt wrote:


On 2012-11-22 16:35, David Holmes wrote:

Hi Mikael,

I prefer this as an internal test too. Though I don't understand 
why execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - 
this is an enhancement to the VM interface not the JNI interface 
isn't it? (Separate issue of course).


I think we'll find that when we get a few more tests in there it's 
time to look at breaking it out into a separate file completely or 
do it in a completely different way, but that's for another day. I 
do hope it's soon though.


BTW the weird indentation makes more sense viewing frames rather 
than diffs :)


Right :)



Looks good to go.


Thanks! Eagerly awaiting a second review!

/Mikael



David

On 23/11/2012 10:16 AM, Mikael Vidstedt wrote:


I gave it some more thought and decided to try what it would look 
like
having the test be part of the ExecuteInternalVMTests family 
instead. I

like this one better personally, but comments are appreciated!

http://cr.openjdk.java.net/~mikael/8003879/webrev.02

Cheers,
Mikael

On 2012-11-22 14:25, Mikael Vidstedt wrote:


New webrev available here:

http://cr.openjdk.java.net/~mikael/8003879/webrev.01

Cheers,
Mikael

On 2012-11-22 14:19, Mikael Vidstedt wrote:

On 2012-11-21 20:17, David Holmes wrote:

Hi Mikael,

On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:


Please review the below change.

The change for 7045397 introduced a couple of duplicate 
entries in the
vmStructs::localHotSpotVMTypes array. This shows up when 
using the

jmap
tool in a rather ugly way:

snip

Other than an indentation problem (which I don't think you
introduced) this fixup seems fine.


For some reason that's the standard for declaring types in that 
long
list - aligning on the first letter of the type name. I 
followed suit.




In addition to removing the two duplicated entries I also 
added a
simple, naive runtime test to walk through and make sure no 
type is
repeated. The VMStructs::init only called in debug_only so 
there's no
startup overhead in product, but it may be better to turn the 
test

into
a unit test and only running it as part of 
ExecuteInternalVMTests.

Feedback appreciated!


I have a few comments on this part:

- Array indices should be int's not size_t (ie signed not 
unsigned)


Will fix.

- I can't clearly see how the localHotSpotVMTypes array is 
declared
or filled but I assume there is guaranteed to be a sentinel 
entry at

the end so that we don't index past the end?


Yeah. I'm not sure you want to know. It took me a few minutes to
figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY
macro which generates the end marker is passed to VM_TYPES,
VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is 
allowed to
actually allowed to call the macro and actually generate the 
marker,
meaning only the implementation of VM_TYPES_OS_CPU actually 
has the

call to last_entry(). Not sure why the end marker isn't just
explicitly added at the end instead, but I think I'll queue 
that up

for later.

All in all, as far as I can tell there is one and only one end 
marker

and it is generated as part of the expansion of VM_TYPES_OS_CPU.


- assert(0, ...) should be assert(false, ...) (as per style guide
[1] ;-) )


Will fix.

That all said I'm not sure this test belongs there, but I 
don't feel

strongly either way.


Me neither :)

Cheers,
Mikael



Cheers,
David

[1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide



http://cr.openjdk.java.net/~mikael/8003879/webrev.00/

Cheers,
Mikael

Re: RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

New webrev available here:

http://cr.openjdk.java.net/~mikael/8003879/webrev.01

Cheers,
Mikael

On 2012-11-22 14:19, Mikael Vidstedt wrote:

On 2012-11-21 20:17, David Holmes wrote:

Hi Mikael,

On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:


Please review the below change.

The change for 7045397 introduced a couple of duplicate entries in the
vmStructs::localHotSpotVMTypes array. This shows up when using the jmap
tool in a rather ugly way:

snip

Other than an indentation problem (which I don't think you 
introduced) this fixup seems fine.


For some reason that's the standard for declaring types in that long 
list - aligning on the first letter of the type name. I followed suit.





In addition to removing the two duplicated entries I also added a
simple, naive runtime test to walk through and make sure no type is
repeated. The VMStructs::init only called in debug_only so there's no
startup overhead in product, but it may be better to turn the test into
a unit test and only running it as part of ExecuteInternalVMTests.
Feedback appreciated!


I have a few comments on this part:

- Array indices should be int's not size_t (ie signed not unsigned)


Will fix.

- I can't clearly see how the localHotSpotVMTypes array is declared 
or filled but I assume there is guaranteed to be a sentinel entry at 
the end so that we don't index past the end?


Yeah. I'm not sure you want to know. It took me a few minutes to 
figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro 
which generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and 
VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed 
to call the macro and actually generate the marker, meaning only the 
implementation of VM_TYPES_OS_CPU actually has the call to 
last_entry(). Not sure why the end marker isn't just explicitly added 
at the end instead, but I think I'll queue that up for later.


All in all, as far as I can tell there is one and only one end marker 
and it is generated as part of the expansion of VM_TYPES_OS_CPU.


- assert(0, ...) should be assert(false, ...)  (as per style guide 
[1] ;-) )


Will fix.

That all said I'm not sure this test belongs there, but I don't feel 
strongly either way.


Me neither :)

Cheers,
Mikael



Cheers,
David

[1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide



http://cr.openjdk.java.net/~mikael/8003879/webrev.00/

Cheers,
Mikael

Re: RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

I gave it some more thought and decided to try what it would look like 
having the test be part of the ExecuteInternalVMTests family instead. I 
like this one better personally, but comments are appreciated!


http://cr.openjdk.java.net/~mikael/8003879/webrev.02

Cheers,
Mikael

On 2012-11-22 14:25, Mikael Vidstedt wrote:


New webrev available here:

http://cr.openjdk.java.net/~mikael/8003879/webrev.01

Cheers,
Mikael

On 2012-11-22 14:19, Mikael Vidstedt wrote:

On 2012-11-21 20:17, David Holmes wrote:

Hi Mikael,

On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:


Please review the below change.

The change for 7045397 introduced a couple of duplicate entries in the
vmStructs::localHotSpotVMTypes array. This shows up when using the 
jmap

tool in a rather ugly way:

snip

Other than an indentation problem (which I don't think you 
introduced) this fixup seems fine.


For some reason that's the standard for declaring types in that long 
list - aligning on the first letter of the type name. I followed suit.





In addition to removing the two duplicated entries I also added a
simple, naive runtime test to walk through and make sure no type is
repeated. The VMStructs::init only called in debug_only so there's no
startup overhead in product, but it may be better to turn the test 
into

a unit test and only running it as part of ExecuteInternalVMTests.
Feedback appreciated!


I have a few comments on this part:

- Array indices should be int's not size_t (ie signed not unsigned)


Will fix.

- I can't clearly see how the localHotSpotVMTypes array is declared 
or filled but I assume there is guaranteed to be a sentinel entry at 
the end so that we don't index past the end?


Yeah. I'm not sure you want to know. It took me a few minutes to 
figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY 
macro which generates the end marker is passed to VM_TYPES, 
VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to 
actually allowed to call the macro and actually generate the marker, 
meaning only the implementation of VM_TYPES_OS_CPU actually has the 
call to last_entry(). Not sure why the end marker isn't just 
explicitly added at the end instead, but I think I'll queue that up 
for later.


All in all, as far as I can tell there is one and only one end marker 
and it is generated as part of the expansion of VM_TYPES_OS_CPU.


- assert(0, ...) should be assert(false, ...)  (as per style guide 
[1] ;-) )


Will fix.

That all said I'm not sure this test belongs there, but I don't feel 
strongly either way.


Me neither :)

Cheers,
Mikael



Cheers,
David

[1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide



http://cr.openjdk.java.net/~mikael/8003879/webrev.00/

Cheers,
Mikael

Re: RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

On 2012-11-22 16:35, David Holmes wrote:

Hi Mikael,

I prefer this as an internal test too. Though I don't understand why 
execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - this is 
an enhancement to the VM interface not the JNI interface isn't it? 
(Separate issue of course).


I think we'll find that when we get a few more tests in there it's time 
to look at breaking it out into a separate file completely or do it in a 
completely different way, but that's for another day. I do hope it's 
soon though.


BTW the weird indentation makes more sense viewing frames rather than 
diffs :)


Right :)



Looks good to go.


Thanks! Eagerly awaiting a second review!

/Mikael



David

On 23/11/2012 10:16 AM, Mikael Vidstedt wrote:


I gave it some more thought and decided to try what it would look like
having the test be part of the ExecuteInternalVMTests family instead. I
like this one better personally, but comments are appreciated!

http://cr.openjdk.java.net/~mikael/8003879/webrev.02

Cheers,
Mikael

On 2012-11-22 14:25, Mikael Vidstedt wrote:


New webrev available here:

http://cr.openjdk.java.net/~mikael/8003879/webrev.01

Cheers,
Mikael

On 2012-11-22 14:19, Mikael Vidstedt wrote:

On 2012-11-21 20:17, David Holmes wrote:

Hi Mikael,

On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:


Please review the below change.

The change for 7045397 introduced a couple of duplicate entries 
in the

vmStructs::localHotSpotVMTypes array. This shows up when using the
jmap
tool in a rather ugly way:

snip

Other than an indentation problem (which I don't think you
introduced) this fixup seems fine.


For some reason that's the standard for declaring types in that long
list - aligning on the first letter of the type name. I followed suit.




In addition to removing the two duplicated entries I also added a
simple, naive runtime test to walk through and make sure no type is
repeated. The VMStructs::init only called in debug_only so 
there's no

startup overhead in product, but it may be better to turn the test
into
a unit test and only running it as part of ExecuteInternalVMTests.
Feedback appreciated!


I have a few comments on this part:

- Array indices should be int's not size_t (ie signed not unsigned)


Will fix.


- I can't clearly see how the localHotSpotVMTypes array is declared
or filled but I assume there is guaranteed to be a sentinel entry at
the end so that we don't index past the end?


Yeah. I'm not sure you want to know. It took me a few minutes to
figure it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY
macro which generates the end marker is passed to VM_TYPES,
VM_TYPES_CPU and VM_TYPES_OS_CPU. But only the last one is allowed to
actually allowed to call the macro and actually generate the marker,
meaning only the implementation of VM_TYPES_OS_CPU actually has the
call to last_entry(). Not sure why the end marker isn't just
explicitly added at the end instead, but I think I'll queue that up
for later.

All in all, as far as I can tell there is one and only one end marker
and it is generated as part of the expansion of VM_TYPES_OS_CPU.


- assert(0, ...) should be assert(false, ...) (as per style guide
[1] ;-) )


Will fix.


That all said I'm not sure this test belongs there, but I don't feel
strongly either way.


Me neither :)

Cheers,
Mikael



Cheers,
David

[1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide



http://cr.openjdk.java.net/~mikael/8003879/webrev.00/

Cheers,
Mikael

Re: RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

On 2012-11-22 19:04, David Holmes wrote:

On 22/11/2012 2:17 PM, David Holmes wrote:

I have a few comments on this part:

- Array indices should be int's not size_t (ie signed not unsigned)


I humbly withdraw that comment. It is required to be an integer type 
which includes both signed and unsigned and various sizes.


I certainly prefer to use only int values. YMMV.


I'm happy to keep it as an 'int'. Thanks for verifying though! :)

/Mikael



David
-

RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

Please review the below change.

The change for 7045397 introduced a couple of duplicate entries in the 
vmStructs::localHotSpotVMTypes array. This shows up when using the jmap 
tool in a rather ugly way:


Attaching to process ID 25561, please wait...
Warning: the type MetablockTreeDictionary (declared in the remote VM 
in VMStructs::localHotSpotVMTypes) had its size declared as 32 twice. 
Continuing.
Warning: the type AFLBinaryTreeDictionary (declared in the remote VM 
in VMStructs::localHotSpotVMTypes) had its size declared as 32 twice. 
Continuing.

Debugger attached successfully.

It does appear like jmap works despite this, but clearly it's annoying 
at a very least to get the warnings.


In addition to removing the two duplicated entries I also added a 
simple, naive runtime test to walk through and make sure no type is 
repeated. The VMStructs::init only called in debug_only so there's no 
startup overhead in product, but it may be better to turn the test into 
a unit test and only running it as part of ExecuteInternalVMTests. 
Feedback appreciated!


http://cr.openjdk.java.net/~mikael/8003879/webrev.00/

Cheers,
Mikael

RFR (XS): 8003690: Example code in JVMTI GetStackTrace documentation is broken

[Mikael Vidstedt]

In the JVMTI documentation for GetStackTrace there is a code snippet 
outlining how to use the functionality:


jvmtiFrameInfo frames[5];
jint count;
jvmtiError err;

err = (*jvmti)-GetStackTrace(jvmti, aThread, 0, 5,
   frames, count);
if (err == JVMTI_ERROR_NONE  count = 1) {
   char *methodName;
   err = (*jvmti)-GetMethodName(jvmti, frames[0].method,
   methodName, NULL);
   if (err == JVMTI_ERROR_NONE) {
  printf(Executing method: %s, methodName);
   }
}

There are two errors in the code:

1. The 5th argument to GetStackTrace (frames) should not have an ampersand
2. GetMethodName takes 5 parameters, but the example only passes four 
parameters to it


Please review the following change:

http://cr.openjdk.java.net/~mikael/8003690/webrev.00/

Thanks,
Mikael

Re: RFR: 7165755 OS Information much longer on linux than other platforms

[Mikael Vidstedt]

Does anybody know if there are any tests that verify the actual contents 
of the dump?


/Mikael

On 2012-05-02 14:08, Nils Loodin wrote:

On May 2, 2012, at 14:00 , David Holmes wrote:


Hi Nils,

On 2/05/2012 9:56 PM, Nils Loodin wrote:

Ignoring Windows (with prints very little) I'd say it is the printing of 
/proc/meminfo that is the main difference. Not sure why printing that was 
necessary ... but if we are going to remove it I think we need to know why it 
was added.

Yes, that's the reason.
Note that nothing is removed. The method still prints exactly the same info, 
but I introduced another method to print briefer info, to be kinder to tool 
developers.

The current one prints /proc/meminfo. You turned that code into 
print_full_memory_info but in the main routine you call print_memory_info. Was 
that a mistake?

YES! Glad you caught that :) Guess (or hope) it would have been caught in dump 
testing otherwise :)

Regards,
Nils Loodin



David


I really don't want to change the output for say, hs_err files, where I believe 
this info is used.


This can make it hard for tool writers to get a summary that look good and 
similar for multiple platforms (sizing of gui fields, having to parse info in 
the tool code etc)
Lookin at the code, it's in some serious need of refactoring. It would be nice with a 
method to get a brief os info for these kinds of tools that looks similar on 
all platforms.

This is my suggested change:
http://cr.openjdk.java.net/~nloodin/7165755/webrev.00/

Seems to me some of this could be factored into the top-level OS class if we 
shoehorn Windows into the same shape as the other OSes ;-)

This was my first attempt also, but then a lot of empty windows-methods ensued, 
which was kind of ugly.


Or at least perhaps put some of the common stuff into os_posix.cpp ?

There's a thought!
I'll investigate that route, it could get things to look nicer.



Cheers,
David

Regards,
Nils Loodin

Re: code review request for initial JDK FDS support (7071907)

[Mikael Vidstedt]

On 2012-04-10 14:53, Daniel D. Daugherty wrote:

On 4/10/12 3:47 PM, Dmitry Samersoff wrote:


1.

 239 # If Full Debug Symbols is enabled, then we want the same debug and
 240 # optimization flags as used by FASTDEBUG. We also want all the
 241 # debug info in one place (-xs).

Sorry! I'm later at the party. What is the reason of enforcing 
certain optimization level with FDS?


FDS doesn't enforce a certain optimization level. FDS takes advantage
of work that other people have done to find optimization levels that
work with fastdebug builds. This isn't so much an issue with the JDK
(that I know of), but it is an issue with HotSpot. With HotSpot,
fastdebug builds often use a lower optimization level than fully
optimized builds because the compiler can't handle it. When FDS is
enabled, we're basically doing a fastdebug build so we piggy back
off of the same settings.


Sorry for being a bit slow here, but I'm still not sure I understand the 
comment. Can you clarify how the build process works and where the 
FASTDEBUG optimization flags are used?


Thanks,
Mikael





2.
 192   ifeq ($(ENABLE_FULL_DEBUG_SYMBOLS),1)
 193 ifeq ($(ZIP_DEBUGINFO_FILES),1)
 194 (set -e ; \
 195  $(CD) $(OBJDIR) ; \
 196  $(ZIPEXE) -q $(PROGRAM).diz $(PROGRAM).map 
$(PROGRAM).pdb ; \

 197  $(RM) $(PROGRAM).map $(PROGRAM).pdb ; \
 198 )
 199 endif
 200   endif


No fallback on zip error here. No ideas what we should do if zip 
fails, so it just a FYI.


The 'set -e' on line 194 means that the build will fail. That is
intentional. If the command fails unexpectedly, then the build
should fail and hopefully with a useful error message.

Dan




-Dmitry


On 2012-04-11 01:17, Daniel D. Daugherty wrote:

Thanks Serguei!

Dan


On 4/10/12 2:51 PM, serguei.spit...@oracle.com wrote:

Dan,

It is good.

Thanks,
Serguei

On 4/9/12 1:51 PM, Daniel D. Daugherty wrote:

Greetings,

Coming soon to a JDK repo near you! Full Debug Symbols!

OK, to just a subset of libraries and programs... on Linux and
Solaris...
If you're a Windows fan, the JDK repo has had Full Debug Symbols 
support

since way back in JDK1.4.1... Now we're trying get Linux and Solaris
caught up...

Runtime Team, we don't have much in the JDK repo, but I tried to 
cover

our few libraries and programs. Let me know if I missed anything...

Serviceability Team, all of your demos, libraries and programs are
covered... for some reason, updating those seemed like reliving old
times and I didn't think you'd mind... :-)

Here is the webrev URL:

http://cr.openjdk.java.net/~dcubed/fds_revamp/7071907-webrev/0-jdk8-jdk/ 



Thanks, in advance, for any review comments.

Dan

P.S.
For those of you that are keeping track of all the FDS
changesets, not everything has hit the various master
repos yet. As a reminder, FDS has to hit the closed
install repo first. The open root and jdk repos along
with the closed deploy repo are in the second wave. And
the hotspot repo, being more Mercurial than his fellow
ghosts, will make his appearance in his own good time
(and via a different set of repos)...

Apologies to Dickens, of course... :-)

Re: RFR(XXS): 7133111: libsaproc debug print should be printed as unsigned long to fit large numbers on 64bit platform

[Mikael Vidstedt]

You're correct Kelly. In the general case one can't trust the size of 
long to be as wide as size_t. For that reason the use case for long has 
always been somewhat of a mystery to me, especially in portable code.


/Mikael

On 2012-04-05 12:46, Kelly O'Hair wrote:

I vaguely recall sizeof(long) being 4 on Windows X64.

Use of the type long always raises red flags for me, including the printf 
options related to the long types.

Just a warning  be safe out there... ;^)

-kto

On Apr 5, 2012, at 11:01 AM, Dmitry Samersoff wrote:


On 2012-04-05 20:32, Staffan Larsen wrote:

Isn't the problem with casting to (unsigned long) that this only a 32 bit type 
and size_t can be 64?

Long scales with architecture as opposite to int.

printf(%d,%d,%d\n, sizeof(size_t), sizeof(long), sizeof(int) );

mircat(FreeBSD 64bit):dms#./test
8,8,4

-Dmitry




/Staffan

On 5 apr 2012, at 16:54, Dmitry Samersoff wrote:


Staffan,

OK for me.

But I would prefer to handle it standard rather than Linux specific way
to benefit other possible ports.

i.e. %ld and (unsigned long) ph-core-map_array[j]-memsz

-Dmitry

On 2012-04-05 16:25, Staffan Larsen wrote:

Please review the following one-character fix to a printf format string. A 'z' 
is added to the printout of a size_t field.

Thanks,
/Staffan


diff --git a/agent/src/os/linux/ps_core.c b/agent/src/os/linux/ps_core.c
--- a/agent/src/os/linux/ps_core.c
+++ b/agent/src/os/linux/ps_core.c
@@ -440,7 +440,7 @@
int j = 0;
print_debug( sorted virtual address map \n);
for (j = 0; jph-core-num_maps; j++) {
-print_debug(base = 0x%lx\tsize = %d\n, ph-core-map_array[j]-vaddr,
+print_debug(base = 0x%lx\tsize = %zd\n, 
ph-core-map_array[j]-vaddr,
   ph-core-map_array[j]-memsz);
}
 }


--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...


--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...