Re: code review request for Full Debug Symbols Revamp (7102323, 7136506)

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

Dan,

I've reviewed this:
  
http://cr.openjdk.java.net/~dcubed/fds_revamp/7102323-webrev/1-hotspot-full/





Wow, you fixed two existing bugs in the make file:

*make/solaris/makefiles/dtrace.make

*

-[ -f $(XLIBJVM_DB_G_DEBUGINFO) ] || { ln -s $(LIBJVM_DB_DEBUGINFO) 
$(XLIBJVM_DB_G_DEBUGINFO); }
+[ -f $(XLIBJVM_DB_G_DEBUGINFO) ] || { ln -s $(XLIBJVM_DB_DEBUGINFO) 
$(XLIBJVM_DB_G_DEBUGINFO); }

-[ -f $(XLIBJVM_DTRACE_G_DEBUGINFO) ] || { ln -s 
$(LIBJVM_DTRACE_DEBUGINFO) $(XLIBJVM_DTRACE_G_DEBUGINFO); }
+[ -f $(XLIBJVM_DTRACE_G_DEBUGINFO) ] || { ln -s 
$(XLIBJVM_DTRACE_DEBUGINFO) $(XLIBJVM_DTRACE_G_DEBUGINFO); }



Wrong indent:

*make/solaris/makefiles/defs.make*

 221   ifeq ($(ZIP_DEBUGINFO_FILES),1)
 222   EXPORT_LIST += $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.diz
 223   else


Question: Just curious why the $(ALT_STRIP_POLICY)  is decommissioned?

*make/linux/makefiles/defs.make*
*  make/solaris/makefiles/defs.make*

-DEF_STRIP_POLICY="min_strip"
-ifeq ($(ALT_STRIP_POLICY),)
-  STRIP_POLICY=$(DEF_STRIP_POLICY)
-else
-  STRIP_POLICY=$(ALT_STRIP_POLICY)
-endif
+# Currently, STRIP_POLICY is only used when Full Debug Symbols is enabled.
+#
+STRIP_POLICY ?= min_strip


The fix is good in general.

Thanks,
Serguei



On 3/16/12 12:58 PM, Daniel D. Daugherty wrote:

Greetings,

I need code reviews for some Makefile and packaging changes.
Wait, come back! They're not that scary...

These are Full Debug Symbols changes... so maybe they are that scary...

These changes have gone through two rounds of internal review.

The following bugs are being used to revamp the OpenJDK side of the
Full Debug Symbols (FDS) implementation:

7102323 4/4 RFE: enable Full Debug Symbols Phase 1 on Solaris
7136506 3/4 FDS: rework jdk repo Full Debug Symbols support

FDS Revamp Summary

The build infrastructure that supports the Full Debug Symbols (FDS)
project is being revamped to reduce the default on-disk footprint
along with other improvements. FDS info will have to be unzip'ed
before being usable in the default build config, but the zip'ed FDS
info occupies about 25% of the disk space as the original FDS info.

Change summary for the group of fixes:
- ENABLE_FULL_DEBUG_SYMBOLS build flag controls the Full Debug
  Symbols feature; enabled by default (ENABLE_FULL_DEBUG_SYMBOLS=1)
- ZIP_DEBUGINFO_FILES build flag controls the zip'ing of "debug info"
  during the build; enabled by default (ZIP_DEBUGINFO_FILES=1).
- FDS is enabled by default for Linux X86/X64, Solaris 
SPARC/SPARC-V9,

  Solaris X86, and Windows X86/X64.
- HSX developer builds will put debug info into .diz files that are
  co-located with the built object, e.g., there will be a libjvm.diz
  file right next to libjvm.so.
- HSX JPRT jobs will also contain .diz files co-located with the 
built

  objects
- RE promoted bits will include new debuginfo.zip bundles that 
contain

  all the .debuginfo, .diz, .map and/or .pdb files generated by the
  various repos that make up the RE promotion.

Notes: FDS is not enabled on Solaris X64 due to a bug in gobjcopy.
   FDS has not yet been implemented on MacOS X.

Just like the original FDS changes, the FDS Revamp changes are in
multiple repos:

'hotspot' repo change summary:

- add support for exporting .diz (Debug Info Zip) files
- add support for ENABLE_FULL_DEBUG_SYMBOLS build flag
  (replaces overloaded uses of OBJCOPY variable)
- add support for ZIP_DEBUGINFO_FILES build flag
- clean up STRIP_POLICY on Linux and Solaris
- On Solaris, also fixes an incorrect 64-bit libjvm_db_g symlink
  and an incorrect 64-bit libjvm_dtrace_g symlink
- The Full Debug Symbols feature is now controllable via
  ENABLE_FULL_DEBUG_SYMBOLS and ZIP_DEBUGINFO_FILES on Windows.
- On Windows, fixed a few hardcoded "sawindbg" uses

'hotspot' repo webrev:

http://cr.openjdk.java.net/~dcubed/fds_revamp/7102323-webrev/1-hotspot-full/


The HotSpot changes are relative to the HSX-24-B03 snapshot plus
one additional fix and are targeted at JDK8-B33/HSX-24-B06.


'jdk' repo change summary:

- add support for importing .diz (Debug Info Zip) files
- add support for ENABLE_FULL_DEBUG_SYMBOLS build flag
- add support for ZIP_DEBUGINFO_FILES build flag
- clean up STRIP_POLICY on Linux and Solaris
- LIBRARY_SUPPORTS_FULL_DEBUG_SYMBOLS is only needed in
  FDS Phase 2 so just a comment for now
- JPRT needs to use the '-y' option with zip on non-Windows
  builds of the jdk repo in order to preserve symbolic links

'jdk' repo webrev:

http://cr.openjdk.java.net/~dcubed/fds_revamp/7136506-webrev/1-jdk-full/


The JDK changes are relative to the T&L snapshot for JDK8-B30
and are targeted at JDK8-B33.


'root' repo change summary:

- JPRT needs to use the '-y' option with zip on non-Windows
  control bu

Re: code review request for Full Debug Symbols Revamp (7102323, 7136506)

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

Ok, thanks!

Thumb up.

Thanks,
Serguei

On 3/17/12 6:29 PM, Daniel D. Daugherty wrote:

Thanks for the review. Replies embedded below...



Dan,

I've reviewed this:
http://cr.openjdk.java.net/~dcubed/fds_revamp/7102323-webrev/1-hotspot-full/




