RFR: 8080775: Better argument formatting for assert() and friends

[Kim Barrett]

On Sep 22, 2015, at 7:38 AM, David Lindholm <david.lindholm at oracle.com> wrote:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8080775
> Webrev: http://cr.openjdk.java.net/~david/JDK-8080775/webrev.00/

I've run out of time to look at this today, so here's what I've got so
far.  Looks good so far, just a few minor things.

[I've reviewed the core changes (debug.[ch]pp and vmError.[ch]pp),
along with all of the rest of the utilities directory (at the end of
the webrev file list), and from the top of the webrev list through the
compiler directory, so maybe 1/3 of the way through.]

I'll try to finish in the next day or two.

------------------------------------------------------------------------------ 
src/share/vm/utilities/debug.hpp 
  43 #define BUFSZ 256

Why rename RES_BUFSZ to BUFSZ?  Both, but particularly the latter,
seem like poorly chosen macro names in a widely used header.  Could we
instead have a static constant in FormatBufferBase?

------------------------------------------------------------------------------ 
src/share/vm/utilities/debug.hpp 
 108 // Used to format messages

Lost period at end of comment.

------------------------------------------------------------------------------ 
make/linux/makefiles/gcc.make

> Since we have about 1000 asserts with "" as detailed message, I had to 
> disable the gcc-only warning for empty format strings.

And they aren't even all that concentrated.  At least 141 files in at
least 26 directories.  Bleh!  I guess there's not much choice here.
But a bug report seems called for.

------------------------------------------------------------------------------

There are a number of eliminations of err_msg_res from places where
the associated ResourceMark is not at all obvious.  Since we're
dealing with assertions and the like, so that we're on our way to
process termination, that probably doesn't matter in practice.  Still,
I think that deserves an "Excellent!" for improved code readability.

------------------------------------------------------------------------------ 
src/cpu/ppc/vm/sharedRuntime_ppc.cpp
 478   ResourceMark rm;
 479   // Note, MaxVectorSize == 8 on PPC64.
 480   assert(size <= 8, "%d bytes vectors are not supported", size);
 481   return size > 8;

ResourceMark no longer needed here.  [This is the only lingering
ResourceMark I've spotted so far.]

Unrelated and not your problem, but that assert followed by the
comparison looks really odd!  There's another of these odd assert then
compare sequences (without a nearby ResourceMark) here:

src/cpu/sparc/vm/sharedRuntime_sparc.cpp
 319   assert(size <= 8, "%d bytes vectors are not supported", size);
 320   return size > 8;

------------------------------------------------------------------------------
src/os/solaris/vm/threadCritical_solaris.cpp
  71         fatal("ThreadCritical::~ThreadCritical: mutex_unlock failed "
  72               "(%s)", strerror(errno));

Remove line break from the format string.

------------------------------------------------------------------------------
src/os/windows/vm/vmError_windows.cpp
  74   VMError::report_and_die(NULL, exception_code, NULL,
  75                 exceptionInfo->ExceptionRecord, exceptionInfo->ContextRecord);

Fix indentation.

------------------------------------------------------------------------------
src/share/vm/code/nmethod.cpp
1712       assert(ic->is_clean(), "nmethod " PTR_FORMAT "not clean %s", from, from->method()->name_and_sig_as_C_string());
...
2543     fatal("nmethod at " INTPTR_FORMAT " not in zone", this);
...
2551     fatal("findNMethod did not find this nmethod (" INTPTR_FORMAT ")", this);
...
2556       tty->print_cr("\t\tin nmethod at " INTPTR_FORMAT " (pcs)", this);

Pre-existing: Don't we need the p2i() dance for the values associated
with the [INT]PTR_FORMAT directives?  The last isn't touched by your
changes, but happens to be nearby.

------------------------------------------------------------------------------ 
src/share/vm/code/vtableStubs.cpp
 225   fatal("bad compiled vtable dispatch: receiver " INTPTR_FORMAT ", "
 226         "index %d (vtable length %d)",
 227         (address)receiver, index, vt->length());

Pre-existing: Another p2i() dance; using it would eliminate the need
for the cast.

Also pre-existing nearby:

230 #endif // Product

"Product" should be all-caps.

------------------------------------------------------------------------------