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

[Coleen Phillimore]

On 5/7/14, 6:38 PM, 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.

Me too.

So this has been a lot of work and a lot of information over email. I 
had one question - why isn't there a FORMAT type that can be used for 
void* ?  Using these p2i functions is going to get really annoying.

Does this also address bug 
https://bugs.openjdk.java.net/browse/JDK-8029996 ?

thanks,
Coleen

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