Wow, you fixed two existing bugs in the make file:

*make/solaris/makefiles/dtrace.make

*
-[ -f $(XLIBJVM_DB_G_DEBUGINFO) ] || { ln -s $(LIBJVM_DB_DEBUGINFO) 
$(XLIBJVM_DB_G_DEBUGINFO); }
+[ -f $(XLIBJVM_DB_G_DEBUGINFO) ] || { ln -s $(XLIBJVM_DB_DEBUGINFO) 
$(XLIBJVM_DB_G_DEBUGINFO); }
-[ -f $(XLIBJVM_DTRACE_G_DEBUGINFO) ] || { ln -s 
$(LIBJVM_DTRACE_DEBUGINFO) $(XLIBJVM_DTRACE_G_DEBUGINFO); }
+[ -f $(XLIBJVM_DTRACE_G_DEBUGINFO) ] || { ln -s 
$(XLIBJVM_DTRACE_DEBUGINFO) $(XLIBJVM_DTRACE_G_DEBUGINFO); }


Yup! I called that out in the 'hotspot' repo change summary:


- On Solaris, also fixes an incorrect 64-bit libjvm_db_g symlink
  and an incorrect 64-bit libjvm_dtrace_g symlink 


However, right after these changes go in, I'll be removing all
the '_g' support via:

7153050 4/4 remove crufty '_g' support from HotSpot repo




Wrong indent:

*make/solaris/makefiles/defs.make*
  221   ifeq ($(ZIP_DEBUGINFO_FILES),1)
  222   EXPORT_LIST += $(EXPORT_JRE_LIB_ARCH_DIR)/libsaproc.diz
  223   else


Nicely spotted! I'll fix that.




Question: Just curious why the $(ALT_STRIP_POLICY)  is decommissioned?

*make/linux/makefiles/defs.make*
*  make/solaris/makefiles/defs.make*
-DEF_STRIP_POLICY="min_strip"
-ifeq ($(ALT_STRIP_POLICY),)
-  STRIP_POLICY=$(DEF_STRIP_POLICY)
-else
-  STRIP_POLICY=$(ALT_STRIP_POLICY)
-endif
+# Currently, STRIP_POLICY is only used when Full Debug Symbols is enabled.
+#
+STRIP_POLICY ?= min_strip


I figured out a different way to do it. If you invoke like so:

gnumake STRIP_POLICY=no_strip ...

then that works. Also, when I looked around ALT_* variables tend
to be used with paths.




The fix is good in general.


Thanks!

Dan




Thanks,
Serguei



On 3/16/12 12:58 PM, Daniel D. Daugherty wrote:

Greetings,

I need code reviews for some Makefile and packaging changes.
Wait, come back! They're not that scary...

These are Full Debug Symbols changes... so maybe they are that scary...

These changes have gone through two rounds of internal review.

The following bugs are being used to revamp the OpenJDK side of the
Full Debug Symbols (FDS) implementation:

7102323 4/4 RFE: enable Full Debug Symbols Phase 1 on Solaris
7136506 3/4 FDS: rework jdk repo Full Debug Symbols support

FDS Revamp Summary

The build infrastructure that supports the Full Debug Symbols (FDS)
project is being revamped to reduce the default on-disk footprint
along with other improvements. FDS info will have to be unzip'ed
before being usable in the default build config, but the zip'ed FDS
info occupies about 25% of the disk space as the original FDS info.

Change summary for the group of fixes:
- ENABLE_FULL_DEBUG_SYMBOLS build flag controls the Full Debug
  Symbols feature; enabled by default (ENABLE_FULL_DEBUG_SYMBOLS=1)
- ZIP_DEBUGINFO_FILES build flag controls the zip'ing of "debug 
info"

  during the build; enabled by default (ZIP_DEBUGINFO_FILES=1).
- FDS is enabled by default for Linux X86/X64, Solaris 
SPARC/SPARC-V9,

  Solaris X86, and Windows X86/X64.
- HSX developer builds will put debug info into .diz files that are
  co-located with the built object, e.g., there will be a 
libjvm.diz

  file right next to libjvm.so.
- HSX JPRT jobs will also contain .diz files co-located with the 
built

  objects
- RE promoted bits will include new debuginfo.zip bundles that 
contain

  all the .debuginfo, .diz, .map and/or .pdb files generated by the
  various repos that make up the RE promotion.

Notes: FDS is not enabled on Solaris X64 due to a bug in gobjcopy.
   FDS has not yet been implemented on MacOS X.

Just like the original FDS changes, the FDS Revamp changes are in
multiple repos:

'hotspot' repo change summary:

- add support for exporting .diz (Debug Info Zip) files
- add support for ENABLE_FULL_DEBUG_SYMBOLS build flag
  (replaces overloaded uses of OBJCOPY variable)
- add support for ZIP_DEBUGINFO_FILES build flag
- clean up STRIP_POLICY on Linux and Solaris
- On Solaris, also fixes an incorrect 64-bit libjvm_db_g symlink
  and an incorrect 64-bit libjvm_dtrace_g symlink
- The Full Debug Symbols feature is now controllable via
  ENABLE_FULL_DEBUG_SYMBOLS and ZIP_DEBUGINFO_FILES on Windows.
- On Windows, fixed a few hardcoded "sawindbg" uses

'hotspot' repo webrev:
http://cr.openjdk.java.net/~dcubed/fds_revamp/7102323-webrev/1-hotspot-full/

The HotSpot changes are relative to the HSX-24-B03 snapshot plus
one additional fix and are targeted at JDK8-B33/HSX-24-B06.


'jdk' repo change summary:

- add support for importing .diz (Debug 

Re: code review request for minor FDS tweak (7157296, 7158067)

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

Dan,

I've reviewed all 3 webrevs.
They look fine.

Thanks,
Serguei



On 3/30/12 10:32 AM, Daniel D. Daugherty wrote:

Greetings,

In my recent Full Debug Symbols changes, I added the new
ENABLE_FULL_DEBUG_SYMBOLS build flag. I originally implemented
this flag to disable debug info for all build configs which
doesn't make a whole lot of sense for non-product builds. After all
what's a debug build without debug info? (Pretty much the same
thing as a product build).

As is usual, I have one bug to track the hotspot repo changes and
another bug to track the other repos:

