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

[Daniel D. Daugherty]

On 6/16/17 6: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/

Normally, I would drop a list of the files here and hit them
all one by one. That style's not going to work well with this
review.

I'm going to review the sub-webrevs one by one and only call
out a file if I have comments. I'm going to lose the sanity
check of knowing that I reviewed every file, but...


> 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/

No comments on this sub-webrev.


> 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
>
> (2) http://cr.openjdk.java.net/~kbarrett/8181449/move_format_buffer/

src/share/vm/utilities/formatBuffer.cpp
     jcheck won't like the blank line at the end of the file.


> 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/

src/share/vm/utilities/vmError.cpp
     L210:   if (fr.pc()) {
     L219:       if (t && t->is_Java_thread()) {
         nit - uses implied boolean
         (not your bug, you just moved the code).


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

No comments on this sub-webrev.


> 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/

No comments on this sub-webrev.


> 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.
>
> 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.

Based on the last two paragraphs, I'm now wondering if there
are files changed in the main webrev that aren't in a sub-webrev.
I think I need to go back to the main webrev to make sure I didn't
miss anything:

> http://cr.openjdk.java.net/~kbarrett/8181449/hotspot.00/

No new comments looking at the changes this way.

Thumbs up! Very nice job of detangling...

Dan