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 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 builds in order to preserve symbolic links

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

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

Thanks, in advance, for any review comments.

Dan

Reply via email to