Re: RFR: (S): JDK-8200613: SA: jstack throws UnmappedAddressException with a CDS core file

[Jini George]

Thank you very much, Coleen. I have converted the flag into a diagnostic 
flag. The revised webrev is at:


http://cr.openjdk.java.net/~jgeorge/8200613/webrev.03/

A plain diff between this patch and the earlier one is at:

http://cr.openjdk.java.net/~jgeorge/8200613/incremental_diff

I will withdraw the associated CSR.

Thanks!
Jini.

On 12/10/2018 11:55 PM, coleen.phillim...@oracle.com wrote:


I see.  In this case, the flag should be diagnostic and not require a 
CSR.  I suppose some documentation should be added so that sustaining 
will know about the option in this case (Hi Kevin!)


thanks,
Coleen


On 12/10/18 1:38 AM, Ioi Lam wrote:

Hi Coleen,

I was one of the people who suggested the DumpPrivateMappingsInCore 
flag. It's enabled by default, so by default all the contents of 
mmap'ed files with MAP_PRIVATE will be saved to the core files.


The worry is, there may be some extreme cases where the JVM has mapped 
very large files (with NIO or JNI, etc). For example, you could have a 
100GB in-memory database. For those cases, if the user is experiencing 
crashes, but they are unable to get a core dump (because it would be 
too large), they can try running with -XX:-DumpPrivateMappingsInCore.


Thanks

- Ioi


On 12/7/18 12:03 PM, coleen.phillim...@oracle.com wrote:


Hi Jini,  We were just talking about this new option.  If someone 
gets a crash, I don't think they're going to run their application 
again with -XX:-DumpPrivateMappingsInCore in the case of the core 
file being too large.  So I don't know how generally useful this 
option is. I think it would be better to not add it and set the bit 
to include the mappings unconditionally.


How much larger is this core file (we had trouble understanding from 
the bug)?  If you need the mappings to understand and use the SA 
tools on it, we want to have them.


Thanks,
Coleen


On 12/7/18 2:22 PM, Jini George wrote:

I have the revised webrev here:

http://cr.openjdk.java.net/~jgeorge/8200613/webrev.02/index.html

The extra changes here are to:

* Introduce a new option DumpPrivateMappingsInCore to control the 
dumping of the file backed private regions into the corefile.