7157296 3/4 FDS: ENABLE_FULL_DEBUG_SYMBOLS flag should only affect
OPT builds
7158067 4/4 FDS: ENABLE_FULL_DEBUG_SYMBOLS flag should only affect
product builds

Here are the webrev URLs:

http://cr.openjdk.java.net/~dcubed/fds_revamp/7157296-webrev/0-jdk8-root/
http://cr.openjdk.java.net/~dcubed/fds_revamp/7158067-webrev/0-jdk8-hotspot/ 


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

Thanks, in advance, for any review comments.

Dan

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

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

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: code review request for initial JDK FDS support (7071907)

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

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

Dan,

Looks good for me.


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?



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.

My understanding is the 'set -e' (line 194) should cause the build to fail.

Thanks,
Serguei



-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: code review request for initial JDK FDS support (7071907)

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

Looks good.

Thanks,
Serguei

On 4/12/12 2:03 PM, Daniel D. Daugherty wrote:

The wonderful T&L nightly testing caught an error in my changes
that were reviewed on this thread. On Linux and Solaris, I installed
the .debuginfo files for programs in $JAVA_HOME/bin. I made a
mistake in the way I interpreted what I was seeing in the Windows
code and in $JAVA_HOME/jre/bin. Long story shorter... we shouldn't
install .debuginfo files for programs.

7160895 3/3 tools/launcher/VersionCheck.java attempts to launch 
.debuginfo

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7160895

The fix is a simple tweak to changes reviewed on this thread:

diff -r d922195b678d make/common/Program.gmk
--- a/make/common/Program.gmk   Wed Apr 11 07:26:35 2012 -0700
+++ b/make/common/Program.gmk   Thu Apr 12 13:38:36 2012 -0700
@@ -268,6 +268,11 @@ else
   $(ZIPEXE) -q $(@F).diz $(@F).debuginfo ; \
   $(RM) $(@F).debuginfo ; \
  )
+  # save ZIP'ed debug info with rest of the program's build 
artifacts

+  $(MV) $@.diz $(OBJDIR)
+else
+  # save debug info with rest of the program's build artifacts
+  $(MV) $@.debuginfo $(OBJDIR)
 endif
   endif # PROGRAM_SUPPORTS_FULL_DEBUG_SYMBOLS
 endif # ENABLE_FULL_DEBUG_SYMBOLS

After generation of the program's .debuginfo file, we move it
to the build directory that has the rest of build artifacts,
i.e., .o files etc. We generate the .debuginfo file in the bin
directory because that is where the link phase puts the program
binary in a Linux and/or Solaris build.

On Windows, the program, the .map and the .pdb files are generated
in the build directory and only the program (.exe) is copied to
the bin directory. So in a Windows build, the program binary is
in the build directory and the bin directory and on Linux and
Solaris the program binary is only in the bin directory.

Dan


On 4/9/12 2: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: code review for JDK FDS gobjcopy work arounds (7170449)

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

Dan,

I've reviewed the fixes as did not notice any problems.
Looks good.

Thanks,
Serguei

On 5/23/12 12:50 PM, Daniel D. Daugherty wrote:

Greetings,

This is a JDK code review request for a pair of Full Debug Symbols
gobjcopy work arounds on Solaris. The gobjcopy utility on Solaris 10
corrupts the SUNW_* sections on objects. This has caused dtrace test
failures and Monitoring & Management test failures. The gobjcopy
utility crashes due to empty sections with the SHF_ALLOC flagset on
Solaris X64 objects. This causes build failures.

There are two new temporary work around tools:

add_gnu_debuglink - adds the ".gnu_debuglink" section to an ELF
object without corrupting the other sections in the object.

fix_empty_sec_hdr_flags - removes the SHF_ALLOC flag from empty
sections in ELF objects.

These temporary work arounds are only needed until the proper
Solaris 10 Update 6 patches are made available. The two patches
are independent of one another which is why there are two
separate temporary work arounds. However, we're putting the
temporary work arounds in place because the 7u6 project window
is closing fast.

Here is the webrev URL for the JDK8-T&L version:

http://cr.openjdk.java.net/~dcubed/fds_revamp/7170449-webrev/0/

This fix will also be backported to 7u6-T&L and I expect the
changes to virtually identical.

Thanks, in advance, for any reviews!

Dan

Re: code review for second hotspot FDS gobjcopy work around (7165598)

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

I do not see any issues with that webrev.
Looks good.

Thanks,
Serguei

On 5/23/12 12:59 PM, Daniel D. Daugherty wrote:

Greetings,

This is a hotspot code review request for the second of a pair of
Full Debug Symbols gobjcopy work arounds on Solaris. The first
hotspot FDS gobjcopy work around was reviewed using bug 7165060
and that fixed the dtrace test failures.

The gobjcopy utility also crashes due to empty sections with the
SHF_ALLOC flagset on Solaris X64 objects. This causes build
failures.

The first new temporary work around tool is add_gnu_debuglink
and it was added by 7165060.The second new temporary work around
tool is:

fix_empty_sec_hdr_flags - removes the SHF_ALLOC flag from empty
sections in ELF objects.

These temporary work arounds are only needed until the proper
Solaris 10 Update 6 patches are made available. The two patches
are independent of one another which is why there are two
separate temporary work arounds. However, we're putting the
temporary work arounds in place because the 7u6/HSX-23.2 project
window is closing fast.

Here is the webrev URL for the HSX-24 version:

http://cr.openjdk.java.net/~dcubed/fds_revamp/7165598-webrev/0/

This fix will also be backported to 7u6/HSX-23.2 and I expect the
changes to virtually identical.

Thanks, in advance, for any reviews!

Dan

Re: code review request for 7153050 remove crufty '_g' support

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

Dan,

It is nice fix and simplified many places.

Just one minor comment:

make/bsd/makefiles/dtrace.make

  41 #LIBJVM_DB = libjvm_db.dylib
  42 LIBJVM_DB = libjvm_db.dylib

  45 #LIBJVM_DTRACE = libjvm_dtrace.dylib
  46 LIBJVM_DTRACE = libjvm_dtrace.dylib

   The lines #41 and #45 can be removed.


Thanks,
Serguei


On 12/13/12 10:53 AM, Daniel D. Daugherty wrote:

Greetings,

I'm sponsoring this code review request from Ron Durbin. This change
is targeted at JDK8/HSX-25 in the RT_Baseline repo. Please make sure
you include Ron on any e-mail replies since he is not yet on the
OpenJDK aliases.

