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

David Lindholm david.lindholm at oracle.com
Wed Sep 23 09:37:52 UTC 2015

Kim,

Thanks for looking at this!

On 2015-09-23 01:34, Kim Barrett wrote:

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 RESBUFSZ 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?

Yes, fixed.

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

Lost period at end of comment.

Ok, fixed.

------------------------------------------------------------------------------ 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 errmsgres 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/sharedRuntimeppc.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.]

Ok, removed.

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/sharedRuntimesparc.cpp 319 assert(size <= 8, "%d bytes vectors are not supported", size);_ _320 return size > 8; ------------------------------------------------------------------------------ src/os/solaris/vm/threadCriticalsolaris.cpp 71 fatal("ThreadCritical::~ThreadCritical: mutexunlock failed " 72 "(%s)", strerror(errno)); Remove line break from the format string.

Fixed.

------------------------------------------------------------------------------ src/os/windows/vm/vmErrorwindows.cpp 74 VMError::reportanddie(NULL, exceptioncode, NULL, 75 exceptionInfo->ExceptionRecord, exceptionInfo->ContextRecord);

Fix indentation.

Fixed.

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

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

You are correct, but this file (and many others) silences GCC warnings for format strings with PRAGMA_FORMAT_MUTE_WARNINGS_FOR_GCC at the top of the file. This should absolutely be fixed in all these files, but fixing all these is a major task, and nothing I want to do in this change.

------------------------------------------------------------------------------ src/share/vm/code/vtableStubs.cpp 225 fatal("bad compiled vtable dispatch: receiver " INTPTRFORMAT ", " 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.

Same comment as above. There are more format string problems in this file silenced by the pragma - these should be fixed in another change.

Also pre-existing nearby:

230 #endif // Product "Product" should be all-caps.

Ok, fixed.

I think I'll wait for the rest of your comments before sending out a new webrev.

Thanks, David

More information about the hotspot-dev mailing list