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

Kim Barrett kim.barrett at oracle.com
Tue Jun 20 01:38:15 UTC 2017


> On Jun 19, 2017, at 4:56 PM, coleen.phillimore at oracle.com wrote:
> 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.  
>> 
>> […]
>> 
>> 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.

Only in an open build; Oracle's closed code includes jvm.h frequently
enough via other (closed) headers that our supported platforms only
needed a very small number (like 1-3, I forget exactly) additional
includes.  Fortunately, jprt includes some pure open builds in its
testing.  I eventually got tired of adding includes and re-running
jprt, and just mass added the includes.

>> […]
>> (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.

print_native_stack seems to be the "portable" and less verbose
alternative to os::platform_print_native_stack.  VMError::report
selects between them based on requested verbosity.  So VMError seems
like a better place than os.

>> […]
>> 
>> 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.

I’m hoping you are not asking for that to be added to this change.

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

Thanks.



More information about the hotspot-dev mailing list