Dan


Intro:

This set of changes removes the makefile support for generation of debug
versions that follow _g semantics.

Defect:

7153050 “remove crufty '_g' support from HotSpot repo”
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153050
https://jbs.oracle.com/bugs/browse/JDK-7153050


Webrev

http://cr.openjdk.java.net/~dcubed/for_rdurbin/7153050-webrev/0

Details:

Many makefiles have been modified to remove all reference and support
for debug versions that follow _g semantics.

Re: RFR: JDK-8189607 Remove duplicated jvmticmlr.h

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

Hi Magnus,

The fix looks good to me.
Thank you for doing this cleanup.

Thanks,
Serguei


On 10/18/17 01:04, Magnus Ihse Bursie wrote:
The file jvmticmlr.h is stored twice in the repo, both in hotspot and 
in java.base. They are both identical, and only the java.base version 
is included in the final product. This might arguably have been useful 
in a pre-consolidated world, but makes absolutely no sense now.


Bug: https://bugs.openjdk.java.net/browse/JDK-8189607
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8189607-remove-duplicated-jvmticmlr/webrev.01


/Magnus

Re: RFR(S): 8196992: Resolve disabled warnings for libdt_socket

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

Chris,

This looks good to me too.

Thanks,
Serguei


On 2/23/18 00:48, Langer, Christoph wrote:

Looks good, Chris.


-Original Message-
From: build-dev [mailto:build-dev-boun...@openjdk.java.net] On Behalf Of
Chris Plummer
Sent: Freitag, 23. Februar 2018 02:04
To: build-dev@openjdk.java.net build-dev ;
serviceability-dev 
Subject: RFR(S): 8196992: Resolve disabled warnings for libdt_socket

Hello,

Please review the following:

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

diff --git a/make/lib/Lib-jdk.jdwp.agent.gmk
b/make/lib/Lib-jdk.jdwp.agent.gmk
--- a/make/lib/Lib-jdk.jdwp.agent.gmk
+++ b/make/lib/Lib-jdk.jdwp.agent.gmk
@@ -43,7 +43,6 @@
   OPTIMIZATION := LOW, \
   CFLAGS := $(CFLAGS_JDKLIB) -DUSE_MMAP \
   $(LIBDT_SOCKET_CPPFLAGS), \
-    DISABLED_WARNINGS_gcc := shift-negative-value, \
   MAPFILE := $(TOPDIR)/make/mapfiles/libdt_socket/mapfile-vers, \
   LDFLAGS := $(LDFLAGS_JDKLIB) \
   $(call SET_SHARED_LIBRARY_ORIGIN), \

This change is undoing the makefile change done as part of JDK-8196985.
The only warning that was turning up in libdt_socket code before
JDK-8196985 was done has already been fixed by JDK-8196909. Thus no
warnings need to be fixed.

After removing the above makefile code, I tested by building with the
new toolchain. As a first test I undid the socketTransport.cpp fix from
JDK-8196909 to verify that the new toolchain exposed the warning. Then I
reverted socketTransport.cpp back to tip sources and saw no warnings
with the new toolchain.

thanks,

Chris

Re: RFR: JDK-8199782: Fix compilation warnings detected by Solaris Developer Studio 12.6

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

Hi Gary,

It looks reasonable.
I'm not very familiar with the concrete SolStudio versions.

Thanks,
Serguei


On 4/4/18 11:18, Gary Adams wrote:
Getting the sources ready for the next Solaris developer studio 
toolchain.


  Issue: https://bugs.openjdk.java.net/browse/JDK-8199782
  Webrev: http://cr.openjdk.java.net/~gadams/8199782/webrev.00/

This update conditionally disables some new error checks, if the
new toolchain is used.

Re: RFR(L) : 8199375 : [TESTBUG] Open source vm testbase monitoring tests

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

Hi Igor,

It looks good.

Thanks,
Serguei


On 5/1/18 19:10, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8199375/webrev.00/index.html

41276 lines changed: 41274 ins; 1 del; 1 mod;

Hi all,

could you please review the patch which open sources monitoring tests from vm 
testbase?

The tests were developed to test hotspot related JMX functionality. as w/ 
common VM testbase code, these tests are old, they have been run from hotspot 
testing for a long period of time. originally, these tests were run by a test 
harness different from jtreg and had different build and execution schemes, 
some parts couldn't be easily translated to jtreg, so tests might have actions 
or pieces of code which look weird. in a long term, we are planning to rework 
them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199375
webrev:  http://cr.openjdk.java.net/~iignatyev/8199375/webrev.00/index.html
testing: vmTestbase_nsk_monitoring test group

Thanks,
-- Igor

Re: 8199271: [TESTBUG] open source VM testbase stress tests

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

Hi Leonid,

Looks good to me too.

Thanks,
Serguei


On 5/14/18 14:04, Leonid Mesnik wrote:

Misha

Thank you for review. I still need one more review from 'R'eviewer.

Leonid


On May 11, 2018, at 9:10 AM, Mikhailo Seledtsov  
wrote:

Looks good to me,

Misha

On 5/8/18, 2:23 PM, Leonid Mesnik wrote:

Hi

Please review this change open sourcing vm testbase stress tests. These tests 
have been developed a long time ago for internal test harness and don't looks 
very nice now.
They are open sourced with minimal changes only. The long term plan is to 
review and improve them moving out of vmTestbase directory in corresponding 
components.

The fix introduce vmTestbase_nsk_stress test group and have make files changes 
required to build native part of tests.

I've verified that "make -- run-test TEST=:vmTestbase_nsk_stress" pass on my 
linux locally.

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

Re: RFR(XL) : 8199383 : [TESTBUG] Open source VM testbase JVMTI tests

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

Hi Igor,

It looks good to me.

Thanks,
Serguei


On 5/22/18 16:35, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8199383/webrev.00/index.html

308253 lines changed: 308253 ins; 0 del; 0 mod;

Hi all,

could you please review this patch which open sources JVMTI tests from VM 
testbase?

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

webrev: http://cr.openjdk.java.net/~iignatyev//8199383/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8199383
testing: :vmTestbase_nsk_jvmti test group

Thanks,
-- Igor

Re: RFR(M) : 8199380 : [TESTBUG] Open source VM testbase AOD tests

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

Igor,

It looks good.
Thank you a lot for taking care about this!

Thanks,
Serguei


