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

Thu Apr 4 12:05:49 UTC 2019

Hi Martin, 

thanks for looking at my change!

> In addition, I have found:
> 1. linkResolver.cpp: 1246 to print class name in external format
Yes, other IllegalAccessErrors use the '.' format, too. This is for 
consistency.
> 2. klassVtable.cpp: 1230 to print external_type instead of just "type".
Also for consistency, see LinkageError in systemDictionary.cpp:2101.

Best regards,
  Goetz.

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