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

Kim Barrett kim.barrett at oracle.com
Tue Sep 22 23:34:58 UTC 2015

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.

More information about the hotspot-dev mailing list