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

[David Chase]

On 2014-05-09, at 7:27 AM, Stefan Karlsson <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>
>>  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));

I didn't need to touch the previous line, so I didn't.  What happened here was other reviewers strongly felt that the name of the format for an intptr_t should be INTPTR_FORMAT.  I therefore did a search+replace across the entire patch replacing all instances of (word) PTR_FORMAT with INTPTR_FORMAT.  Because I did not need to touch the previous line in the patch, I did not change those instances.  Ditto for print_raw and print_raw_cr -- they're not in the patch anymore, but there may be other uses elsewhere in the code that I did not patch.

I can't do what you ask without annoying other reviewers.  And if I do what you ask, it then (probably) forces a wholesale revisit of GC code if/when we attempt to redefine PTR_FORMAT to be truly for pointers and get rid of p2i in our internal-use-only debugging code.  (We'll have to do a lot of change anyhow to get rid of the remaining PTR_FORMATs).


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

I could make my patch even larger and change all instances of PTR_FORMAT to INTPTR_FORMAT.  That would be (1) completely consistent in our formatting in the code and (2) consistent with feedback from other reviewers and (3) no enduser visible changes.  It would take us even further down the road towards defining PTR_FORMAT to be truly for pointers (though not with a reliable-width output, and with different CApitAlizAtion on Windows).  How do you (and anyone else reading this) feel about that?  I would rather do it in a second step, because it would also be a large change.