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

[Stefan Karlsson]

On 2014-05-10 03:54, Christian Thalinger wrote:
>
> On May 9, 2014, at 1:27 AM, Stefan Karlsson 
> <stefan.karlsson at oracle.com <mailto:stefan.karlsson at oracle.com>> wrote:
>
>>
>> On 2014-05-09 13:03, David Chase wrote:
>>> On 2014-05-09, at 4:20 AM, Stefan Karlsson 
>>> <stefan.karlsson at oracle.com <mailto: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 
>> <http://cr.openjdk.java.net/%7Edrchase/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 
>>>> <http://cr.openjdk.java.net/%7Edrchase/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.
>
> …which is ironic in itself :-)

Yeah. It's even worse if you see how that value is created ...

StefanK
>
>>
>>>
>>>> 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 
>>>> <http://cr.openjdk.java.net/%7Edrchase/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
>