[ping] RFR(L): 8221470: Print methods in exception messages in java-like Syntax.

[David Holmes]

Looks good.

I'm re-running through our test system.

Thanks,
David

On 4/04/2019 1:18 am, Lindenmaier, Goetz wrote:
> Hi,
> 
> here a new webrev:
> http://cr.openjdk.java.net/~goetz/wr19/8221470-exMsg-signature/03/
> 
> I have removed the test with the bad class name.
> I named the variables name_str/array_str now.
> 
> I'll push it to jdk-submit for further testing.
> 
> Best regards,
>    Goetz.
> 
> 
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Mittwoch, 3. April 2019 00:42
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'hotspot-runtime-
>> dev at openjdk.java.net' <hotspot-runtime-dev at openjdk.java.net>
>> Subject: Re: [ping] RFR(L): 8221470: Print methods in exception messages in
>> java-like Syntax.
>>
>> Two follow ups ...
>>
>> On 2/04/2019 11:26 pm, Lindenmaier, Goetz wrote:
>>> Hi David,
>>>
>>>> Overall this looks good to me - a few minor nits/comments below.
>>> thanks!
>>>
>>>> I've applied the patch and am running it through our internal build and
>>>> test system (tiers 1-3 initially).
>>>>
>>>> I have a suspicion there will be other tests that need to be updated -
>>>> possibly even JCK tests. Discovering those a-priori will be difficult
>>>> (simply running all the tests would take an extremely long time). Will
>>>> have a discussion about how best to handle those internally.
>>>
>>> I ran most JCK test without problem. They usually don't check messages.
>>> I ran all hotspot, jdk, langtools, nashorn and jaxp test (except
>>> for headful tests).
>>
>> Thanks for the additional testing info. I duplicated some of that but
>> found no issues, other than a couple of closed tests.
>>
>>>> src/hotspot/share/oops/method.cpp
>>>> Please put a blank line after each new method.
>>> Fixed.
>>>
>>>> src/hotspot/share/oops/symbol.cpp
>>>>
>>>> +       os->print(".");
>>>> +     } else {
>>>> +       os->print("%c", start[i]);
>>>>
>>>> Please use os->put(char c) for individual characters.
>>> Fixed.
>>>
>>>> The "start" name would seem better as "buf" to me.
>>> Hmm, buf to me is a local chunk of memory used temporarily.
>>> What about array_sig, class_sig?
>>
>> Not really "sigs".
>>
>> str? Else just leave it.
>>
>> Thanks,
>> David
>> -----
>>
>>>> +     } else if (start[i] == 'L') {
>>>> +       print_class(os, start+i+1, len-i-2);
>>>> Can you insert a comment that help explains the -2:
>>> Done.
>>>
>>>> +   for(SignatureStream ss(this); !ss.is_done(); ss.next()) {
>>>> space after for (2 occurrences)
>>> Fixed.
>>>
>>>>
>> test/hotspot/jtreg/runtime/exceptionMsgs/methodPrinting/TestPrintingMeth
>>>> ods.java
>>>>
>>>> Not sure the special characters can be used directly in the sources. Can
>>>> they not be put in as unicode escapes at all places?
>>> I'll try what Ioi proposed.  I'll post a new webrev including that.
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>
>>>>
>>>> ---
>>>>
>>>> Thanks,
>>>> David
>>>> -------
>>>>
>>>>
>>>> On 1/04/2019 12:32 pm, David Holmes wrote:
>>>>> Hi Goetz,
>>>>>
>>>>> I'm looking at this ...
>>>>>
>>>>> On 29/03/2019 8:26 pm, Lindenmaier, Goetz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Any interest in this change?
>>>>>
>>>>> I'm personally of two minds here because these VM generated exceptions
>>>>> are not only delivered to Java source code. I'd like to know how other
>>>>> language developers using the JVM runtime would view this.
>>>>>
>>>>> That aside if you're going to make a change like this then I think the
>>>>> full signature string has to be quoted in some way to delineate it
>>>>> within the larger message.
>>>>>
>>>>>> Should I split it to adapt the exceptions separately one-by-one to
>>>>>> make the change smaller and simplify the review?
>>>>>
>>>>> I don't think that is necessary.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> I would propose to start out with AbstractMethodError only.
>>>>>>
>>>>>> Best regards,
>>>>>>      Goetz.
>>>>>>
>>>>>>
>>>>>>
>>>>>> From: Lindenmaier, Goetz
>>>>>> Sent: Tuesday, March 26, 2019 1:06 PM
>>>>>> To: hotspot-runtime-dev at openjdk.java.net
>>>>>> Subject: RFR(L): 8221470: Print methods in exception messages in
>>>>>> java-like Syntax.
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> A row of exceptions are thrown from the hotspot runtime.
>>>>>> They print methods with their JNI signatures. To increase
>>>>>> readability and resemblance to source code, this change proposes
>>>>>> to print them in a Java-like syntax.
>>>>>>
>>>>>> Some examples:
>>>>>> current method printouts:
>>>>>>
>>>>>> test.TeMe3_B.ma()V
>>>>>> test.TeMe3_B.ma(IZ[[BF)[[D
>>>>>> test.TeMe3_B.ma([[[Ljava/lang/Object;)[[Ltest/TeMe3_B;
>>>>>>
>>>>>> improved format:
>>>>>>
>>>>>> void test.TeMe3_B.ma()
>>>>>> double[][] test.TeMe3_B.ma(int, boolean, byte[][], float)
>>>>>> test.TeMe3_B[][] test.TeMe3_B.ma(java.lang.Object[][][])
>>>>>>
>>>>>> So far, Method::name_and_sig_as_C_string() is used to print
>>>>>> these messages.
>>>>>>
>>>>>> This change implements function Method::external_name() that prints
>>>>>> the better
>>>>>> format.
>>>>>> external_name() is chosen according to Klass::external_name().
>>>>>>
>>>>>> Printing the better format requires parsing the signature
>>>>>> Symbol. This is implemented in
>>>>>> void Symbol::print_as_signature_external_return_type(outputStream
>> *os);
>>>>>> void Symbol::print_as_signature_external_parameters(outputStream
>> *os);
>>>>>> These method names are chosen according to
>>>>>> Symbol::as_class_external_name().
>>>>>>
>>>>>> See this partial webrev for the new functions:
>>>>>> http://cr.openjdk.java.net/~goetz/wr19/8221470-exMsg-signature/01-
>>>> new_methods/
>>>>>>
>>>>>>
>>>>>> Also, I changed a lot of exception messages to use the new format.
>>>>>> This required to adapt a row of tests. I added a test to check
>>>>>> the signature printing does not regress.  For all these changes, see
>>>>>> the full webrev:
>>>>>> http://cr.openjdk.java.net/~goetz/wr19/8221470-exMsg-signature/01/
>>>>>>
>>>>>> I hope I detected all places where method signatures are printed to
>>>>>> exception messages.
>>>>>>
>>>>>> Best regards,
>>>>>>      Goetz.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>