RFR: 8141211: Convert TraceExceptions to Unified Logging

[Rachel Protacio]

Hello! An update and updated webrev.

I've rewritten the two portions of code that had used ttyLockers to 
print in one function-call, since there is no locker equivalent in UL, 
plus the fact that it will be easier for users to look for one message's 
contents without the decorators interrupting the lines. (And I 
especially tested that the fix prints the whole line correctly.)

As mentioned in the other thread, I will file a separate RFE to 
deprecate this and other product flags being converted to logging.

Since my last iteration was on hold/not reviewed yet, I've simply 
updated it. http://cr.openjdk.java.net/~rprotacio/8141211.01

Thanks,
Rachel

On 12/9/2015 1:12 PM, Rachel Protacio wrote:
> Hi,
>
> (Sorry if this sent twice. Thunderbird is acting up.)
>
> Thanks for the review. Replies inline.
> Updated webrev: http://cr.openjdk.java.net/~rprotacio/8141211.01/
>
> On 12/8/2015 10:53 PM, David Holmes wrote:
>> Hi Rachel,
>>
>> On 9/12/2015 1:42 AM, Rachel Protacio wrote:
>>> Hello,
>>>
>>> Please review my conversion of -XX:+TraceExceptions to
>>> -Xlog:exceptions=info. The existing (product) flag is aliased to the
>>> logging flag at the info level.
>>
>> Q: how does use of ttyLocker map into UL? I see an awful lot of 
>> multi-line logging blocks that are going to be completely messed up 
>> without locking!
> I assumed that UL worked as-is for these instances, but I will check 
> in with Marcus.
>>
>> src/share/vm/opto/runtime.cpp
>>
>> 1239     ResourceMark rm;
>>
>> What requires the introduction of the ResourceMark?
> A ResourceMark is necessary before using LogHandle output streams.
>>
>> ---
>>
>> src/share/vm/runtime/thread.cpp
>>
>> 2084       if (log_is_enabled(Info, exceptions)) {
>> 2085          ResourceMark rm;
>>
>> An extra leading space has crept in at +2085
>>
>> 2087         logstream->print("Async. exception installed at runtime 
>> exit (" INTPTR_FORMAT ")", p2i(this));
>> 2088          if (has_last_Java_frame()) {
>> 2089            frame f = last_frame();
>> 2090           logstream->print(" (pc: " INTPTR_FORMAT " sp: " 
>> INTPTR_FORMAT " )", p2i(f.pc()), p2i(f.sp()));
>> 2091          }
>>
>> Indention of these lines is wrong at #2088 and #2089 - need an 
>> additional space.
> Thanks for catching these!
>>
>> 2092         logstream->print_cr(" of type: %s", 
>> InstanceKlass::cast(_pending_async_exception->klass())->external_name());
>>
>> Why did the InstanceKlass::cast need to be introduced ??
> That was a vestige from code before a new changeset at the same line, 
> i.e. my merging the repo didn't catch this. I've gotten rid of the cast.
>>
>> ---
>>
>> test/runtime/CommandLine/TraceExceptionsTest.java
>>
>> You improved the readability of the source code by breaking the 
>> @summary over two lines, but IIRC jtreg will write the summary into 
>> the test log with all the additional spaces you added, as the summary 
>> extends until the next tag.
> Ok, I didn't realize that! I've undone that change.
>>
>> ---
>>
>> test/runtime/logging/ExceptionsTest.java
>>
>> Can you avoid the code duplication please.
>>
>> Also perhaps you could/should also check there are no logging 
>> statements present when logging is supposed to be disabled. I'm 
>> guessing the other logging tests haven't considered this. ??
> Good points. I've added a check.
>>
>>> If you have any questions on the alias table (in the arguments.cpp and
>>> .hpp files), Max will chime in as he is the one who implemented that
>>> portion.
>>
>> Okay some general questions.
>>
>> First I would expect that aliased logging options are also marked 
>> deprecated so that we can eventually make them obsolete and remove them.
> I'm not sure about this one. Coleen? Max?
>>
>> Secondly, arguably if someone specifies -XX:-TraceExceptions it 
>> should disable exception logging - which may have been turned on by 
>> -Xlog:all.
>>
>> Which leads to: how does the position of -XX:+/-TraceExceptions 
>> interact with the position of -Xlog:xxx on the command-line (or the 
>> other argument introducing mechanisms) ?
> Very true. I've added an entry in the alias table to that effect.
>>
>> src/share/vm/runtime/arguments.cpp
>>
>> 2743       if(lookup_logging_aliases(option->optionString, 
>> aliased_logging_option)){
>>
>> Need space after "if", and before {
> Done.
>
> Thanks,
> Rachel
>>
>> Thanks,
>> David
>> -----
>>
>>
>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8141211/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8141211
>>>
>>> Testing: jtreg, JPRT, jck vm tests, refworkload performance tests, rbt
>>> quick & non-colo tests
>>>
>>> Thank you!
>>> Rachel
>