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

Thu Jun 22 00:29:24 UTC 2017

> On Jun 20, 2017, at 5:49 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi Kim,

Thanks for such a careful review of this largish and mostly very
boring to read change.  Responses inline.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8181449/hotspot.01/
incr: http://cr.openjdk.java.net/~kbarrett/8181449/hotspot.01.inc/

> On 2017-06-17 02:33, Kim Barrett wrote:
>> (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.

It seems to be available everywhere needed via indirect includes,
since there weren't any build failures.

There are 25 files referring to err_msg, of which 18 lack a direct
#include of the the new formatBuffer.hpp.

Some uses look like they would be better eliminated by changing
surrounding code.  For example, callers of os::commit_memory_or_exit
pay the cost of building the error message in an err_msg, even in the
normal no-error case.  Addressing that should be done in its own RFE.
https://bugs.openjdk.java.net/browse/JDK-8182679

Since I think existing uses of err_msg ought to be examined and some
eliminated, and there isn't presently a build problem, I'd prefer to
not address these missing includes at this time.  But I could have my
arm twisted...

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

I kept getting confused by this code while trying to figure out what
to do with pd_ps, which is why I changed it.  Seems I was even more
confused than I thought, since I failed to see the difference between
p->print_stack() and p->trace_stack().  Well spotted.  I've reverted
this change.

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

The mis-ordering was not intentional.

TARGET_COMPILER_xxx is defined by the build system, not by macros.hpp.

If compilerWarnings.hpp were changed to use the new
TARGET_COMPILER_HEADER() dispatch (defined in macros.hpp), then the
include would need to be added to compilerWarnings.hpp.

So I don't think there's a bug here, but I'll fix the mis-ordered
includes.

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

Gone now.

> 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:
> […]
> 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?

I'd forgotten about the TARGET_ prefix in the old usage. Yes, dropping
it for the new COMPILER_xxx macros would be consistent, and I mostly
like it now that you've pointed it out. My only concern is that we've
also got COMPILER1 and COMPILER2 based macros, and the visual
distinction is a little subtle. But I'll go ahead and make the change.

> http://cr.openjdk.java.net/~kbarrett/8181449/flip_depend/src/share/vm/utilities/debug.hpp.frames.html
> 
> Same comment as in (4) above.

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

That change got moved to the earlier jvm_h patch, and I forgot to
update this patch description.