[9] RFR(L): 8037816 : Fix for 8036122 breaks build with Xcode5/clang
Stefan Karlsson
stefan.karlsson at oracle.com
Fri May 9 08:20:44 UTC 2014
On 2014-05-07 20:35, 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.
>
> 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/
I went through the GC changes and it looks like it does what you intend.
I have a couple of things that I would like to get cleaned up before
this gets pushed.
1) You arbitrarily change some of the PTR_FORMAT to INTPTR_FORMAT and
leave others as PTR_FORMAT. Can you leave all of them as PTR_FORMAT in
the GC code?
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/concurrentMark.cpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/concurrentMark.inline.hpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/g1BiasedArray.cpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/g1CardCounts.hpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/heapRegionSeq.inline.hpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/parNew/parOopClosures.inline.hpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/parallelScavenge/parMarkBitMap.hpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.inline.hpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/shared/immutableSpace.cpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_interface/collectedHeap.cpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/memory/cardTableModRefBS.cpp.udiff.html
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/memory/genCollectedHeap.cpp.udiff.html
2) Could you change this to use %u instead:
*! assert(shift_by < sizeof(uintptr_t) * 8, err_msg("Shifting by%zd, larger than word size?", shift_by));*
*! assert(shift_by < sizeof(uintptr_t) * 8, err_msg("Shifting by_" SSIZE_FORMAT ", larger than word size?", (size_t)_ shift_by));*
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/g1BiasedArray.hpp.udiff.html
3) These FIXME comments are not really helpful. Could you give
meaningful comments instead.
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html
25 #if !defined(__clang_major__) && defined(__GNUC__)
26 #define ATTRIBUTE_PRINTF(x,y) // FIXME, formats are a mess.
27 #endif
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp.udiff.html
+ #ifndef __clang_major__
+ #define ATTRIBUTE_PRINTF(x,y) // FIXME, formats are a mess.
+ #endif
4) For the record, I find it highly annoying that we need to mess up the
code base with all these p2i, but I understand the motivation why we
need it for now.
If you fix 1-3 I'm fine with this patch, from a GC perspective.
thanks,
StefanK
> 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.
>
More information about the hotspot-dev
mailing list