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

Thu Apr 4 10:53:32 UTC 2019

Hi Götz,

looks good to me, too. Seems like there are no objections against using this syntax. I don't have any ones, either.

I've looked over the code and haven't found any issues.
Most of the hotspot change is only related to the signature change from JNI to external Java syntax.
In addition, I have found:
1. linkResolver.cpp: 1246 to print class name in external format
2. klassVtable.cpp: 1230 to print external_type instead of just "type".
I think these are nice improvements.
I hope I haven't missed anything important to notice.

Best regards,
Martin

-----Original Message-----
From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net> On Behalf Of David Holmes
Sent: Donnerstag, 4. April 2019 01:53
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.

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