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

[David Lindholm]

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

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

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

Fixed.

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

Fixed.

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

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

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