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

Thu Jun 22 06:49:05 UTC 2017

Hi Kim,

On 2017-06-22 02:29, Kim Barrett wrote:
>> 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/

Looks good. Just a couple of minor nits that would be nice to squash.

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

Fine.

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

OK. I wonder if this code would have been easier to read with to #ifdef 
PRODUCT sections.

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

I agree that this isn't a bug.

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

I wasn't particularly clear on this request, and you cut out the 
following part of my mail:
"or get rid of the following stray new line so that the code in the 
different globalDefinitions files are more consistent."

With your last change the layout is still inconsistent:

globalDefinitions_gcc.hpp
  195
  196 // checking for nanness

globalDefinitions_sparcWorks.hpp
  213 
//----------------------------------------------------------------------------------------------------
  214
  215 // checking for nanness

globalDefinitions_visCPP.hpp
  128 
//----------------------------------------------------------------------------------------------------
  129 // Checking for nanness

  110 
//----------------------------------------------------------------------------------------------------

globalDefinitions_xlc.hpp
  111
  112 // checking for nanness

Could you make all these files consistent in this regard before pushing 
this?

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

OK

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

OK

http://cr.openjdk.java.net/~kbarrett/8181449/hotspot.01.inc/src/share/vm/utilities/globalDefinitions.hpp.udiff.html

   28 #include "utilities/macros.hpp"
   29 #include "utilities/compilerWarnings.hpp"
   30 #include "utilities/debug.hpp"

   31
   32 #include TARGET_COMPILER_HEADER(utilities/globalDefinitions)
   33 // Defaults for macros that might be defined per compiler.
   34 #ifndef NOINLINE
   35 #define NOINLINE
   36 #endif

Would you mind adding a newline between 32 and 32 before pushing?

Thanks,
StefanK

> 
> 
> 