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