On 5/23/18 12:44, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev/8199380/webrev.00/

2370 lines changed: 2370 ins; 0 del; 0 mod;

Hi all,

could you please review this patch which open sources AOD tests from VM 
testbase? these tests test hotspot's attach-on-demand.

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199380
webrev: http://cr.openjdk.java.net/~iignatyev//8199380/webrev.00/index.html
testing: :vmTestbase_vm_aod test group

Thanks,
-- Igor

Re: RFR: JDK-8223319: Add copyright footer to specs and man pages

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

Hi Erik,

Could you, please, show a simple diff for jvmti.html and jdwp-protocol.html?

Thanks,
Serguei


On 5/3/19 09:37, Erik Joelsson wrote:
The (optional) specs and man pages should have the same copyright 
footer as the generated API docs. This patch adds the logic to add 
such footers. It also removes the existing footer in jvmti.html.


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

Webrev: http://cr.openjdk.java.net/~erikj/8223319/webrev.01/index.html

/Erik

Re: RFR: JDK-8242629 Remove references to deprecated java.util.Observer and Observable

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

Hi Magnus,

This looks okay to me unless there is a better solution.

Thanks,
Serguei

On 4/14/20 04:04, Magnus Ihse Bursie wrote:
As a first step towards fixing deprecation warnings in SA, all the 
references (200+) to the deprecated java.util.Observer and Observable 
needs to be fixed, otherwise all other changes will drown in this one.


This solution is the result of the preceding discussions in 
serviceability-dev. That means that most of the change consists of 
adding an explicit import like this:


import sun.jvm.hotspot.utilities.Observable;
import sun.jvm.hotspot.utilities.Observer;

to override the general java.util.* import that was already present in 
all (or almost all) files, and make 
sun.jvm.hotspot.utilities.Observable and Observer extend the java.util 
versions but with deprecation warnings disabled.


It turned out however, that this simplest approach did not work fully. 
Since the interface java.util.Observer had the single method "void 
update(java.util.Observable o, Object arg)" it did not help to create 
a new interface sun.jvm.hotspot.utilities.Observer that extended 
java.util.Observer. I did not observe this issue in my PoC webrev that 
I posted during the discussion. :-(


Instead, for Observer, I had just created a new interface with the 
same method, but that uses sun.jvm.hotspot.utilities.Observable 
instead of java.util.Observable.


The end effect is the same -- the only change needed to most files is 
an added import, we get rid of the deprecation warnings, and we did 
not have to copy any significant amount of code from java.util.


I now hope this is acceptable by all.

Bug: https://bugs.openjdk.java.net/browse/JDK-8242629
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242629-fix-SA-Observer/webrev.01


(When reading the patch, I recommend looking at the patch file 
http://cr.openjdk.java.net/~ihse/JDK-8242629-fix-SA-Observer/webrev.01/open.patch 
instead of individually checking the files in the webrev.)


/Magnus

Re: RFR: JDK-8242808 Fix all remaining deprecation warnings in jdk.hotspot.agent

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

Hi Magnus,

It looks good to me.

Thanks,
Serguei


On 4/15/20 06:00, Magnus Ihse Bursie wrote:
Here is an updated version, that avoids the SuppressWarnings for 
modelToView:


http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.02 



(Only change is in SourceCodePanel.java compared to v01)

/Magnus

On 2020-04-15 14:30, Magnus Ihse Bursie wrote:



On 2020-04-15 12:59, Prasanta Sadhukhan wrote:

Hi Magnus,

Why can't we just use modelToView2D() to get Rectangle2D and then 
cast to Rectangle tobe used in scrollRectToVisible() or else use 
(int)Rectangle2D.getX(), (int)getY(), getWidth(), getHeight() to 
construct a new Rectangle()?
Casting a Rectangle2D to a Rectangle does not seem to me to be an 
improvement. That presupposes even more knowledge about 
implementation details.


However, I'll look into constructing a Rectangle using the getters 
from the Rectangle2D, as you suggested. Thanks!


/Magnus


Regard
Prasanta
On 15-Apr-20 3:35 PM, Magnus Ihse Bursie wrote:

Hi swing-dev,

Do you have any other suggestions for how to resolve the 
deprecation of modelToView() in this code?


Basically, the code does this:

   Rectangle rect = source.modelToView(offset);
   source.scrollRectToVisible(rect);

but scrollRectToVisible() requires a Rectangle (a subtype of 
Rectangle2D), and modelToView() is deprecated with reference to 
modelToView2D(), which returns a Rectangle2D, which is thus 
unusable by scrollRectToVisible(). I also cannot find a way to 
create a new Rectangle, given a Rectangle2D.


In practice, under the hood, it's still just Rectangles (not 
Rectangle2D).


/Magnus

On 2020-04-15 11:37, David Holmes wrote:

Hi Magnus,

This one sounds like it needs a Swing/Java2D developer to review 
it :)


Cheers,
David

On 15/04/2020 7:13 pm, Magnus Ihse Bursie wrote:
After JDK-8242804, a few places remain which are using deprecated 
methods. They too should be fixed, and the deprecation warning 
should no longer be disabled.


This patch presupposes the fix for JDK-8242804 has been applied 
(otherwise we cannot turn the deprecation warning back on).


Some brief comments about each fix:

* In ConstantPool.java, there was a boxing deprecation that I 
missed in JDK-8242804 (sorry!)


* In HighPrecisionJScrollBar.java, there is a trivial replacement 
to use BigDecimal.divide with RoundingMode semantics.


* In SourceCodePanel.java, I settled for suppressing the warning. 
The issue here is that modelToView (which returns a Rectangle) is 
deprecated in favor of modelToView2D, which returns a Rectangle2D 
(which is a supertype of Rectangle). But we use the result in 
scrollRectToVisible, and there exist no version of that which 
accepts a Rectangle2D instead of a Rectangle, nor a way to 
created a Rectangle from a Rectangle2D (that I could find). In 
practice, this is just a game of types -- under the hood, 
modelToView2D still returns a Rectangle (even though it only 
promises a Rectangle2D). The alternative here would be to cast 
the result of modelToView2D to a Rectangle, but I found that less 
attractive.


* In JTreeTable.java, I've replaced the use of the old-style 
modifier mask with the new-style extended modifier mask. To the 
best of my understanding, this will just work the same for the 
code here (and for the MouseEvent constructor, using the extended 
mask is actually prescribed).


