RFR (S): 8143215: gcc 4.1.2: fix three issues breaking the build.

[Bengt Rutisson]

Hi Kim,

On 2015-12-14 21:02, Kim Barrett wrote:
> On Dec 14, 2015, at 4:43 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>>
>> Hi Kim and Goetz,
>>
>> On 2015-12-13 00:02, Kim Barrett wrote:
>>> On Dec 11, 2015, at 5:52 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>>>> diff -r 73e267ddd93d src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>>>> --- a/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp     Wed Nov 18 11:31:59 2015 +0100
>>>> +++ b/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp     Fri Dec 11 11:42:52 2015 +0100
>>>> @@ -2731,7 +2731,7 @@
>>>>    // Not MT-safe; so do not pass around these StackObj's
>>>>    // where they may be accessed by other threads.
>>>>    jlong wallclock_millis() {
>>>> -    return TimeHelper::counter_to_millis(os::elapsed_counter() - _trace_time.start_time());
>>>> +    return (jlong)TimeHelper::counter_to_millis(os::elapsed_counter() - _trace_time.start_time());
>>>>    }
>>>> };
>>> This conversion was introduced by the recently pushed 8145092: Use Unified Logging for the GC logging.
>>>
>>> The only caller of wallclock_millis compares the result with CMSMaxAbortablePrecleanTime (an intx).
>>>
>>> So I think we’ve always had a bit of a type mismatch here.
>>>
>>> I’m not sure what to do about this one yet, but adding a cast doesn’t seem like the right solution to me.
>> I would suggest changing CMSMaxAbortablePrecleanTime to a double. It may not be the cleanest way to handle the flag but having it as an intx is also not very clean. Negative values won't make sense. Changing it to a double won't make the situation much worse than it is now and it will simplify the code.
> Unfortunately, the syntax for floating point options requires the decimal point and fraction part; integer
> syntax is rejected.  So existing uses of that option would be broken by changing its type.
> CMSMaxAbortablePrecleanTime is a product option, making it not so easy to make such a change.

Right. I would never have expected that from the floating point parsing. 
Seems like a bug to me. But that's a different story.

I agree that making CMSMaxAbortablePrecleanTime is not the right solution.

Thanks,
Bengt

>
>
>>>> diff -r 73e267ddd93d src/share/vm/gc/g1/g1CollectedHeap.cpp
>>>> --- a/src/share/vm/gc/g1/g1CollectedHeap.cpp    Wed Nov 18 11:31:59 2015 +0100
>>>> +++ b/src/share/vm/gc/g1/g1CollectedHeap.cpp    Fri Dec 11 11:42:52 2015 +0100
>>>> @@ -3621,7 +3621,7 @@
>>>>      log_info(gc)("To-space exhausted");
>>>>    }
>>>>
>>>> -  double pause_time_sec = TimeHelper::counter_to_seconds(pause_time_counter);
>>>> +  double pause_time_sec = TimeHelper::counter_to_seconds((jlong)pause_time_counter);
>>>>    g1_policy()->print_phases(pause_time_sec);
>>>>
>>>>    g1_policy()->print_detailed_heap_transition();
>>> Adding a cast is not the proper fix here.
>>>
>>> The problem is that log_gc_footer’s double pause_time_counter argument ought to itself be a jlong.
>>> The caller (do_collection_pause_at_safepoint) contains
>>>
>>>    double pause_start_counter = os::elapsed_counter();
>>>    …
>>>    log_gc_footer(os::elapsed_counter() - pause_start_counter);
>>>
>>> elapsed_counter returns a jlong.  So pause_start_counter ought to be a jlong too, and fixing the
>>> parameter type for log_gc_footer would then make everything work out.
>>>
>>> This appears to be a mistake introduced by the recently pushed 8145092: Use Unified Logging for the GC logging.
>>>
>> Yes, this is a mistake in the push for the unified logging work. I created a patch and sent out a new review request to fix it.
>>
>> RFR: JDK-8145303: Clean up the units for log_gc_footer
>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-December/015903.html
>>
>> http://cr.openjdk.java.net/~brutisso/8145303/webrev.00/
> Thanks for taking care of this.
>