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

[Vladimir Kozlov]

The same as John said:

Use print_raw(msg) for now and file RFE to rename (I prefer puts()).
Use cr() for empty strings.

os_linux.cpp: you can include space into format now after you added "%s"

-  st->print("%s", os::Linux::glibc_version()); st->print(" ");
---
+  st->print("%s ", os::Linux::glibc_version());

Do the same in your changes in os_posix.cpp instead of using print_raw().

I don't like "0x%" PRIxPTR combination. Can you use PTR_FORMAT instead 
of "0x%" PRIxPTR ?:

+    case T_OBJECT: out->print("obj:0x%" PRIxPTR, p2i(as_jobject())); 
      break;
+    case T_METADATA: out->print("metadata:0x%" PRIxPTR, 
p2i(as_metadata()));break;

and INTPRT_FORMAT for:

out->print("%3d:0x" UINT64_FORMAT_X, type(), (uint64_t)as_jlong());

The places with absent '0x' should be fixed IMHO. For example in 
c1_Runtime1.cpp: 'at pc %" PRIxPTR,'

Can you fix vm_version_<arc>.cpp by casting flags values to (int)?

arguments.cpp: Can you use (uint) instead of (unsigned int)?

Thanks,
Vladimir

On 4/24/14 1:49 PM, David Chase wrote:
> Revised webrev.
>
> Webrev: http://cr.openjdk.java.net/~drchase/8037816/webrev.02/
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8037816
>
> Testing: built everywhere that JPRT builds, personally checked on Mountain Lion (gcc),
> Mavericks (clang), Linux-32 (gcc), Linux-64 (gcc), Solaris-11-sparc (solstudio).
>
> Before you look at the webrev, four things:
>
> (0) I'll handle copyrights in an automated post-pass, if that's okay.  In defense of its absurd size,
> this patch is wide but not very deep; the most common changes were:
>       sprinkle in calls to p2i (277 lines)
>       MUTE_WARNINGS_FOR_GCC (128 lines)
>       white space (120 lines)
>       print_raw[_cr] (108)
>       print_cr() (68)
>       PRAGMA_ defines or push/pop/diagnostic macros. (90)
>       matching format/arg for SIZE_FORMAT (47)
>       ATTRIBUTE_PRINTF (26)
>       Other macro definitions (#endif, #else, remaining format macros) (24)
>       Added "%s" format strings to resolve-nonliteral-string warnings. (20)
>       Other instances of _FORMAT (may be format change or cast arg change) (19)
>       Comments (11)
>       Stray copyright updates (6)
>       "fmt_" macros (6)
>       "PRI.PTR" (5)
> I've appended the remaining uncategorized added/changed lines at the bottom.
>
> (1) the revised change set leaves all the format checking enabled as much as possible for gcc
> and clang compilation, and all gross format errors have been repaired.  This includes our own
> functions and methods with printf-like behavior.  Not only with this build with clang, it should generally
> tend to continue to build with clang.
>
> No new big format mistakes (*) will be allowed anywhere, and new little mistakes will be warned
> except in those files currently marked with PRAGMA_FORMAT_MUTE_WARNINGS_FOR_GCC.
> The intent is that those files will get cleaned up incrementally (one by one, maybe dozen by dozen).
> (*) Defined to be: mismatched format/arg lengths, empty format string, non-literal format string.
>
> (2) there will be additional RFEs covering code I was unsure of; it's as broken/unportable
> as it ever was, but now we know it.
>
> (a) For two files,
>    src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
>    src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp
> the macro ATTRIBUTE_PRINTF(x,y)
> is defined to be empty above the header inclusions; the format strings are wrong in peculiar ways, and they are buried in macros.  This is conditional on not-clang, because clang (1) doesn’t warn about the formats in the same way and (2) has a problem with conflicting definitions before includes and within precompiled header files.
>
> (b) The xsl file used to generate some of the jvmti sources now generates a mute-warnings macro after the header inclusions, because gcc doesn't like some of the format/arg pairs in that generated code, and fixing it was beyond me.
>
>
> (3) these are the parts/questions I think need opinions/resolution:
>
> (a) print_raw(foo) vs print(“%s”, foo)?
>
> (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.
>
> (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) {
>
>
> --- 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);
>   }
>
> ---------------------------------------------------------------------
>
> Among the changes/observations:
>
> int64_t and jlong are NOT NOT NOT the same type on all platforms.
>
> 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
>
> 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);
> +       warning("Failed to attach shared memory (errno = %d).", err);
> +                      _level, (int) MaxBCEAEstimateLevel);
> +        tty->print_cr("code size (%d) exceeds MaxBCEAEstimateSize (%d).",
> +                      method()->code_size(), (int) MaxBCEAEstimateSize);
> +              out->print(" %d %s", (int)(dp_to_di(vdata->dp() + in_bytes(vdata->receiver_offset(i))) / sizeof(intptr_t)), k->name()->as_quoted_ascii());
> +              out->print(" %d %s", (int)(dp_to_di(vdata->dp() + in_bytes(vdata->receiver_offset(i))) / sizeof(intptr_t)), k->name()->as_quoted_ascii());
> +inline void assert_property(bool b, const char* msg, TRAPS) {
> +    tty->print_cr(" #%d %s = %dK (hdr %d%%,  loc %d%%, code %d%%, stub %d%%, [oops %d%%, metadata %d%%, data %d%%, pcs %d%%])",
> +                  (int)(total() / K),
> +              mark_stack_size, (uintx) 1, MarkStackSizeMax);
> +                  MarkStackSize, (uintx) 1, MarkStackSizeMax);
> +      (reserved().byte_size()/K), reserved().byte_size());
> +      (working_promoted/K), working_promoted);
> +      (used_in_bytes()/K), used_in_bytes());
> +      (min_gen_size()/K), min_gen_size());
> +      (max_contraction/K), max_contraction);
> +      (policy->promo_increment(max_contraction)/K),
> +        err_msg("MaxTenuringThreshold should be 0 or markOopDesc::max_age + 1, but is %d", (int) MaxTenuringThreshold));
> +       (int) active_workers, (int) new_active_workers, (int) prev_active_workers,
> +       (int) active_workers_by_JT, (int) active_workers_by_heap_size);
> +        "of %d%%", (int) GCTimeLimit);
> +        (int) GCTimeLimit, gc_overhead_limit_count());
> +    uintx age = 1;
> +        desired_survivor_size*oopSize, (uintx) result, MaxTenuringThreshold);
> +	    age, sizes[age]*oopSize, total*oopSize);
> +        gclog_or_tty->print_cr(
> +                           demand,     old_rate,             rate,     new_rate,                 old_desired,                   _desired);
> +    st->print_cr("%d not in CP[*]?", i);
> +    st->print_cr("%d not in OBJ[*]?", i);
> +    st->print(" <MethodHandle of kind %d index at %d>", kind, i2);
> +  st->print("parameter types"); // FIXME extra ignored?
> +  tty->print("         members:");
> +                  (int) (hi-lo+1), nsing, _max_switch_depth, _est_switch_depth);
> +                    (int) _verify_full_passes);
> +                  (int) _verify_counter, (int) _verify_full_passes);
> +     const char * trace_mesg, TRAPS)  {
> +      (unsigned int) (MarkStackSize / K), (unsigned int) (MarkStackSizeMax / K));
> +    tty->print_cr("ConcGCThreads: %u", (unsigned int) ConcGCThreads);
> +       " based on pause goal of %d (ms)", (int) MaxGCPauseMillis);
> +      (unsigned int) (MarkStackSize / K), (unsigned int) (MarkStackSizeMax / K));
> +    tty->print_cr("ConcGCThreads: %u", (unsigned int) ConcGCThreads);
> +inline void Events::log(Thread* thread, const char* format, ...)  {
> +  return (intptr_t) p;
> +}
>
>