Bug: https://bugs.openjdk.java.net/browse/JDK-8242808
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8242808-fix-all-SA-deprecation/webrev.01 

Re: RFR: JDK-8242943 Fix all remaining unchecked warnings in jdk.hotspot.agent

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

Hi Magnus,

This looks pretty good to me.
I hope, Joe gave some insight for Comparable's.

Thanks,
Serguei


On 4/16/20 05:47, Magnus Ihse Bursie wrote:
This is the final part of removing all warnings from the build of 
jdk.hotspot.agent. This patch includes a number of non-trivial fixes 
for the few remaining unchecked warnings.


The good news is that with this fix (and after the recent removal of 
Nashorn and rmic), the JDK build is finally complete free from 
warnings for the first time ever! EPIC!1!!! :-)


The bad news is that this patch is probably the most complex of all 
I've posted so far. :-/


I'll break down the changes in four groups:

* ConstMethod superclass issue

ConstMethod current extends VMObject. However, it is treated as a 
Metadata (Metadata is a subtype of VMObject). For instance, this code 
appears in Metadata.java:

  metadataConstructor.addMapping("ConstMethod", ConstMethod.class);

I believe it is just an oversight that ConstMethod does not also 
extend Metadata like the rest of the classes added to the 
metadataConstructor mapping, one which has not been caught without the 
extra type safety given by generics.


* ProfileData casting

In several places, a ProfileData item is extracted from a collection, 
its type checked with instanceof, and then acted on accordingly. 
(Yeah, nice and clean OOP abstraction at work! :-))


This works well most of the time, but when casting to ReceiverTypeData 
or CallTypeDataInterface, the type include generic parametrisation, so 
it needs to be cast to e.g. ReceiverTypeData. 
Unfortunately, this cannot be verified using instanceof, due to 
generic type erasure, and thus the compiler considers this an 
unchecked cast. (At least, this is my understanding of what is 
happening.) As far as I can tell, this is legitimate code, and the 
only way to really handle this (barring a more extensive rewrite) is 
to disable the warning for these cases.


* Generification of the InstanceConstructor hierarchy

I have properly generified the InstanceConstructor hierarchy, with the 
VirtualConstructor, StaticBaseConstructor and VirtualBaseConstructor 
subclasses. (And also the related helper class VMObjectFactory.)


Most of it turned out OK (and with improved type safety), but there 
was one piece of code that is a bit unfortunate. In 
VirtualBaseConstructor, we have the following:

    @SuppressWarnings("unchecked")
    Class lookedUpClass = (ClassT>)Class.forName(packageName + "." + superType.getName());

    c = lookedUpClass;

That is, we send in a name for a class and assumes that it will 
resolve to a subtype of T, but there is no way to enforce this with 
type safety. I have verified all current callers to the constructor so 
that they all abide to this rule (of course, otherwise the code would 
not have worked, but I checked it anyway). The alternative to 
suppressing the warning is to redesign the entire API, so we do not 
send in a class name, but instead a Class, and use that 
to extract the name instead. At this point, I don't find it worth it. 
This is, after all, no worse than it was before.


* SortableTableModel and TableModelComparator

This one was quite messy. The tables are being used in quite different 
ways in different places, which made it hard to generify in a good 
way. A lot of it revolves around keeping the data as Objects and 
casting it to a known type. (Some of this behavior comes from the fact 
that they extend old Swing classes which does this.) I've tried to 
tighten it up as much as possible by introducing increased type safety 
by generification, but in the end, it was not fully possible to do 
without a major surgery of the entire class structure. As above, most 
of it turned out OK, but not all.


The tricky one here is the helper TableModelComparator. This one took 
me a while to wrap my head around. It implements Comparator, but the 
compare() method takes "rows" from the model, which are just expressed 
as Objects, and left to subclasses to define differently. For one of 
the subclasses it uses the type T that the TableModel is created 
around, but in other it uses an independent domain model. :-/ Anyway. 
The compare() method then extracts data for the individual columns of 
each row using the method getValueForColumn(), and compare them 
pairwise. This data from the rows are supposed to implement Comparable.


In the end, I think I got it pretty OK, but I'm still an apprentice 
when it comes to generics black magic. The subclasses of 
TableModelComparator want to return different objects from 
getValueForColumn() for different columns in the row, like Long or 
String. They are all Comparable, but String is Comparable and 
Long is Comparable. So I needed to specify the abstract method 
as returning Comparable, since Comparable is not 
Comparable.


But then, for reasons that I do not fully fathom, I could not specify

Comparable o1 = getValueForColumn(row1, columns[i]);
Comparable o2 = getV

Re: RFR: 8076473 Remove the jhat code and update makefiles

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

It looks good.
A question:
  Do we want to get rid of the jhat in the 
jdk/closed/old-build/common/Release.gmk ?

jhat.1 \
com/sun/tools/hat   \
jhat$(EXE_SUFFIX)

Thanks,
Serguei



On 4/23/15 5:06 AM, Staffan Larsen wrote:
Please review this change to remove the jhat tool. I will not push 
this change until the tests have been removed (see a different review 
thread).


JEP: https://bugs.openjdk.java.net/browse/JDK-8059039
bug: https://bugs.openjdk.java.net/browse/JDK-8076473

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

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



Thanks,
/Staffan

Re: RFR: 8076473 Remove the jhat code and update makefiles

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

On 4/23/15 2:50 PM, Erik Joelsson wrote:
No, that file is old legacy and not used anymore. It will hopefully 
soon go away.


Ok then.

Thanks, Erik!
Serguei



/Erik

On 2015-04-23 12:52, serguei.spit...@oracle.com wrote:

It looks good.
A question:
  Do we want to get rid of the jhat in the 
jdk/closed/old-build/common/Release.gmk ?

jhat.1 \
com/sun/tools/hat   \
jhat$(EXE_SUFFIX)

Thanks,
Serguei



On 4/23/15 5:06 AM, Staffan Larsen wrote:
Please review this change to remove the jhat tool. I will not push 
this change until the tests have been removed (see a different 
review thread).


JEP: https://bugs.openjdk.java.net/browse/JDK-8059039
bug: https://bugs.openjdk.java.net/browse/JDK-8076473

