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

[Stefan Karlsson]

On 2014-05-09 13:03, David Chase wrote:
> On 2014-05-09, at 4:20 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>> 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?
> If I didn't change all PTR_FORMAT to INTPTR_FORMAT, it was either because the PTR_FORMAT was already matched with an intptr_t and did not generate a warning, or because a file was so full of warnings that I just slapped a MUTE_WARNINGS macro on the top of it to put this off till later.  The intent, based on feedback from other reviewers, is to consistently print intptr_t with INTPTR_FORMAT, and to change PTR_FORMAT to INTPTR_FORMAT.  However, I did not go looking for additional opportunities to do this beyond the fixes I had to make to get the format-warning code happy, because the patch is already large.

I don't think that's the case. For example, look at:
http://cr.openjdk.java.net/~drchase/8037816/webrev-9.05-08a/src/share/vm/gc_implementation/g1/concurrentMark.cpp.udiff.html

You change one PTR_FORMAT, but not the other three:

       HeapWord* b = hr->bottom();
       HeapWord* e = hr->end();
       HeapWord* t = hr->top();
       HeapWord* p = _g1h->top_at_mark_start(hr, _vo);
       _out->print_cr("** ["PTR_FORMAT", "PTR_FORMAT"] top: "PTR_FORMAT" "
*!                     "TAMS: "PTR_FORMAT, b, e, t, p);*
*!                     "TAMS: "_  INTPTR_FORMAT, p2i(b), p2i(e), p2i(t), p2i(p)_);*


>
> The difficulty with doing what you ask is that it truly does close the door on introducing PTR_FORMAT being some variation on %p.  I was under the impression that GC printing was what is most exposed to end users with automated log-parsing tools, so if we ever did convert PTR_FORMAT to %p, this would have a user-visible impact.  That this also aims us in a direction where we have two ugly macros (PTR_FORMAT and INTPTR_FORMAT) defined to mean exactly the same thing is some suggestion that this would be a mistake (i.e., that the status quo, which the other reviewers wanted me to move away from, is currently a mistake).

It doesn't make sense to change some of our pointers to use 
INTPTR_FORMAT, while other places use PTR_FORMAT. It just makes the code 
inconsistent (yes, I know we already have the inconsistency in some 
places.) IMO, if we want to print a pointer we should use PTR_FORMAT and 
workaround the format problem by using p2i. If that's not what we want, 
we should get rid of PTR_FORMAT. But arbitrarily changing some usage of 
PTR_FORMAT to INTPTR_FORMAT doesn't help us in any direction.

>
>> 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
> i.e., cast shift_by to (int), then.  Will do.

No, shift_int is an uint so no casting is needed.

>
>> 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
> Would this be better?
> // FIXME ,the GC code has formats with mismatched arg counts embedded in complex macros.  Someone who knows what is intended needs to fix this.  Disable this macro definition and compile for more information.

OK. I'll create a bug after you have pushed this.

StefanK

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