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

[Lindenmaier, Goetz]

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