webrev root: http://cr.openjdk.java.net/~sla/8076473/webrev.root.00/ 
<http://cr.openjdk.java.net/%7Esla/8076473/webrev.root.00/>
webrev jdk: http://cr.openjdk.java.net/~sla/8076473/webrev.jdk.00/ 
<http://cr.openjdk.java.net/%7Esla/8076473/webrev.jdk.00/>


Thanks,
/Staffan

Re: RFR: 8079345: After 8079248 fixed JDK still fails with "jdk\\bin\\management_ext.dll: The specified procedure could not be found"

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

This looks good.
It is better with the moved comment.

Thanks,
Serguei

On 5/6/15 4:29 AM, Staffan Larsen wrote:

On 6 maj 2015, at 11:46, Magnus Ihse Bursie  
wrote:

On 2015-05-06 11:39, Erik Joelsson wrote:

This one looks better. Sorry for not spotting the problem in the previous 
review.

/Erik

On 2015-05-06 11:24, Staffan Larsen wrote:

My fix for 8079248 was broken, so here is a new attempt. I intend to push this 
directly to jdk9/hs since that is where 8079248 was pushed.

bug: https://bugs.openjdk.java.net/browse/JDK-8079345#comment-13638237
webrev: http://cr.openjdk.java.net/~sla/8079345/webrev.00 


Looks good to me. If you care, maybe you could move (and properly indent) the comment 
about the flag to inside the "if windows" clause? You don't need to respin the 
webrev if you do that.

Done:

diff --git a/make/lib/Lib-jdk.management.gmk b/make/lib/Lib-jdk.management.gmk
--- a/make/lib/Lib-jdk.management.gmk
+++ b/make/lib/Lib-jdk.management.gmk
@@ -39,10 +39,12 @@
  $(LIBJAVA_HEADER_FLAGS) \
  #

-# In (at least) VS2013 and later, -DPSAPI_VERSION=1 is needed to generate
-# a binary that is compatible with windows versions older than 7/2008R2.
-# See MSDN documentation for GetProcessMemoryInfo for more information.
-BUILD_LIBMANAGEMENT_EXT_CFLAGS += -DPSAPI_VERSION=1
+ifeq ($(OPENJDK_TARGET_OS), windows)
+  # In (at least) VS2013 and later, -DPSAPI_VERSION=1 is needed to generate
+  # a binary that is compatible with windows versions older than 7/2008R2.
+  # See MSDN documentation for GetProcessMemoryInfo for more information.
+  LIBMANAGEMENT_EXT_CFLAGS += -DPSAPI_VERSION=1
+endif

  LIBMANAGEMENT_EXT_OPTIMIZATION := HIGH
  ifneq ($(findstring $(OPENJDK_TARGET_OS), solaris linux), )

Re: RFR: 8079360 AttachProviderImpl could not be instantiated

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

Looks good.

Thanks,
Serguei

On 5/6/15 11:51 AM, Staffan Larsen wrote:
This is another library that needs to be compiled with 
-DPSAPI_VERSION=1 due to the recent Windows compiler upgrade.


I have also fixed a better error message that prints the underlaying 
exception if something like this happens again.


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


bug: https://bugs.openjdk.java.net/browse/JDK-8079360

Thanks,
/Staffan

Re: RFR: 8076470 - JEP 240: Remove the JVM TI hprof Agent

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

Hi Staffan,

Looks good.

I'm re-posting the same question in this review:
  Q1: Should the folder jdk/src/demo/share/jvmti/java_crw_demo also be 
removed?


Thanks,
Serguei


On 8/6/15 11:57 PM, Staffan Larsen wrote:
Please review the following changes to remove the hprof JVMTI agent. 
There are changes in three different repositories. All tests that used 
the hprof agent has been removed in previous changesets.


Note: This does not remove the ability of the Hotspot VM to output 
heap dumps in the hprof format.


bug: https://bugs.openjdk.java.net/browse/JDK-8046661

top-level changes: 
http://cr.openjdk.java.net/~sla/8076470/root/webrev.00/ 

jdk changes: http://cr.openjdk.java.net/~sla/8076470/jdk/webrev.00/ 

hotspot changes: 
http://cr.openjdk.java.net/~sla/8076470/hotspot/webrev.00/ 



Thanks,
/Staffan

Re: RFR: 8076470 - JEP 240: Remove the JVM TI hprof Agent

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

Dmitry,


On 8/7/15 2:31 AM, Dmitry Samersoff wrote:

Serguei,

   Q1: Should the folder jdk/src/demo/share/jvmti/java_crw_demo also be
removed?

Yes. See

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


Thank you for the comment.
But, please, see also the related comment from Staffan.

Thanks,
Serguei



-Dmitry

On 2015-08-07 12:24, serguei.spit...@oracle.com wrote:

Hi Staffan,

Looks good.

I'm re-posting the same question in this review:
   Q1: Should the folder jdk/src/demo/share/jvmti/java_crw_demo also be
removed?

Thanks,
Serguei


On 8/6/15 11:57 PM, Staffan Larsen wrote:

Please review the following changes to remove the hprof JVMTI agent.
There are changes in three different repositories. All tests that
used the hprof agent has been removed in previous changesets.

Note: This does not remove the ability of the Hotspot VM to output
heap dumps in the hprof format.

bug: https://bugs.openjdk.java.net/browse/JDK-8046661

top-level changes:
http://cr.openjdk.java.net/~sla/8076470/root/webrev.00/
<http://cr.openjdk.java.net/%7Esla/8076470/root/webrev.00/>
jdk changes: http://cr.openjdk.java.net/~sla/8076470/jdk/webrev.00/
<http://cr.openjdk.java.net/%7Esla/8076470/jdk/webrev.00/>
hotspot changes:
http://cr.openjdk.java.net/~sla/8076470/hotspot/webrev.00/
<http://cr.openjdk.java.net/%7Esla/8076470/hotspot/webrev.00/>

Thanks,
/Staffan

Re: RFR: 8076470 - JEP 240: Remove the JVM TI hprof Agent

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

Ok.

Thanks, Staffan!
Serguei


On 8/7/15 2:27 AM, Staffan Larsen wrote:


On 7 aug 2015, at 11:24, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:


Hi Staffan,

Looks good.


Thanks!



I'm re-posting the same question in this review:
  Q1: Should the folder jdk/src/demo/share/jvmti/java_crw_demo also 
be removed?


No, it is used by some of the demos (mtrace, minst, etc). But they 
statically link the code so the lib is no longer needed.


/Staffan



Thanks,
Serguei