* Close the modules file before dumping core in os::abort(). 
Currently, there is a small bug 
(https://bugs.openjdk.java.net/browse/JDK-8215026) which prevents 
the closure of the image file in unmapping the entire file.


I plan to take up the unmapping of NIO MapMode.PRIVATE files as a 
separate task (https://bugs.openjdk.java.net/browse/JDK-8215027) 
since this seems a bit involved.


Thanks a bunch,
Jini.

On 11/12/2018 10:26 AM, Jini George wrote:

Thank you very much, Chris, Kevin and Ioi for your comments!

I will send another webrev with this change enabled under an 
opt-out flag, as you suggest, and would look at unmapping the JDK 
modules file and if possible, the NIO mapped files too in the 
signal handler.


Thanks a bunch,
Jini.

On 11/9/2018 11:53 PM, Ioi Lam wrote:

Hi Jini,

Thanks for investigating the size expansion issue.

I agree that the size increase is worth it. Even when not using 
SA, if we open the core file inside GDB, we cannot read certain 
sections in the CDS archive (such as the RO section and strings 
sections). That would make debugging difficult. So I am in favor 
of this change.


For the JDK modules file, maybe we can unmap it in the signal 
handler, before going ahead with the core dump? I think it's 
hardly needed for debugging purposes. (Perhaps we can also do the 
same for the NIO mapped files?)


A opt-flag as suggested by Kevin is a good idea.

Thanks

- Ioi

On 11/9/18 3:29 AM, Kevin Walls wrote:

Hi Jini,

Looks good to me.  It might be a significant increase in size of 
_some_ core files, but so many core files we see are much larger, 
in gigabytes++ of course, so the CDS data size should not be such 
a significant increase on (I think) most files.


The flexibiity of always having the CDS data there is very 
significant.  A core file should ideally be usable, without 
additionally requiring the CDS archive from the machine. That 
additional human round-trip upload request on every transmitted 
core that needs investigating, seems like a less efficient 
route...).


Is there an opt-out?  It's conditional on UseSharedSpaces but 
could there be a flag to disable, in case we see crashes with 
gigabytes of private mappings that we really don't want to retain 
(the user would have to know to set a flag, to disable the new 
coredump filter ahead of time).


Thanks!
Kevin


On 29/10/2018 06:02, Jini George wrote:
Thank you very much, Ioi, for looking into this, and the 
clarification offline. My bad, I had missed the earlier mail 
from you. :-( My responses below.


Yes, I had tested this on MacOS. The issue does not exist on 
MacOS since the file backed private mmap()-ed regions get dumped 
into the MacOS corefiles by default.


The corefile sizes on Linux do increase due to this change. And 
the increase would also include any file mapped 

Re: RFR 8215050: [TESTBUG] serviceability/tmtools/jstack/WaitNotifyThreadTest.java fails when run with flag -Xcomp

[David Holmes]

Sorry please ignore - "off by one error" checking the CI results. This 
bug was fixed in 1097 and I was looking at 1096.


David

On 11/12/2018 2:50 pm, David Holmes wrote:

 This is still failing in tier4:

Error: incorrect monitor info: locked, a java.lang.Object, 
0x00078653eeb0

Expected: locked, a java.lang.Object, no object reference available

Looks like the presence/absence of an address is contingent on other 
things ??


"MyWaitingThread" #13 prio=5 os_prio=0 cpu=0.27ms elapsed=2.63s 
tid=0x7f522c009000 nid=0x62d0 in Object.wait()  [0x7f526537d000]

    java.lang.Thread.State: WAITING (on object monitor)
Thread: 0x7f522c009000  [0x62d0] State: _at_safepoint 
_has_called_back 0 _at_poll_safepoint 0

    JavaThread state: _thread_blocked
 at java.lang.Object.wait(java.base@12-internal/Native Method)
 - waiting on 
 at java.lang.Object.wait(java.base@12-internal/Object.java:328)
 at WaitNotifyThreadTest$WaitThread.run(WaitNotifyThreadTest.java:80)
 - locked <0x00078653eeb0> (a java.lang.Object)

Or maybe there is a bug in the stack printing code thats printing the 
information?


Filed: https://bugs.openjdk.java.net/browse/JDK-8215199

David
-

On 9/12/2018 9:21 am, David Holmes wrote:

Hi Patricio,

On 9/12/2018 9:04 am, Patricio Chilano wrote:

Hi,

Could you review this small fix for test 
serviceability/tmtools/jstack/WaitNotifyThreadTest.java ?
After change 8214148 the test fails if the flag -Xcomp is used as 
explained in the bug details. The proposed change is to identified 
this special case and set the monitor address to match the one expected.


http://cr.openjdk.java.net/~pchilanomate/8215050.02/webrev/
https://bugs.openjdk.java.net/browse/JDK-8215050

Run tier-4 where the test was failing and passed (the test was 
failing on tier6 also because it uses -Xcomp too). Currently running 
tiers1-3.


Change seems fine. The lack of address must be something relatively 
new as I don't see it with Xcomp in JDK 9.


Thanks,
David


Thanks,
Patricio

Re: RFR 8215050: [TESTBUG] serviceability/tmtools/jstack/WaitNotifyThreadTest.java fails when run with flag -Xcomp

[David Holmes]

 This is still failing in tier4:

Error: incorrect monitor info: locked, a java.lang.Object, 
0x00078653eeb0

Expected: locked, a java.lang.Object, no object reference available

Looks like the presence/absence of an address is contingent on other 
things ??


"MyWaitingThread" #13 prio=5 os_prio=0 cpu=0.27ms elapsed=2.63s 
tid=0x7f522c009000 nid=0x62d0 in Object.wait()  [0x7f526537d000]

   java.lang.Thread.State: WAITING (on object monitor)
Thread: 0x7f522c009000  [0x62d0] State: _at_safepoint 
_has_called_back 0 _at_poll_safepoint 0

   JavaThread state: _thread_blocked
at java.lang.Object.wait(java.base@12-internal/Native Method)
- waiting on 
at java.lang.Object.wait(java.base@12-internal/Object.java:328)
at WaitNotifyThreadTest$WaitThread.run(WaitNotifyThreadTest.java:80)
- locked <0x00078653eeb0> (a java.lang.Object)

Or maybe there is a bug in the stack printing code thats printing the 
information?


Filed: https://bugs.openjdk.java.net/browse/JDK-8215199

David
-

On 9/12/2018 9:21 am, David Holmes wrote:

Hi Patricio,

On 9/12/2018 9:04 am, Patricio Chilano wrote:

Hi,

Could you review this small fix for test 
serviceability/tmtools/jstack/WaitNotifyThreadTest.java ?
After change 8214148 the test fails if the flag -Xcomp is used as 
explained in the bug details. The proposed change is to identified 
this special case and set the monitor address to match the one expected.


http://cr.openjdk.java.net/~pchilanomate/8215050.02/webrev/
https://bugs.openjdk.java.net/browse/JDK-8215050

Run tier-4 where the test was failing and passed (the test was failing 
on tier6 also because it uses -Xcomp too). Currently running tiers1-3.


Change seems fine. The lack of address must be something relatively new 
as I don't see it with Xcomp in JDK 9.


Thanks,
David


Thanks,
Patricio

Re: RFR (L) 8215160: Normalize spaces for remaining vmTestbase tests

[Alex Menkov]

Sorry, it was review for 8215161

Some minor notes for this webrev:
- nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/ji05t001.cpp
for some reason there are missed spaces before "?" in statements like
 (indx == 0)? "A" : "B"
see lines 190, 227, 243

Also there is some inconsistency with spaces around "%" operator -
in nsk/stress/jni/libjnistress001.cpp and nsk/stress/jni/libjnistress003.cpp
there are no spaces like
+if (allocs%printperiod == 0) {

in other files the spaces are added
like (nsk/stress/jni/libjnistress005.cpp)
+if (Exceptcalls % 1000 == 0)

--alex

On 12/10/2018 14:28, Alex Menkov wrote:

+1

One minor request.
Could you please fix
nsk/jvmti/NativeMethodBind/nativemethbind002/nativemethbind002.cpp
replacing

if (!(methNam == NULL))
and
if (!(methSig == NULL))

with
if (methNam != NULL)
and
if (methSig != NULL)

No need for new round of the review.

--alex


On 12/10/2018 13:54, serguei.spit...@oracle.com wrote:

Hi Jc,

LGTM

Thank you for the re-post!
Serguei


On 12/10/18 13:46, JC Beyler wrote:

Hi all,

Let's try this again; my apologies for the spam.

Could I get a review that normalizes spaces around comparisons and 
the ternary operator? This is the second of two webrevs to handle this.


Webrev: http://cr.openjdk.java.net/~jcbeyler/8215160/webrev.00/ 


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

Thanks,
Jc

Re: RFR (L) 8215160: Normalize spaces for remaining vmTestbase tests

[Alex Menkov]

+1

One minor request.
Could you please fix
nsk/jvmti/NativeMethodBind/nativemethbind002/nativemethbind002.cpp
replacing

if (!(methNam == NULL))
and
if (!(methSig == NULL))

with
if (methNam != NULL)
and
if (methSig != NULL)

No need for new round of the review.

--alex


On 12/10/2018 13:54, serguei.spit...@oracle.com wrote:

Hi Jc,

LGTM

Thank you for the re-post!
Serguei


On 12/10/18 13:46, JC Beyler wrote:

Hi all,

Let's try this again; my apologies for the spam.

Could I get a review that normalizes spaces around comparisons and the 
ternary operator? This is the second of two webrevs to handle this.


Webrev: http://cr.openjdk.java.net/~jcbeyler/8215160/webrev.00/ 


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

Thanks,
Jc

Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

[Sergei Ustimenko]

Martin,

>you probably won't be able to build openjdk on a system without bash

Most certainly not, it appears

I really wonder now if there's any policy/rule of thumb to not to use
/bin/sh.
I don't feel really comfortable if there would be such a policy.

I've checked build docs and it seems that troubleshooting section could be
a bit enhanced, though I guess that is given, that user knows how to
change a /bin/sh symlink. So I don't know if it worth to do this.

Regards,
Sergei

On Mon, 10 Dec 2018 at 22:27, Martin Buchholz  wrote:

> I don't know if there's an official policy on how ultra-portable tests are
> supposed to be.  In practice, you probably won't be able to build openjdk
> on a system without bash.
>
> On Mon, Dec 10, 2018 at 1:12 PM Sergei Ustimenko 
> wrote:
>
>> Hi Martin,
>>
>> That sounds good!
>>
>> I've counted all the sh-shebangs and it appears that
>> there are at least 66 of them inside the test/ directory,
>> where only 12 bashes.
>>
>> I've also ran the search  in order to identify all the
>> occurrences that use either [[ or == and found only
>> three of them that use "==". That one for example:
>>
>> http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63
>> of course `dash` reports failure in that case.
>>
>> So I'm quite hesitant in that case and not really sure
>> what to do. I haven't also found any existent JBS ticket
>> for such /bin/sh => /bin/bash a replacement.
>>
>> So any advise in this case would be appreciated!
>>
>> Regards,
>> Sergei
>>
>> On Mon, 10 Dec 2018 at 18:32, Martin Buchholz 
>> wrote:
>>
>>> I would not try to remove all bash-isms from openjdk. Instead I would
>>> find instances of /bin/sh that need to be changed to /bin/bash.
>>>
>>> (Ubuntu's use of /bin/sh -> /bin/dash is technically correct, but caused
>>> much suffering
>>> https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463
>>> )
>>>
>>

Re: RFR (L) 8215161: Normalize spaces for vmTestbase/[a-j]

[Martin Buchholz]

Looks good to me.

On Mon, Dec 10, 2018 at 1:49 PM JC Beyler  wrote:

> Hi all,
>
> Let's try this again; my apologies for the spam.
>
> Could I get a review that normalizes spaces around comparisons and the
> ternary operator? This is the first of two webrevs to handle this.
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8215161/webrev.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215161
>
> Thanks,
> Jc
>

Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

[David Holmes]

On 11/12/2018 7:27 am, Martin Buchholz wrote:
I don't know if there's an official policy on how ultra-portable tests 
are supposed to be.  In practice, you probably won't be able to build 
openjdk on a system without bash.


configure must be run with bash; so no bash, no build.

David

On Mon, Dec 10, 2018 at 1:12 PM Sergei Ustimenko > wrote:


Hi Martin,

That sounds good!

I've counted all the sh-shebangs and it appears that
there are at least 66 of them inside the test/ directory,
where only 12 bashes.

I've also ran the search  in order to identify all the
occurrences that use either [[ or == and found only
three of them that use "==". That one for example:

http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63
of course `dash` reports failure in that case.

So I'm quite hesitant in that case and not really sure
what to do. I haven't also found any existent JBS ticket
for such /bin/sh => /bin/bash a replacement.

So any advise in this case would be appreciated!

Regards,
Sergei

On Mon, 10 Dec 2018 at 18:32, Martin Buchholz mailto:marti...@google.com>> wrote:

I would not try to remove all bash-isms from openjdk. Instead I
would find instances of /bin/sh that need to be changed to
/bin/bash.

(Ubuntu's use of /bin/sh -> /bin/dash is technically correct,
but caused much suffering
https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463
)

Re: RFR (L) 8215161: Normalize spaces for vmTestbase/[a-j]

[serguei.spit...@oracle.com]

  
  
Hi Jc,
  
  LGTM
  
  Thanks,
  Serguei
  
  
  On 12/10/18 13:48, JC Beyler wrote:


  
Hi all,
  
  
  Let's try this again; my apologies for the spam.
  
  
  Could I get a review that normalizes spaces around
comparisons and the ternary operator? This is the first of
two webrevs to handle this.


Webrev: http://cr.openjdk.java.net/~jcbeyler/8215161/webrev.00/
  Bug: https://bugs.openjdk.java.net/browse/JDK-8215161

  


Thanks,
Jc
  

  

  


  

Re: RFR (L) 8215160: Normalize spaces for remaining vmTestbase tests

[serguei.spit...@oracle.com]

  
  
Hi Jc,
  
  LGTM
  
  Thank you for the re-post!
  Serguei
  
  
  On 12/10/18 13:46, JC Beyler wrote:


  Hi all,


Let's try this again; my apologies for the spam.


Could I get a review that normalizes spaces around
  comparisons and the ternary operator? This is the second of
  two webrevs to handle this.
  
  
  Webrev: http://cr.openjdk.java.net/~jcbeyler/8215160/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215160
  

  
  
  Thanks,
  Jc

  



  


  

Re: RFR (L) 8215161: Remove spaces in assignments for vmTestbase/[a-j]

[Martin Buchholz]

Looks good to me.  Make sure to "test" using
hg diff -wbB

On Mon, Dec 10, 2018 at 1:04 PM JC Beyler  wrote:

> Hi all,
>
> Could I get a review that adds a space around comparisons for the
> vmTestbase? This is the first of two webrevs to handle this.
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8215161/webrev.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215161
>
> Thanks,
> Jc
>

Re: RFR(L) 8212960: Remove spaces in assignments for remaining vmTestbase

[serguei.spit...@oracle.com]

  
  
Hi Jc,
  
  It looks good.
  
  The same comment from Martin applies to this.
  The bug title does not match the webrev update.
  One of the options is fix the bug title and re-post the RFR.
  
  Thanks,
  Serguei 
  
  
  On 12/10/18 13:02, JC Beyler wrote:


  

  Hi all,


Could I get a review that adds a space around
  comparisons for the vmTestbase? This is the second of two
  webrevs to handle this.
  
  
  Webrev: http://cr.openjdk.java.net/~jcbeyler/8215160/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215160
  

  
  
  Thanks,
  Jc

  

  

  


  

Re: RFR (L) 8215161: Remove spaces in assignments for vmTestbase/[a-j]

[serguei.spit...@oracle.com]

  
  
I'd suggest to say: "normalize spaces"
  or something like this.
  
  Thanks,
  Serguei
  
  
  On 12/10/18 13:31, Martin Buchholz wrote:


  


  On Mon, Dec 10, 2018 at 1:28 PM JC Beyler 
wrote:
  
  

  
  
  I know, Claes noticed it too and I modified the title
in the bug and the webrevs but the RFR is out.

  
  
  
  You should modify the bug Description to agree with the
bug title. 

  


  

Re: RFR (L) 8215161: Remove spaces in assignments for vmTestbase/[a-j]

[Martin Buchholz]

On Mon, Dec 10, 2018 at 1:28 PM JC Beyler  wrote:

>
> I know, Claes noticed it too and I modified the title in the bug and the
> webrevs but the RFR is out.
>

You should modify the bug Description to agree with the bug title.

Re: RFR (L) 8215161: Remove spaces in assignments for vmTestbase/[a-j]

[JC Beyler]

Hi Martin,

I know, Claes noticed it too and I modified the title in the bug and the
webrevs but the RFR is out. Should I send a new email thread for
's/Remove/Add' ?
Jc

On Mon, Dec 10, 2018 at 1:23 PM Martin Buchholz  wrote:

> The bug title says
> Add spaces
> but the description says
> Remove spaces
>
> On Mon, Dec 10, 2018 at 1:04 PM JC Beyler  wrote:
>
>> Hi all,
>>
>> Could I get a review that adds a space around comparisons for the
>> vmTestbase? This is the first of two webrevs to handle this.
>>
>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8215161/webrev.00/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8215161
>>
>> Thanks,
>> Jc
>>
>

-- 

Thanks,
Jc

Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

[Martin Buchholz]

I don't know if there's an official policy on how ultra-portable tests are
supposed to be.  In practice, you probably won't be able to build openjdk
on a system without bash.

On Mon, Dec 10, 2018 at 1:12 PM Sergei Ustimenko  wrote:

> Hi Martin,
>
> That sounds good!
>
> I've counted all the sh-shebangs and it appears that
> there are at least 66 of them inside the test/ directory,
> where only 12 bashes.
>
> I've also ran the search  in order to identify all the
> occurrences that use either [[ or == and found only
> three of them that use "==". That one for example:
>
> http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63
> of course `dash` reports failure in that case.
>
> So I'm quite hesitant in that case and not really sure
> what to do. I haven't also found any existent JBS ticket
> for such /bin/sh => /bin/bash a replacement.
>
> So any advise in this case would be appreciated!
>
> Regards,
> Sergei
>
> On Mon, 10 Dec 2018 at 18:32, Martin Buchholz  wrote:
>
>> I would not try to remove all bash-isms from openjdk. Instead I would
>> find instances of /bin/sh that need to be changed to /bin/bash.
>>
>> (Ubuntu's use of /bin/sh -> /bin/dash is technically correct, but caused
>> much suffering
>> https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463
>> )
>>
>

Re: RFR (L) 8215161: Remove spaces in assignments for vmTestbase/[a-j]

[Martin Buchholz]

The bug title says
Add spaces
but the description says
Remove spaces

On Mon, Dec 10, 2018 at 1:04 PM JC Beyler  wrote:

> Hi all,
>
> Could I get a review that adds a space around comparisons for the
> vmTestbase? This is the first of two webrevs to handle this.
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8215161/webrev.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215161
>
> Thanks,
> Jc
>

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Ali İnce]

Hi Alexey,

I’ve searched for |GetProcAddress| usages across source code and couldn’t find 
(hopefully tbh) other occurrences of such mismatches.

Regards,

Ali

> On 7 Dec 2018, at 20:24, Alexey Ivanov  wrote:
> 
> Hi Ali,
> 
> On 06/12/2018 22:49, Ali İnce wrote:
>> Hi Magnus, Alexey,
>> 
>> I believe we won’t be able to get further opinions from serviceability-dev.
> 
> Unfortunately, no one has replied as of now.
> Have you found any issues with jdwpTransport_OnLoad after removing JNICALL?
> 
>> Andrew Luo suggested using a similar mechanism as is used for jvm.dll by 
>> using symbol name files mapped by platform (files are under 
>> make/hotspot/symbols and interestingly windows is almost the only platform 
>> for which a symbol file doesn’t exist).
> 
> Andrew says the .def files are auto-generated for Windows. There's a set of 
> shared functions.
> JVM cannot change the calling convention, jvm.dll is the public interface to 
> JVM.
> 
>> Another issue, again opened against AdoptOpenJDK 32-bit builds is somehow 
>> related with the same problem - but this time it is jimage.dll depending on 
>> zip.dll expecting the ZIP_InflateFully function to be decorated with JNICALL 
>> - whereas it was removed earlier from the implementation and the runtime 
>> image just fails with access violation just because jimage source code still 
>> declared it with JNICALL 
>> (https://github.com/AdoptOpenJDK/openjdk-build/issues/763 
>> ).
> 
> The usage of ZIP_InflateFully from zip.dll by jimage.dll was overlooked 
> during work on https://bugs.openjdk.java.net/browse/JDK-8201226 
> .
> 
> I can take care of it.
> (I could not build 32 bit Windows. It seem to have overcome the problem by 
> adding --disable-warnings-as-errors option to configure.)
> 
> However, the report says the resulting image crashes in 64 bit windows too 
> which shouldn't be affected by JNICALL mismatch.
> 
>> I’ve also searched for GetProcAddress calls across the source code - and I 
>> think it’s important to work on the consistency of JNICALL usages across the 
>> whole source code.
> 
> There are many places where libraries are loaded dynamically or a function 
> may be unavailable on older versions of the platform.
> Have you identified any other place where functions from JDK DLLs are looked 
> up by names?
> 
>> Another noteworthy thing I’ve noticed is there are still JNICALL modifiers 
>> for example in 
>> https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49
>>  
>> 
>>  - which I guess will also be reported as broken since these functions are 
>> again name decorated.
> 
> This is a JNI method. It should use JNICALL modifier. If it wasn't found, the 
> class sun.security.pkcs11.Secmod would fail to load. I guess JVM handles name 
> mangling somehow for native method implementation.
> 
> Such functions were never explicitly exported using mapfiles or /export 
> options on Windows, at least in the client area. For Linux and Solaris, 
> adding or removing a native method required editing mapfiles.
> 
>> If we’re still determined to remove JNICALL, what about just removing 
>> __stdcall from JNICALL declaration at 
>> https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31
>>  
>> 
>>  ? Wouldn’t that be a more consistent move? 
> 
> We can't do that.
> Implementation of Java native methods must use __stdcall calling convention.
> 
>> 
>> Regards,
>> 
>> Ali
>> 
>>> On 22 Nov 2018, at 17:29, Magnus Ihse Bursie >> > wrote:
>>> 
>>> I think we are in complete agreement. What's missing is some expert opinion 
>>> from serviceability-dev if there is any problem with changing the 
>>> signature. I'd preferably have that. 
>>> 
>>> If no one knows, I'd say, change it, and see if we still pass the relevant 
>>> tests, and if so, ship it. 
>>> 
>>> /Magnus
>>> 
>>> 22 nov. 2018 kl. 18:04 skrev Alexey Ivanov >> >:
>>> 
 
 On 21/11/2018 12:16, Magnus Ihse Bursie wrote:
> 
> On 2018-11-20 16:49, Alexey Ivanov wrote:
> 
>> Hi Ali, Magnus,
>> 
>> I absolutely agree this change has to be reviewed by someone from 
>> serviceability.
>> 
>> There are several options:
>> 
>> 1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
>> http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
>>  
>> 
>> so it partially reverts the changes from
>> 

Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

[Sergei Ustimenko]

Hi Martin,

That sounds good!

I've counted all the sh-shebangs and it appears that
there are at least 66 of them inside the test/ directory,
where only 12 bashes.

I've also ran the search  in order to identify all the
occurrences that use either [[ or == and found only
three of them that use "==". That one for example:
http://hg.openjdk.java.net/jdk/sandbox/file/f94276ccc9fc/test/hotspot/jtreg/vmTestbase/jit/tiered/tieredTest.sh#l63
of course `dash` reports failure in that case.

So I'm quite hesitant in that case and not really sure
what to do. I haven't also found any existent JBS ticket
for such /bin/sh => /bin/bash a replacement.

So any advise in this case would be appreciated!

Regards,
Sergei

On Mon, 10 Dec 2018 at 18:32, Martin Buchholz  wrote:

> I would not try to remove all bash-isms from openjdk. Instead I would
> find instances of /bin/sh that need to be changed to /bin/bash.
>
> (Ubuntu's use of /bin/sh -> /bin/dash is technically correct, but caused
> much suffering
> https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463
> )
>

Re: [PATCH] Windows 32-bit DLL name decoration

[Ali İnce]

> On 10 Dec 2018, at 15:11, Magnus Ihse Bursie  
> wrote:
> 
> 
> 
> On 2018-12-10 16:02, Alexey Ivanov wrote:
>> 
>> 
>> On 10/12/2018 10:41, Magnus Ihse Bursie wrote:
>>> 
>>> 
>>> On 2018-12-07 22:18, Simon Tooke wrote:
 Looking at the code, it seems (to me) the "correct" thing to do is to is
 to create a Windows-specific version of dbgsysBuildFunName() in
 linker_md.c, and have it decorate the function name as appropriate for
 32-bit windows.  Note that in transport.c, you'd need to pass in the
 parameter stack size (the bit after the @), but it could at least behave
 properly on 32 vs 64 bit Windows.
 
 Notice this approach is already used for the library name, via
 dbgsysBuildLibName()
 
 If the function is never intended to be called by Java via JNI, then it
 should never have been declared JNICALL in the first place - but that
 ship has sailed.
 
 I don't think changing the decorated exported name to undecorated is a
 good idea, as it obfuscates the calling convention used.
 
>>> Good analysis, Simon. I agree. 
>>> 
>>> I have now looked more into the situation. In fact, it looks a lot like 
>>> JDWP has been broken on Windows 32-bit for a long time, but we have patched 
>>> around the issue in the JDK by using /export. This is the spec we're 
>>> dealing with: 
>>> https://docs.oracle.com/javase/10/docs/specs/jdwp/jdwp-transport.html. I 
>>> quote:
>>> 
 The transport library must export an onload function with the following 
 prototype:
 
 JNIEXPORT jint JNICALL
 jdwpTransport_OnLoad(JavaVM *jvm,
  jdwpTransportCallback *callback,
  jint version,
  jdwpTransportEnv** env);
 
 This function will be called by the JDWP (or other agent) when the library 
 is loaded.
 
 
>>> 
>>> Nothing here says anything that the function should be exported using 
>>> anything other than the normal _stdcall (implied by JNICALL) name mangling 
>>> rules. However, JDWP has not been working according to the spec and looking 
>>> for the correct symbol, and while we could have noticed that in the JDK, we 
>>> didn't, because someone (long ago) added /export:jdwpTransport_OnLoad as a 
>>> workaround to the affected libraries, instead of fixing JDWP.
>> 
>> This means that we cannot change the calling convention: it must stay as it 
>> is now.
>> 
>> I think JDWP has always been working according to the spec. Using __stdcall 
>> is the recommended calling convention for Win32 and for DLLs in particular. 
>> Usually function names are exported undecorated in DLL. (If my memory serves 
>> me well, older Microsoft tools exported extern "C" __stdcall functions 
>> undecorated.)
> So then the question becomes: what does the spec mean? That the __stdcall 
> convention should be used, or that the function name should be explicitly 
> exported under a non __stdcall-convention name? Neither behavior seems 
> clearly written out, so this is left to an implicit interpretation of what 
> that "usually" means..?

A couple of MSFT quotes on name decoration from 
https://docs.microsoft.com/en-gb/cpp/build/reference/decorated-names?view=vs-2017
 says;

> Normally, you don't have to know the decorated name to write code that 
> compiles and links successfully. Decorated names are an implementation detail 
> internal to the compiler and linker.

> Name decoration is also important when linking to code written in other 
> programming languages or using other compilers. Different compilers use 
> different name decoration conventions. When your executable links to code 
> written in another language, special care must be taken to match the exported 
> and imported names and calling conventions.

And we should also keep in mind that MSFT itself doesn’t use name mangling in 
the core windows API.

I still see this |jdwpTransport_OnLoad| not much different than 
|JNI_CreateJavaVM| in the context of it’s availability and accessibility from 
outer world.

>> 
>> I believe this — exporting the undecorated function names — allows for 
>> interoperability between 32 bit and 64 bit in cases where you load a DLL and 
>> dynamically look up a function in it.
>> 
>> There were too many /export options to export Win32 functions undecorated 
>> for this one to be an accident rather than the intention.
> How do you mean? 
>> 
>>> Now, given that we've shipped code that didn't adhere to the specs for so 
>>> long, we can probably not break that behavior. Instead, I propose we amend 
>>> dbgsysBuildFunName() so that on Windows 32-bit, it looks for both the 
>>> "jdwpTransport_OnLoad", and the symbol as mangled by _stdcall rules. I also 
>>> propose that if both symbols are present, the _stdcall named one should 
>>> take precedence, since that's according to the spec (and the other is just 
>>> a fallback to maintain backwards compatibility).
>> 
>> Since removing JNICALL is 

RFR (L) 8215161: Remove spaces in assignments for vmTestbase/[a-j]

[JC Beyler]

Hi all,

Could I get a review that adds a space around comparisons for the
vmTestbase? This is the first of two webrevs to handle this.

Webrev: http://cr.openjdk.java.net/~jcbeyler/8215161/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215161

Thanks,
Jc

RFR(L) 8212960: Remove spaces in assignments for remaining vmTestbase

[JC Beyler]

Hi all,

Could I get a review that adds a space around comparisons for the
vmTestbase? This is the second of two webrevs to handle this.

Webrev: http://cr.openjdk.java.net/~jcbeyler/8215160/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215160

Thanks,
Jc

Re: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

[Martin Buchholz]

I would not try to remove all bash-isms from openjdk. Instead I would find
instances of /bin/sh that need to be changed to /bin/bash.

(Ubuntu's use of /bin/sh -> /bin/dash is technically correct, but caused
much suffering
https://bugs.launchpad.net/ubuntu/+source/dash/+bug/61463
)

Re: RFR: JDK-8210106: sun/tools/jps/TestJps.java timed out

[Daniel D. Daugherty]

On 12/10/18 2:55 AM, David Holmes wrote:

Hi Dan,


Gary saw a longest time value of 280 seconds in his testing with
fastdebug bits. The default timeout value is 120 seconds with a
default timeout factor of 4 in Mach5 gives a total timeout of
480 seconds (8 minutes). With Gary's new default timeout of
360 seconds, we'll have a total timeout value of 1440 seconds
(24 minutes). 


So based on that it seems Gary's observations are not relevant to what 
has been seen in mach5 because at 280 seconds we should never have 
been timing out!


Correct. Gary's test runs never ran into the same state as the
end-of-August run where the test ran into the 20 minute timeout.
Since those logs are now gone, we cannot discern any more information
about what happened back then...

Dan




I guess these changes won't hurt.

Thanks,
David

On 8/12/2018 1:12 am, Daniel D. Daugherty wrote:

On 12/7/18 7:58 AM, Gary Adams wrote:

On 12/6/18, 7:52 PM, David Holmes wrote:

Hi Gary,

On 6/12/2018 11:27 pm, Gary Adams wrote:
On a local linux-x64-debug build this test consistently runs in 
less than 40 seconds.
On the mach5 testing machines there was a large fluctuation in the 
time to complete this test.


Since the test runs jps with different combinations of arguments, 
the total
test time is dependent on the number of processes running java 
programs.
Since the mach5 test infrastructure runs java programs I have seen 
a 3X

in the amount of output the test produces compared to local test
runs.

I've run the test several hundred times through mach5 on the 
slower platforms
and then examined the test logs using a 3X setting from the 
default 120 second
jtreg timeout. The slowest reported elapse time from the test logs 
showed

280 seconds to complete.

To improve the reliability of the test to complete, I'd like to 
increase the

timeout to 360 seconds.

   Issue: https://bugs.openjdk.java.net/browse/JDK-8210106

Proposed fix:

diff --git a/test/jdk/sun/tools/jps/TestJps.java 
b/test/jdk/sun/tools/jps/TestJps.java

--- a/test/jdk/sun/tools/jps/TestJps.java
+++ b/test/jdk/sun/tools/jps/TestJps.java
@@ -27,7 +27,7 @@
   * @modules jdk.jartool/sun.tools.jar
   * @build LingeredAppForJps
   * @build LingeredApp
- * @run main/othervm TestJps
+ * @run main/othervm/timeout=360 TestJps
   */


Doesn't that then get scaled by the timeout factor resulting in a 
much longer timeout than the 360 seconds you intended?


For other timeout adjustments the needed time has been divided by 
the timeout factor to get the actual intended timeout.


This bug was filed fairly recently in Aug 2018.
At that time the timeout and timeout factor were not sufficient
to avoid the test failing.

The mach5 timeout factors were adjusted recently, so this test may
no longer be an issue.

If that is true, then we could simply close this bug as "cannot 
reproduce".

An argument could be made that the change in timeout factor may be
responsible for fixing a lot more of the intermittent bugs and that 
they

should be closed in a similar manner.

Historically, we could say this particular bug should have had timeouts
reassessed when the infrastructure switched from jprt to mach5 testing
where there were more visible Java processes running.

Using a higher explicit timeout will not make the test run any longer
than it needs. It will simply allow the test to not be terminated 
sooner

in a hung test scenario.

What is your preference for this particular issue:
   - increase the explicit timeout
   - close as cannot reproduce attributed to the timeout factor 
adjustments


What would you recommend going forward for other similar issues:
   - determine a new explicit timeout
   - close if no timeout failures have been observed since the 
timeout factor

  was raised


General guidance that I've been giving folks is that you bump the
default timeout when you see timeouts with 'release' bits with
a timeout factor of '1'. The default timeout factor in Mach5 is 4
so we run both 'release' and 'fastdebug' bits with that timeout
factor. That means our total timeout value for 'release' bits is
probably a little long and our total timeout value for 'fastdebug'
bits is probably just right for a concurrency factor of 12.

On my personal servers I use the following timeout factors:

 release)
 TIMEOUT_FACTOR="4"
 ;;
 fastdebug)
 TIMEOUT_FACTOR="6"
 ;;
 slowdebug)
 TIMEOUT_FACTOR="12"
 ;;

However, I run with a concurrency factor of 16. I rarely see
timeout failures.

All that is well and good, but a 'jps' test is a different
beast since it is affected by factors outside of most tests.
Since the number of java processes running on the system can
affect the test, I'm okay with using a default timeout of '360'
for this test as a guard against increases in load or...

Gary saw a longest time value of 280 seconds in his testing with
fastdebug bits. The default timeout value is 120 seconds 

Re: optimize KlassInfoTable size to power of 2

[Andrew Haley]

On 12/10/18 2:46 PM, 臧琳 wrote:
> Would you like to give more details about the benefit of using hash table 
> with prime 
> size?

Really? OK.

Imagine that your hash table is an exact power of two in size. Reducing the
index mod any power of two means using only the lower n bits of the hash
value.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: [PATCH] Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-10 16:02, Alexey Ivanov wrote:



On 10/12/2018 10:41, Magnus Ihse Bursie wrote:



On 2018-12-07 22:18, Simon Tooke wrote:

Looking at the code, it seems (to me) the "correct" thing to do is to is
to create a Windows-specific version of dbgsysBuildFunName() in
linker_md.c, and have it decorate the function name as appropriate for
32-bit windows.  Note that in transport.c, you'd need to pass in the
parameter stack size (the bit after the @), but it could at least behave
properly on 32 vs 64 bit Windows.

Notice this approach is already used for the library name, via
dbgsysBuildLibName()

If the function is never intended to be called by Java via JNI, then it
should never have been declared JNICALL in the first place - but that
ship has sailed.

I don't think changing the decorated exported name to undecorated is a
good idea, as it obfuscates the calling convention used.

Good analysis, Simon. I agree.

I have now looked more into the situation. In fact, it looks a lot 
like JDWP has been broken on Windows 32-bit for a long time, but we 
have patched around the issue in the JDK by using /export. This is 
the spec we're dealing with: 
https://docs.oracle.com/javase/10/docs/specs/jdwp/jdwp-transport.html. 
I quote:


The transport library must export an/onload/function with the 
following prototype:


|JNIEXPORT jint JNICALL jdwpTransport_OnLoad(JavaVM *jvm, 
jdwpTransportCallback *callback, jint version, jdwpTransportEnv** env);|


This function will be called by the JDWP (or other agent) when the 
library is loaded.





Nothing here says anything that the function should be exported using 
anything other than the normal _stdcall (implied by JNICALL) name 
mangling rules. However, JDWP has not been working according to the 
spec and looking for the correct symbol, and while we could have 
noticed that in the JDK, we didn't, because someone (long ago) added 
/export:jdwpTransport_OnLoad as a workaround to the affected 
libraries, instead of fixing JDWP.


This means that we cannot change the calling convention: it must stay 
as it is now.


I think JDWP has always been working according to the spec. Using 
__stdcall is the recommended calling convention for Win32 and for DLLs 
in particular. Usually function names are exported undecorated in DLL. 
(If my memory serves me well, older Microsoft tools exported |extern 
"C" __stdcall| functions undecorated.)
So then the question becomes: what does the spec mean? That the 
__stdcall convention should be used, or that the function name should be 
explicitly exported under a non __stdcall-convention name? Neither 
behavior seems clearly written out, so this is left to an implicit 
interpretation of what that "usually" means..?


I believe this — exporting the undecorated function names — allows for 
interoperability between 32 bit and 64 bit in cases where you load a 
DLL and dynamically look up a function in it.


There were too many /export options to export Win32 functions 
undecorated for this one to be an accident rather than the intention.

How do you mean?


Now, given that we've shipped code that didn't adhere to the specs 
for so long, we can probably not break that behavior. Instead, I 
propose we amend dbgsysBuildFunName() so that on Windows 32-bit, it 
looks for both the "jdwpTransport_OnLoad", and the symbol as mangled 
by _stdcall rules. I also propose that if both symbols are present, 
the _stdcall named one should take precedence, since that's according 
to the spec (and the other is just a fallback to maintain backwards 
compatibility).


Since removing JNICALL is not an option, there are only two options:

1. Add |/export| option to the Makefile or pragma-comment to the 
source file;
2. Lookup the decorated name |_jdwpTransport_OnLoad@16| for Win32 with 
fallback to undecorated one.

Yes.

I think the correct solution here is 2.



I just wonder how it's possible to implement a generic 
|dbgsysBuildFunName| for an arbitrary function without knowing the 
size of function parameters. Could it be the reason why most DLLs 
export __stdcall functions undecorated?
(I assume you mean dbgsysFindLibraryEntry; there is a dbgsysBuildFunName 
as well but it appears to be dead code.)


The dbgsysFindLibraryEntry does not need to work with an arbitrary 
function. It's implemented in jdk.jdwp.agent, for the sole reason of 
locating the jdwpTransport_OnLoad function.


In general, I assume this to hold true. If you don't know the signature 
of the function you want to call, you're screwed anyway, regardless of 
calling convention.


/Magnus



Regards,
Alexey



/Magnus

-Simon Tooke


On 12/7/2018 2:09 PM, Alexey Ivanov wrote:

Hi Andrew,

Sorry for my belated reply.
Yes, it's also an option… however, this solution seems to be
overcomplicated to export one function or two. The calling convention
of exported functions for JVM cannot be changed, they can be called
from third-party software.

If the function is used internally-only, its calling 

RE: RFR (M) 8214892: Delayed starting of debugging via jcmd

[Schmelter, Ralf]

Hi Christoph,

> I have thought about a more generic name for the option rather than
> "onjcmd". What about "standby=y". That would indicate that the
> debugging agent is on standby and can be triggered by whatever
> means. What do others think?

I'm not sure making the name more generic is the right move, when it will 
probably be enabled via the jcmd in >95% of the cases.

> I'd prefer  if you write out either "Debugging has been started." 
> or "Debugging is already active.". I think this would make it more
> clear for the user of the feature what actually happened.

OK, I've changed that.

> A better wording would be:
> "Starts up the Java debugging if the jdwp agentlib was enabled with the 
> option onjcmd=y."

OK.

> replace "debug triggered by jcmd?" with " start debug via jcmd"

Ok.

> Is it really necessary/desired to set suspendOnInit to false? We
> could still honor suspend=y|n. The suspension will of course
> happen at the time debug activation is triggered.

I don't think it would make sense to support suspend=y. In makes sense when 
starting the debugging directly (so no, or at least not much) Java code can be 
executed and you can debug from the beginning. And it also makes sense for the 
onthrow/onuncaught, since you can to see the exceptions in the debugger. But 
when triggered from the outside, you have no natural event to wait for and no 
state to preserve. The only effect you would see is that the jcmd would not 
return, since the initialize method would be blocked until a debugger has been 
connected.

I've updated the webrev: 
http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.03/

Best regards,
Ralf Schmelter


-Original Message-
From: Langer, Christoph 
Sent: Freitag, 7. Dezember 2018 09:41
To: Schmelter, Ralf ; Chris Plummer 
; serviceability-dev@openjdk.java.net
Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd

Hi Ralf,

thanks for doing this change. Overall, it looks very good to me and seems to be 
a nice feature.

I have thought about a more generic name for the option rather than "onjcmd". 
What about "standby=y". That would indicate that the debugging agent is on 
standby and can be triggered by whatever means. What do others think?

Here are some minor comments, mostly about the wording of text messages:

src/hotspot/share/services/diagnosticCommand.cpp, line 1097:
I'd prefer  if you write out either "Debugging has been started." or "Debugging 
is already active.". I think this would make it more clear for the user of the 
feature what actually happened.

src/hotspot/share/services/diagnosticCommand.hpp, line 878:
"Starts up the Java debugging if enabled in the jdwp agentlib via the onjcmd=y 
option."
A better wording would be:
"Starts up the Java debugging if the jdwp agentlib was enabled with the option 
onjcmd=y."

src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:
line 876:
replace "debug triggered by jcmd?" with " start debug via jcmd" (or "start 
debug on request" if we opt to change the option name from onjcmd to something 
more general)
line 1300:
 suspendOnInit = JNI_FALSE;
Is it really necessary/desired to set suspendOnInit to false? We could still 
honor suspend=y|n. The suspension will of course happen at the time debug 
activation is triggered.

Another question: Do we need a CSR for this?

Best regards
Christoph

> -Original Message-
> From: serviceability-dev  On
> Behalf Of Schmelter, Ralf
> Sent: Donnerstag, 6. Dezember 2018 15:54
> To: Chris Plummer ; serviceability-
> d...@openjdk.java.net
> Subject: [CAUTION] RE: RFR (M) 8214892: Delayed starting of debugging via
> jcmd
> 
> Hi Chris,
> 
> > I think I prefer passing EI_VM_INIT for triggering_ei, but if you prefer
> > to keep it as is, I suggest a comment to clarify why it might be out of
> > range.
> 
> Actually, I used EI_VM_INIT for the longest time and only changed it
> recently, because I thought that code could assume that e.g. no classes have
> been loaded yet when getting the INIT_VM event. But since the JVMTI spec
> does not guarantees this in any way (it allows other events to be send before
> a VM_INIT), I just will change it back to use EI_VM_INIT for the initialize 
> call.
> 
> Regarding the name of the option, I agree that onjcmd, while not technically
> fully accurate, makes most sense for the common user.
> 
> > ... It think you could just return the error right away and remove
> > the error checking code that comes later.
> 
> I've changed the code to just return the error directly.
> 
> > It's not clear to me why you want the name and address of the first
> > transport. Why is the first one necessarily the one you want?
> 
> Since currently the bag of transports must always have a size of 1, getting 
> the
> first or the last transport is the same. But the callback function used to 
> iterate
> the bag has to return a boolean value. I've decided to return JNI_FALSE,
> which would mean the iteration stops at the first entry.
> 

Fwd: [PATCH] JDK-8025886: Replace [[ and == bash extensions in tests

[Sergei Ustimenko]

Hi,

I've recently been working on bug
https://bugs.openjdk.java.net/browse/JDK-8214052
that is mainly about cleaning up the [[ and == bash
extensions from the tests.

I've discovered, that there's a leftover after hg forest
consolidation. GeneratePropertyPassword.sh script
still uses beforementioned bash extensions. That has
been fixed within the scope of this bug:
https://bugs.openjdk.java.net/browse/JDK-8025886
Though fix didn't get to the consolidated forest.

The fix is trivial and just reapplies the patch from the
JBS' comments. Change has been tested with `dash`
and works fine.

Please find the patch inlined below. That would also be
great if anyone could sponsor it.

diff --git
a/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
b/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
---
a/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
+++
b/test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
@@ -34,12 +34,13 @@
 OS=`uname -s`
 UMASK=`umask`

-if [[ $OS == CYGWIN_NT* ]] ; then
+case $OS in
+CYGWIN_NT*)
 OS="Windows_NT"
 if [ -z "$SystemRoot" ] ;  then
-SystemRoot=`cygpath $SYSTEMROOT`
+SystemRoot=$SYSTEMROOT
 fi
-fi
+esac

 case $OS in
 SunOS | Linux | Darwin | AIX )


Thanks,
Sergei

Re: optimize KlassInfoTable size to power of 2

[Andrew Haley]

On 12/10/18 6:24 AM, 臧琳 wrote:
>  My observation is that when “jmap histo” iterating objects , the 
> object’s klass
>is identified and then hash idx in KlassInfoTable’s buckets[] is 
> calculated by mod
> of _num_bucktes, which would issue a heavy instruction idiv on x86 platform. 
> It
> means for every object scanned, a idiv instruction is issued, which lag the 
> performance espically when
> there are large number of objects in heap.

I'm surprised that GCC uses idiv in this case: it has an optimization for
constant modulo which doesn't use division. There is sometimes an
advantage for using hash tables of prime size. Is it that the size of
the table is not a constant that GCC can see?

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: (S): JDK-8200613: SA: jstack throws UnmappedAddressException with a CDS core file

[Jini George]

Thank you very much, Kevin!

- Jini.

On 12/10/2018 3:09 PM, Kevin Walls wrote:

Thanks for adding the flag Jini, all looks good to me too,

Thanks
Kevin


On 10/12/2018 06:44, Jini George wrote:

Thank you very much, Ioi!

- Jini.

On 12/10/2018 12:01 PM, Ioi Lam wrote:

Hi Jini,

These changes look good to me. Thanks!

- Ioi


On 12/7/18 11:22 AM, Jini George wrote:

I have the revised webrev here:

http://cr.openjdk.java.net/~jgeorge/8200613/webrev.02/index.html

The extra changes here are to:

* Introduce a new option DumpPrivateMappingsInCore to control the 
dumping of the file backed private regions into the corefile.


* Close the modules file before dumping core in os::abort(). 
Currently, there is a small bug 
(https://bugs.openjdk.java.net/browse/JDK-8215026) which prevents 
the closure of the image file in unmapping the entire file.


I plan to take up the unmapping of NIO MapMode.PRIVATE files as a 
separate task (https://bugs.openjdk.java.net/browse/JDK-8215027) 
since this seems a bit involved.


Thanks a bunch,
Jini.

On 11/12/2018 10:26 AM, Jini George wrote:

Thank you very much, Chris, Kevin and Ioi for your comments!

I will send another webrev with this change enabled under an 
opt-out flag, as you suggest, and would look at unmapping the JDK 
modules file and if possible, the NIO mapped files too in the 
signal handler.


Thanks a bunch,
Jini.

On 11/9/2018 11:53 PM, Ioi Lam wrote:

Hi Jini,

Thanks for investigating the size expansion issue.

I agree that the size increase is worth it. Even when not using 
SA, if we open the core file inside GDB, we cannot read certain 
sections in the CDS archive (such as the RO section and strings 
sections). That would make debugging difficult. So I am in favor 
of this change.


For the JDK modules file, maybe we can unmap it in the signal 
handler, before going ahead with the core dump? I think it's 
hardly needed for debugging purposes. (Perhaps we can also do the 
same for the NIO mapped files?)


A opt-flag as suggested by Kevin is a good idea.

Thanks

- Ioi

On 11/9/18 3:29 AM, Kevin Walls wrote:

Hi Jini,

Looks good to me.  It might be a significant increase in size of 
_some_ core files, but so many core files we see are much larger, 
in gigabytes++ of course, so the CDS data size should not be such 
a significant increase on (I think) most files.


The flexibiity of always having the CDS data there is very 
significant.  A core file should ideally be usable, without 
additionally requiring the CDS archive from the machine. That 
additional human round-trip upload request on every transmitted 
core that needs investigating, seems like a less efficient 
route...).


Is there an opt-out?  It's conditional on UseSharedSpaces but 
could there be a flag to disable, in case we see crashes with 
gigabytes of private mappings that we really don't want to retain 
(the user would have to know to set a flag, to disable the new 
coredump filter ahead of time).


Thanks!
Kevin


On 29/10/2018 06:02, Jini George wrote:
Thank you very much, Ioi, for looking into this, and the 
clarification offline. My bad, I had missed the earlier mail 
from you. :-( My responses below.


Yes, I had tested this on MacOS. The issue does not exist on 
MacOS since the file backed private mmap()-ed regions get dumped 
into the MacOS corefiles by default.


The corefile sizes on Linux do increase due to this change. And 
the increase would also include any file mapped using NIO with 
MapMode.PRIVATE. The typical corefile size increase with this 
change would include the following components at a high level:


* Any NIO file mapping with MapMode.PRIVATE.
* Any file mmap()-ed by any native library with MAP_PRIVATE.
* The read only CDS regions (ro and od): Of the order of a few MB.
* The shared strings CDS region. (typically less than 1 MB).
* 2 MB per native shared library (regions with ---p permissions 
mapped by the dynamic linker for better alignment and for 
keeping libraries efficiently shareable).

* The JDK 'modules' file. (About 140 MB).

So, without including any NIO mapping, I typically see around 
250-300 MB increase in the corefile sizes. I agree that the size 
increase could be a cause for concern, but for FWIW, these 
privately mapped files get dumped into the corefile for MacOS 
too. And the corefile sizes for the same program on MacOS are 
way larger (of the order of a few GB as against about 300 MB on 
Linux (without the change)).


The advantage of fixing this by modifying the coredump_filter 
v/s doing it in SA (by reading in more sections of the shared 
archive file) is that this would benefit other debuggers like 
gdb also. (And reduces the dependence on having the shared 
archive file being available at the time of debugging). If folks 
still think this is a cause for concern, I could make 
modifications to fix this by reading in the regions from the 
shared archive file in the SA code. I also wonder if it is worth 
taking a relook at the mapping types 

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Magnus Ihse Bursie]

On 2018-12-07 21:24, Alexey Ivanov wrote:

Hi Ali,

On 06/12/2018 22:49, Ali İnce wrote:

Hi Magnus, Alexey,

I believe we won’t be able to get further opinions from 
serviceability-dev.


Unfortunately, no one has replied as of now.
Have you found any issues with jdwpTransport_OnLoad after removing 
JNICALL?


Andrew Luo suggested using a similar mechanism as is used for jvm.dll 
by using symbol name files mapped by platform (files are under 
make/hotspot/symbols and interestingly windows is almost the only 
platform for which a symbol file doesn’t exist).


Andrew says the .def files are auto-generated for Windows. There's a 
set of shared functions.
JVM cannot change the calling convention, jvm.dll is the public 
interface to JVM.


Another issue, again opened against AdoptOpenJDK 32-bit builds is 
somehow related with the same problem - but this time it is 
jimage.dll depending on zip.dll expecting the ZIP_InflateFully 
function to be decorated with JNICALL - whereas it was removed 
earlier from the implementation and the runtime image just fails with 
access violation just because jimage source code still declared it 
with JNICALL (https://github.com/AdoptOpenJDK/openjdk-build/issues/763).


The usage of ZIP_InflateFully from zip.dll by jimage.dll was 
overlooked during work on 
https://bugs.openjdk.java.net/browse/JDK-8201226.


I can take care of it.
It might be worthwhile to scan the entire code base for JNICALL and 
verify that we have no more mismatches. In general, JNICALL should be 
preserved on all officially supported, exported interfaces. It need not 
be present on interfaces used only internally, and my current thinking 
is that it would be better if it were removed on all internal 
interfaces. But at the very least, it should be consistently specificed 
where exported and imported. (Any misses here is probably due to 
duplicate declarations, instead of properly including header files, so 
that's the correct way to resolve any problems found...)


/Magnus

(I could not build 32 bit Windows. It seem to have overcome the 
problem by adding --disable-warnings-as-errors option to configure.)


However, the report says the resulting image crashes in 64 bit windows 
too which shouldn't be affected by JNICALL mismatch.


I’ve also searched for GetProcAddress calls across the source code - 
and I think it’s important to work on the consistency of JNICALL 
usages across the whole source code.


There are many places where libraries are loaded dynamically or a 
function may be unavailable on older versions of the platform.
Have you identified any other place where functions from JDK DLLs are 
looked up by names?


Another noteworthy thing I’ve noticed is there are still JNICALL 
modifiers for example in 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49 - 
which I guess will also be reported as broken since these functions 
are again name decorated.


This is a JNI method. It should use JNICALL modifier. If it wasn't 
found, the class sun.security.pkcs11.Secmod would fail to load. I 
guess JVM handles name mangling somehow for native method implementation.


Such functions were never explicitly exported using mapfiles or 
/export options on Windows, at least in the client area. For Linux and 
Solaris, adding or removing a native method required editing mapfiles.


If we’re still determined to remove JNICALL, what about just removing 
__stdcall from JNICALL declaration at 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31 ? Wouldn’t 
that be a more consistent move?


We can't do that.
Implementation of Java native methods must use __stdcall calling 
convention.




Regards,

Ali

On 22 Nov 2018, at 17:29, Magnus Ihse Bursie 
> wrote:


I think we are in complete agreement. What's missing is some expert 
opinion from serviceability-dev if there is any problem with 
changing the signature. I'd preferably have that.


If no one knows, I'd say, change it, and see if we still pass the 
relevant tests, and if so, ship it.


/Magnus

22 nov. 2018 kl. 18:04 skrev Alexey Ivanov >:




On 21/11/2018 12:16, Magnus Ihse Bursie wrote:


On 2018-11-20 16:49, Alexey Ivanov wrote:


Hi Ali, Magnus,

I absolutely agree this change has to be reviewed by someone from 
serviceability.


There are several options:

1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
so it partially reverts the changes from
https://bugs.openjdk.java.net/browse/JDK-8200178

2. Remove JNICALL (__stdcall) modifier, eventually changing the 
calling convention to __cdecl.

http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html

3. Use inline /export option via #pragma:
#pragma comment(linker, "/export:jdwpTransport_OnLoad= 

Re: RFR: (S): JDK-8200613: SA: jstack throws UnmappedAddressException with a CDS core file

[Kevin Walls]

Thanks for adding the flag Jini, all looks good to me too,

Thanks
Kevin


On 10/12/2018 06:44, Jini George wrote:

Thank you very much, Ioi!

- Jini.

On 12/10/2018 12:01 PM, Ioi Lam wrote:

Hi Jini,

These changes look good to me. Thanks!

- Ioi


On 12/7/18 11:22 AM, Jini George wrote:

I have the revised webrev here:

http://cr.openjdk.java.net/~jgeorge/8200613/webrev.02/index.html

The extra changes here are to:

* Introduce a new option DumpPrivateMappingsInCore to control the 
dumping of the file backed private regions into the corefile.


* Close the modules file before dumping core in os::abort(). 
Currently, there is a small bug 
(https://bugs.openjdk.java.net/browse/JDK-8215026) which prevents 
the closure of the image file in unmapping the entire file.


I plan to take up the unmapping of NIO MapMode.PRIVATE files as a 
separate task (https://bugs.openjdk.java.net/browse/JDK-8215027) 
since this seems a bit involved.


Thanks a bunch,
Jini.

On 11/12/2018 10:26 AM, Jini George wrote:

Thank you very much, Chris, Kevin and Ioi for your comments!

I will send another webrev with this change enabled under an 
opt-out flag, as you suggest, and would look at unmapping the JDK 
modules file and if possible, the NIO mapped files too in the 
signal handler.


Thanks a bunch,
Jini.

On 11/9/2018 11:53 PM, Ioi Lam wrote:

Hi Jini,

Thanks for investigating the size expansion issue.

I agree that the size increase is worth it. Even when not using 
SA, if we open the core file inside GDB, we cannot read certain 
sections in the CDS archive (such as the RO section and strings 
sections). That would make debugging difficult. So I am in favor 
of this change.


For the JDK modules file, maybe we can unmap it in the signal 
handler, before going ahead with the core dump? I think it's 
hardly needed for debugging purposes. (Perhaps we can also do the 
same for the NIO mapped files?)


A opt-flag as suggested by Kevin is a good idea.

Thanks

- Ioi

On 11/9/18 3:29 AM, Kevin Walls wrote:

Hi Jini,

Looks good to me.  It might be a significant increase in size of 
_some_ core files, but so many core files we see are much larger, 
in gigabytes++ of course, so the CDS data size should not be such 
a significant increase on (I think) most files.


The flexibiity of always having the CDS data there is very 
significant.  A core file should ideally be usable, without 
additionally requiring the CDS archive from the machine. That 
additional human round-trip upload request on every transmitted 
core that needs investigating, seems like a less efficient 
route...).


Is there an opt-out?  It's conditional on UseSharedSpaces but 
could there be a flag to disable, in case we see crashes with 
gigabytes of private mappings that we really don't want to retain 
(the user would have to know to set a flag, to disable the new 
coredump filter ahead of time).


Thanks!
Kevin


On 29/10/2018 06:02, Jini George wrote:
Thank you very much, Ioi, for looking into this, and the 
clarification offline. My bad, I had missed the earlier mail 
from you. :-( My responses below.


Yes, I had tested this on MacOS. The issue does not exist on 
MacOS since the file backed private mmap()-ed regions get dumped 
into the MacOS corefiles by default.


The corefile sizes on Linux do increase due to this change. And 
the increase would also include any file mapped using NIO with 
MapMode.PRIVATE. The typical corefile size increase with this 
change would include the following components at a high level:


* Any NIO file mapping with MapMode.PRIVATE.
* Any file mmap()-ed by any native library with MAP_PRIVATE.
* The read only CDS regions (ro and od): Of the order of a few MB.
* The shared strings CDS region. (typically less than 1 MB).
* 2 MB per native shared library (regions with ---p permissions 
mapped by the dynamic linker for better alignment and for 
keeping libraries efficiently shareable).

* The JDK 'modules' file. (About 140 MB).

So, without including any NIO mapping, I typically see around 
250-300 MB increase in the corefile sizes. I agree that the size 
increase could be a cause for concern, but for FWIW, these 
privately mapped files get dumped into the corefile for MacOS 
too. And the corefile sizes for the same program on MacOS are 
way larger (of the order of a few GB as against about 300 MB on 
Linux (without the change)).


The advantage of fixing this by modifying the coredump_filter 
v/s doing it in SA (by reading in more sections of the shared 
archive file) is that this would benefit other debuggers like 
gdb also. (And reduces the dependence on having the shared 
archive file being available at the time of debugging). If folks 
still think this is a cause for concern, I could make 
modifications to fix this by reading in the regions from the 
shared archive file in the SA code. I also wonder if it is worth 
taking a relook at the mapping types of the various CDS regions 
also.


Thank you,
Jini.

On 10/22/2018 10:27 AM, 

RE: optimize KlassInfoTable size to power of 2

[臧琳]

Our preliminary measurement show the patch introduced about ~5% speed up
for jmap �Chisto when iterate ~13GB committed heap. ( with �CXX:+UseG1GC 
�CXmx180g configured)

Just FYI.

Cheers,
Lin

From: 臧琳
Sent: Monday, December 10, 2018 2:24 PM
To: serviceability-dev@openjdk.java.net
Subject: optimize KlassInfoTable size to power of 2

Dear All,
 I am investigating JMap utility for large heap (~200GB) and found that
the current KlassInfoTable’s _num_buckets size(20011) may not be optimal.
 My observation is that when “jmap histo” iterating objects , the object’s 
klass
   is identified and then hash idx in KlassInfoTable’s buckets[] is calculated 
by mod
of _num_bucktes, which would issue a heavy instruction idiv on x86 platform. It
means for every object scanned, a idiv instruction is issued, which lag the 
performance espically when
there are large number of objects in heap. Hence if the _num_buckets can be 
changed to
a pow of 2, (e.g. 65536) the idiv can be replaced with a faster instruction 
such as shl (left bit shift),
And I have prepared a patch for this change.
  My question is that why 20011 is used now? is there any special reason? And 
is there
any potential problem if I change the value to 65536, or 32768?
  Thanks!

BRs,
Lin