[9] RFR(L): 8037816 : Fix for 8036122 breaks build with Xcode5/clang

[John Rose]

On Apr 24, 2014, at 1:49 PM, David Chase <david.r.chase at oracle.com> wrote:

> (3) these are the parts/questions I think need opinions/resolution:
> 
> (a) print_raw(foo) vs print(“%s”, foo)?

This seems to want some API clarification:  What does print_raw really mean, etc...  But if you start improving APIs and refactoring uses more than absolutely necessary you'll never get this work done.

I favor print_raw(foo) as a more direct (but now legal) rendering of what the programmer wrote.  I sympathize partially with David H.'s objection to print_raw(s) over print("%s", s), but the programmer was not writing a format string here, so don't introduce one.  And sorry, David, I don't buy that printf("%s", s) obviates puts(s).  The latter produces more compact code.  I would favor a rename of print_raw to prints or print_str, but *not* in this change set.

Replacing print_raw feels like "why don't you clean it up as long as you are changing the code".  I think you are right to resist the temptation to do cleanups in this change set, because it is already absurdly large just doing the bare minimum.

> (b) print_cr() vs print_raw_cr(“”) vs print_cr(“%s”, “”) vs cr().
> Note that some of our format-related constructors demand a format, hence they go
> with print("%s", "") when they start empty.

Applying the above reasoning, I do not support introducing "print_cr()" as an alias for "cr()".  If we don't like the name of "cr()" we should change it in a cleanup.  Adding an alias is (a) API tweaking, and (b) leads to pseudo-information in the code which will need cleaning later.

(By pseudo-information, I mean what amounts to a free choice between cr and print_cr injecting a random bit at each use point, and causing future programmers to muse, "he must have a reason for making the distinction; some tool or audience must need that information".)

> (c) Parameter was passed with no format (two cases).  I removed it.  What do we really want here?
> 
> --- old/src/share/vm/opto/gcm.cpp	2014-04-23 14:08:48.000000000 -0400
> +++ new/src/share/vm/opto/gcm.cpp	2014-04-23 14:08:47.000000000 -0400
> @@ -2014,7 +2014,7 @@
>   tty->print("%s: %d  trip_count: %6.0f freq: %6.0f\n",
>              _depth == 0 ? "Method" : "Loop", _id, trip_count(), _freq);
>   for (int i = 0; i < _depth; i++) tty->print("   ");
> -  tty->print("         members:", _id);
> +  tty->print("         members:");
>   int k = 0;
>   for (int i = 0; i < _members.length(); i++) {
>     if (k++ >= 6) {

Correct change.  The _id value is already printed above.

> --- old/src/share/vm/oops/methodData.cpp	2014-04-24 11:18:22.000000000 -0400
> +++ new/src/share/vm/oops/methodData.cpp	2014-04-24 11:18:22.000000000 -0400
> @@ -634,7 +636,7 @@
> }
> 
> void ParametersTypeData::print_data_on(outputStream* st, const char* extra) const {
> -  st->print("parameter types", extra);
> +  st->print("parameter types"); // FIXME extra ignored?
>   _parameters.print_data_on(st);
> }

That's a reasonable fix.  I think Roland will want to take a look at it.

> ---------------------------------------------------------------------
> 
> Among the changes/observations:
> 
> int64_t and jlong are NOT NOT NOT the same type on all platforms.

Lovely.   C has favored us with not only signed and unsigned types, but "unspecified" types as a the portable superposition of both.  Truly the sign of int64_t is the silence of the Tao.

> I tested the various pragma/macros idioms for clang as far back as 3.1, gcc as far back as 4.2 .
> 
> Added some formatting macros:
> #define UINT64_FORMAT_X        "%" PRIx64
> #define PTR_FORMAT_W(width)   "%" #width PRIxPTR
> #define SIZE_FORMAT_XW(width) "%" #width PRIxPTR
> #define SIZE_FORMAT_X         "%" PRIxPTR

A good step.  That large herd of macros may need further taming later.  Your thoughts on this (now that it's all in your head) would be welcome.

> The garbage collector seems to be a little bit nutty about the uintx type.
> Look at some of the command-line options parameters that are uintx,
> in many cases a uint8_t would be completely adequate.
> 
> The mute-gcc macro:
> #define PRAGMA_FORMAT_MUTE_WARNINGS_FOR_GCC _Pragma("GCC diagnostic ignored \"-Wformat\"") _Pragma("GCC diagnostic error \"-Wformat-nonliteral\"") _Pragma("GCC diagnostic error \"-Wformat-security\"")
> 
> ---------------------------------------------------------------------
> 
> +       warning("Failed to reserve shared memory (errno = %d).", errno);

I'm not sure what to do with the snippets at the end of your note, except to say I agree that it is reasonable to insert "(int)" casts for arguments to "%d", at least when there is reason to believe that the programmer would accept a format string containing a truncated value.  The presence of "%d" strongly suggests the programmer expects a small int; same with "%u".  And truncating the value does not add a bug; at most the risk is of continuing to hide a smaller bug.

— John