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

[David Holmes]

On 8/05/2014 8:38 AM, Vladimir Kozlov wrote:
> On 5/7/14 11:35 AM, David Chase wrote:
>> Re-revised patch.
>>
>> Main changes from previous:
>>
>> 1) 'print_raw(' introductions replaced with 'print("%s", ' with the
>> exception that the autogenerated code in adlc still uses print_raw
>> because that still seemed slightly less annoying than dealing with the
>> quoted and unquoted format strings appearing in the same fprintf
>> format.  Print_raw was generally judged to be kinda ugly, and it also
>> turned out that there were other stream-like classes with no "raw"
>> equivalent so the "%s", idiom had to be used elsewhere anyway.  I
>> didn't replace cr() with print_cr("%s", "") because there were
>> opposing opinions (and not just mine) as to which choice was uglier.
>
> I am one of who are against using print_cr("%s", "") instead of cr().
> The cr() is well know and used method. And usually it is invoked as
> tty->cr() or st->cr() so its meaning is clear.

The 70 occurrences of print_cr("") versus 800+ for cr() suggests it was 
not universally well known. I would have preferred it to have been 
called print_cr() but that ship sailed long ago. I certainly would never 
suggest using print_cr("%s", "") !

Cheers,
David H.
-------

>
> I see several cases of new print_raw_cr(). Why not print_cr("%s", msg)?
> It is in ciInstanceKlass.cpp, javaClasses.cpp, codeBlob.cpp,
> compilerOracle.cpp, g1CollectedHeap.cpp, psPromotionManager.cpp,
> metaspaceShared.cpp, constMethod.cpp, constantPool.cpp, cpCache.cpp,
> instanceKlass.cpp, method.cpp, loopnode.cpp, jniCheck.cpp, java.cpp,
> os.cpp, sweeper.cpp, thread.cpp, attachListener.cpp,
> diagnosticCommand.cpp, nmtDCmd.cpp, ostream.cpp, vmError.cpp.
>
> In ciEnv.cpp  it can be merged with previous line:
>
>     tty->print("# Compiler replay data is saved as: %s", buffer);
>     tty->print("# Compiler inline data is saved as: %s", buffer);
>
> In ageTable.cpp why you changed type of 'age' from uint to uintx? The
> used format for it is "%3u".
>
> Did you changes some spacing in generateOopMap.cpp? I see only Copyright
> year update.
>
> In memnode.cpp use INTX_FORMAT instead of %" PRIdPTR.
>
> Thanks,
> Vladimir
>
>>
>> 2) PRIxPTR introductions replaced with INTPTR_FORMAT.
>>
>> 3) PTR_FORMAT introductions replaced with INTPTR_FORMAT -- though they
>> are the same, we are printing intptr_t, so the format should be
>> INTPTR_FORMAT.
>>
>> Note that neither PRIxPTR nor PTR_FORMAT are banished -- I merely did
>> not make the confusion worse.
>> If we want to normalize those out altogether, that is another cleanup.
>>
>> 4) I followed Henry Jen's suggestions for sprinkling in more format
>> attributes to reduce the need for disabled checking, and removed the
>> check-disables that were no longer necessary (and I think I got them
>> all).
>>
>> 5) To help ensure against future portability bit-rot, I tracked down
>> more functions/methods that were eligible for format attributes.  This
>> lead to another round of format-use corrections under both gcc and
>> clang (different sets of corrections, I took their union, obviously).
>>
>> 6) I also prepared a backport to 8u to look for any glitches there
>> (there were glitches) and see if they had any influence on the patches
>> to 9 (they didn't).
>>
>> Webrevs:
>> jdk9:   http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-07a/
>> jdk8u: http://cr.openjdk.java.net/~drchase/8037816/webrev-8u.05-07b/
>>
>> Testing:
>> Built "by hand" fastdebug on
>> Mountain Lion + XCode 4.6.3
>> Mavericks + XCode 5.1.1
>> Ubuntu 32-bit + gcc 4.8
>>
>> JPRT of hotspot building on all platforms.
>>
>> JTREG of hotspot/{compiler,gc,runtime} on Mountain Lion
>>
>> 8u has been build-tested on Mountain Lion and Ubuntu-32-gcc-4.8 --
>> out-of-the-box new build still fails to go on Mavericks, though I
>> understand it can be persuaded with enough flags.
>>
>> I'm told that a patch of this size might need careful timing both
>> because it splatters across all of hotspot, and because it will
>> complicate any backports of bug fixes to 8, which is why I prepared a
>> backport patch in addition to the regular one, both to accelerate that
>> process when we need it to happen and to give a feel for what the
>> patch would look like and touch.
>>