On 8/6/15 11:57 PM, Staffan Larsen wrote:
Please review the following changes to remove the hprof JVMTI agent. 
There are changes in three different repositories. All tests that 
used the hprof agent has been removed in previous changesets.


Note: This does not remove the ability of the Hotspot VM to output 
heap dumps in the hprof format.


bug: https://bugs.openjdk.java.net/browse/JDK-8046661

top-level changes: 
http://cr.openjdk.java.net/~sla/8076470/root/webrev.00/ 
<http://cr.openjdk.java.net/%7Esla/8076470/root/webrev.00/>
jdk changes: http://cr.openjdk.java.net/~sla/8076470/jdk/webrev.00/ 
<http://cr.openjdk.java.net/%7Esla/8076470/jdk/webrev.00/>
hotspot changes: 
http://cr.openjdk.java.net/~sla/8076470/hotspot/webrev.00/ 
<http://cr.openjdk.java.net/%7Esla/8076470/hotspot/webrev.00/>


Thanks,
/Staffan

Re: RFR: JDK-8063154: Checked in jvmti.h not in sync with generated jvmti.h

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

Hi Erik

+1 to what Tim said.
Thank you a lot for fixing this issue!

Thanks,
Serguei


On 11/1/16 14:38, Tim Bell wrote:

Erik:


New webrev: http://cr.openjdk.java.net/~erikj/8063154/webrev.02/

I had not managed to revert all changes from another patch.

/Erik


On 2016-11-01 17:22, Erik Joelsson wrote:

Hello,

The header file jvmti.h is generated in the hotspot build. There is
also a copy of this file in the jdk repository which is the one that
is actually included in the jdk image. This patch removes the copy and
uses the generated file instead. JDK-8147943 fixed the generated file
so that it has the correct license header, making the files equivalent
at this time, so this change is not changing the build output.

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

Webrev: http://cr.openjdk.java.net/~erikj/8063154/webrev.01/


Looks good.  Approved.  It is nice to get down to one copy of jvmti.h.

Tim

Re: RFR: JDK-8180300 Move JDWP specs to specs directory

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

Magnus,

I've included the serviceability mailing list.
Thank you for catching these links!

But these two files are not the only fixes for this sub-task, right?
Should this webrev include your new markdown files as well?

Thanks,
Serguei



On 6/2/17 06:03, Magnus Ihse Bursie wrote:
The JDWP spec will move to a new place in the "specs" directory in the 
docs image, and links to it needs to be updated.


Bug: https://bugs.openjdk.java.net/browse/JDK-8180300
WebRev: 
http://cr.openjdk.java.net/~ihse/JDK-8180300-move-jdwp-spec/webrev.01


This is a noreg-docs fix and will be pushed to jdk9.

/Magnus

Re: RFR: JDK-8180322 Move JNI spec to specs directory

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

On 6/1/17 18:50, Mandy Chung wrote:

On Jun 1, 2017, at 5:46 AM, Magnus Ihse Bursie  
wrote:

The JNI spec will move to a new place in the "specs" directory in the docs 
image, and links to it needs to be updated.

Bug: https://bugs.openjdk.java.net/browse/JDK-8180322
WebRev: http://cr.openjdk.java.net/~ihse/JDK-8180322-move-jni-spec/webrev.01

This is a noreg-docs fix and will be pushed to jdk9.

Looks okay to me.


+1

Thanks,
Serguei



Mandy

Re: RFR(S): 8022071 Some vm/jvmti tests fail because cannot attach to the Java virtual machine

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

The fix looks good.
The Copyright comment of the WindowsVirtualMachine.c**can be updated 
with 2013.


Thanks,
Serguei

On 8/16/13 9:51 AM, Staffan Larsen wrote:

This failure happens when compiling with the VS 2012 compiler. The attach code 
relies on the order of two methods in the compiled binary and VS 2012 changed 
that order. The solution used is the linker flag /ORDER [1] which allows us to 
specify the order in which methods are laid out in the binary image. Since the 
flag only operates on non-static methods, the methods have been made non-static 
and also changed name so that they will not clash with other methods.

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

The change has been tested with both VS 2010 and VS 2012.

Thanks,
/Staffan


[1] http://msdn.microsoft.com/en-us/library/vstudio/00kh39zz.aspx

Re: RFR(S) Solaris Full Debug Symbols (FDS) fix for 8033602 and 8034005

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

Dan,

The fix looks good.
Nice cleanup from workarounds: Good Thing (TM)! :)

Thanks,
Serguei

On 11/10/14 4:00 PM, Daniel D. Daugherty wrote:

Greetings,

I have a Solaris Full Debug Symbols (FDS) fix ready for review.
Yes, it is a small fix, but it is in Makefiles so feel free to
run screaming from the room... :-)  On the plus side the fix does
delete two work around source files (Coleen would say that's a
Good Thing (TM)!)

The fix is to detect the version of GNU objcopy that is being
used on the machine and only enable Full Debug Symbols when that
version is 2.21.1 or newer. If you don't have the right version,
then the build drops back to pre-FDS build configs with a message
like this:

WARNING: /usr/sfw/bin/gobjcopy --version info:
WARNING: GNU objcopy 2.15
WARNING: an objcopy version of 2.21.1 or newer is needed to create 
valid .debuginfo files.

WARNING: ignoring above objcopy command.
WARNING: patch 149063-01 or newer contains the correct Solaris 10 
SPARC version.
WARNING: patch 149064-01 or newer contains the correct Solaris 10 X86 
version.

WARNING: Solaris 11 Update 1 contains the correct version.
INFO: no objcopy cmd found so cannot create .debuginfo files.
INFO: ENABLE_FULL_DEBUG_SYMBOLS=0

This work is being tracked by the following bug IDs:

JDK-8033602 wrong stabs data in libjvm.debuginfo on JDK 8 - SPARC
https://bugs.openjdk.java.net/browse/JDK-8033602

JDK-8034005 cannot debug in synchronizer.o or objectMonitor.o on 
Solaris X86

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

Here is the webrev URL:

http://cr.openjdk.java.net/~dcubed/8033602-webrev/0-jdk9-hs-rt/

Testing:

- JPRT test jobs to verify that the current JPRT Solaris hosts
  are happy
- local builds on my Solaris 10 X86 machine to verify that the
  wrong version of GNU objcopy is caught

Thanks, in advance, for any comments, questions or suggestions.

Dan