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

Tue Jun 20 09:49:50 UTC 2017

Hi Kim,

On 2017-06-17 02:33, 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

OK

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

The following has now been moved to formatBuffer.hpp:

  116 // Used to format messages.
  117 typedef FormatBuffer<> err_msg;

but formatBuffer.hpp is not explicitly included in all files using err_msg.

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

You removed the call to p->trace_stack(). Was that intentional?

    if (p->has_last_Java_frame()) {
      // If the last_Java_fp is set we are in C land and
      // can call the standard stack_trace function.
-#ifdef PRODUCT
      p->print_stack();
    } else {
+#ifdef PRODUCT
      tty->print_cr("Cannot find the last Java frame, printing stack 
disabled.");
  #else // !PRODUCT
-    p->trace_stack();
-  } else {

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

======================================================================
http://cr.openjdk.java.net/~kbarrett/8181449/target_macros/src/share/vm/utilities/globalDefinitions.hpp.frames.html

sort order?:
   28 #include "utilities/macros.hpp"
   29 #include "utilities/compilerWarnings.hpp"

or is this intentional? I see that compilerWarnings.hpp has the comment:
   64 // Defaults when not defined for the TARGET_COMPILER_xxx.

which seems to suggest that macros.hpp need to be included before 
compilerWarnings.hpp.

This used to work when globalDefinitions.hpp dispatched to 
globalDefinitions_<compiler>.hpp, but now this seems fragile.

debug.hpp even includes compilerWarnings.hpp before macros.hpp, so it 
seems like the following attribute is not used when debug.hpp is included!:

-#ifndef ATTRIBUTE_PRINTF
-#define ATTRIBUTE_PRINTF(fmt,vargs)  __attribute__((format(printf, fmt, 
vargs)))
-#endif

That seems like a bug to me.

======================================================================
http://cr.openjdk.java.net/~kbarrett/8181449/target_macros/src/share/vm/utilities/globalDefinitions_gcc.hpp.udiff.html

Maybe also get rid of the following line:

//----------------------------------------------------------------------------------------------------
-// Debugging

or get rid of the following stray new line so that the code in the 
different globalDefinitions files are more consistent.

======================================================================
http://cr.openjdk.java.net/~kbarrett/8181449/target_macros/src/share/vm/utilities/macros.hpp.frames.html

  490 #define CPU_HEADER_STEM(basename) PASTE_TOKENS(basename, 
INCLUDE_SUFFIX_CPU)
  491 #define OS_HEADER_STEM(basename) PASTE_TOKENS(basename, 
INCLUDE_SUFFIX_OS)
  492 #define OS_CPU_HEADER_STEM(basename) PASTE_TOKENS(basename, 
PASTE_TOKENS(INCLUDE_SUFFIX_OS, INCLUDE_SUFFIX_CPU))
  493 #define TARGET_COMPILER_HEADER_STEM(basename) 
PASTE_TOKENS(basename, INCLUDE_SUFFIX_TARGET_COMPILER)

We used to use the TARGET prefix for cpu/arch and os, for example:
-#ifdef TARGET_ARCH_x86
-# include "c1_globals_x86.hpp"
-#endif
-#ifdef TARGET_ARCH_sparc
-# include "c1_globals_sparc.hpp"
-#endif
-#ifdef TARGET_ARCH_arm
-# include "c1_globals_arm.hpp"
-#endif
-#ifdef TARGET_ARCH_ppc
-# include "c1_globals_ppc.hpp"
-#endif
-#ifdef TARGET_ARCH_aarch64
-# include "c1_globals_aarch64.hpp"
-#endif
-#ifdef TARGET_OS_FAMILY_linux
-# include "c1_globals_linux.hpp"

but changed it to:
+#include "utilities/macros.hpp"
+
+#include CPU_HEADER(c1_globals)
+#include OS_HEADER(c1_globals)

with this patch:
http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/8a5735c11a84

Do we want to name the macro COMPILER_HEADER instead of 
TARGET_COMPILER_HEADER?

> 
> (5) http://cr.openjdk.java.net/~kbarrett/8181449/flip_depend/
> 
> a. Changed globalDefinitions.hpp to #include debug.hpp, rather than
> the other way around.

======================================================================
http://cr.openjdk.java.net/~kbarrett/8181449/flip_depend/src/share/vm/utilities/debug.hpp.frames.html

Same comment as in (4) above.

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

I don't see that change.

> 
> d. Moved printf-style formatters earlier in globalDefinitions.hpp, so
> they can be used in assert messages in this file.

OK

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

OK

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

OK.

Thanks,
StefanK