RFR(XL): 8181449: Fix debug.hpp / globalDefinitions.hpp dependency inversion

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Mon Jun 19 20:56:50 UTC 2017

On 6/16/17 8:33 PM, Kim Barrett wrote:
> Please review this refactoring of debug.hpp and globalDefinitions.hpp
> so that debug.hpp no longer includes globalDefinitions.hpp.  Instead,
> the include dependency is now in the other direction.  Among other
> things, this permits the use of the assert macros by inline functions
> defined in globalDefinitions.hpp.
> There are a few functions declared toward the end of debug.hpp that
> now seem somewhat misplaced there.  I'm leaving them there for now,
> but will file a new CR to figure out a better place for them, possibly
> in vmError.  There are a number of additional cleanups for dead code
> and the like that I'll be filing as followups; this change is already
> rather large and I didn't want to add more stuff to it.
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8181449
> Testing:
> jprt
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8181449/hotspot.00/
> The full webrev is somewhat large. However, much of the bulk involves
> either adding #includes to files or moving code from one place to
> another without changing it.  To simplify reviewing the changes, I've
> broken it down into a sequence of patches, each associated with a
> particular bit of the refactoring.  The full change is equivalent to
> applying these patches in the given order.  (Note: I don't know if
> applying a subset gives a working repository.)
> (1) http://cr.openjdk.java.net/~kbarrett/8181449/jvm_h/
> a. In preparation for removing the #include of jvm.h from debug.hpp
> (see move_format_buffer webrev), ensured all files that contain
> references to jio_printf variants include jvm.h.  This mostly involved
> adding a #include to lots of files.
> b. For a few header files that referenced jio_printf variants, moved
> the function definition from the .hpp to the corresponding .cpp, and
> added #include of jvm.h to the .cpp.
> - macroAssembler_sparc.[ch]pp
> - macroAssembler_x86.[ch]pp
> - macroAssembler_aarch64.[ch]pp

Well that was boring.   I assume that these files adding #include jvm.h 
actually got an error without the include.
> (2) http://cr.openjdk.java.net/~kbarrett/8181449/move_format_buffer/
> a. Moved FormatBuffer and related stuff from debug.[ch]pp to new
> formatBuffer.[ch]pp, and updated users to #include the new files.
> This includes moving the #include of jvm.h, which is no longer needed
> by debug.hpp.
> b. Made the #include of debug.hpp explicit when already otherwise
> modifying a file that uses assert-like macros, rather than depending
> on indirect inclusion of that file.

> (3) http://cr.openjdk.java.net/~kbarrett/8181449/move_pns/
> a. Moved print_native_stack to VMError class.
> b. Removed unused and undefined pd_obfuscate_location.
> c. Simplified #ifdef PRODUCT guard in ps().

Why can't print_native_stack be in debug.cpp?  I see.  Actually it might 
be better in os.cpp than vmError.cpp.

> (4) http://cr.openjdk.java.net/~kbarrett/8181449/target_macros/
> a. Moved / combined definitions of BREAKPOINT macro from
> globalDefinitions_*.hpp to new breakpoint.hpp.
> b. Deleted all definitions of unused DEBUG_EXCEPTION macro.
> c. Moved gcc-specific ATTRIBUTE_PRINTF, pragma macros, and friends
> from globalDefinitions_gcc.hpp to new compilerWarnings.hpp.  Also
> moved the default definitions for those macros from
> globalDefinitions.hpp to compilerWarnings.hpp.
> d. Added TARGET_COMPILER_HEADER[_INLINE] macros, similar to the
> CPU/OS/OS_CPU/_HEADER[_INLINE] macros, for including files based on
> new INCLUDE_SUFFIX_TARGET_COMPILER macro provided by the build system.


> (5) http://cr.openjdk.java.net/~kbarrett/8181449/flip_depend/
> a. Changed globalDefinitions.hpp to #include debug.hpp, rather than
> the other way around.
> b. Changed globals.hpp to #include globalDefinitions.hpp rather than
> debug.hpp, since it actually needs to former and not the latter.
> c. Changed jvmci_globals.cpp to #include jvm.h, since it's no longer
> being indirectly included via an indirect include of debug.hpp that
> was including globalDefinitions.hpp.
> d. Moved printf-style formatters earlier in globalDefinitions.hpp, so
> they can be used in assert messages in this file.
> e. In globalDefinitions.hpp, changed some #ifdef ASSERT blocks of
> conditional calls to basic_fatal to instead use assert.  While doing
> so, made the error messages more informative.

Also looks good.

> In addition to globals.hpp, there are about 90 files that #include
> debug.hpp but not globalDefinitions.hpp.  The few changes mentioned
> were sufficient to fix missing indirect includes resulting from
> debug.hpp no longer including globalDefinitions.hpp.  There could be
> additional problems with platforms not supported by Oracle though.
> There are also about 40 files which directly include debug.hpp but
> don't appear to use any of the assertion macros.
Hm, might be worth another round to clean these up too.

These changes look good though.  Thank you for the cleanup!!  It wasn't 
as large as I expected (at least this round).


More information about the hotspot-dev mailing list