RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Iris Clark]

Hi, Goetz.

The error messages using "cannot", the preferred word according to the Chicago 
Manual of Style [0], look good to me.

Thanks,
Iris

[0] 
https://www.chicagomanualofstyle.org/qanda/data/faq/topics/Usage/faq0010.html

-Original Message-
From: Lindenmaier, Goetz  
Sent: Thursday, May 9, 2019 1:04 AM
To: Coleen Phillimore ; Schmelter, Ralf 
; Java Core Libs ; 
hotspot-runtime-...@openjdk.java.net; Harold David Seigel 

Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi,

here a webrev which uses 'cannot'. Nothing else changed.
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/10-cannot/

Best regards,
  Goetz.

> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Donnerstag, 9. Mai 2019 08:55
> To: coleen.phillim...@oracle.com; Schmelter, Ralf 
> ; Java Core Libs 
> ; hotspot-runtime- 
> d...@openjdk.java.net; Harold David Seigel 
> Subject: RE: RFR(L): 8218628: Add detailed message to 
> NullPointerException describing what is null.
> 
> Hi,
> 
> > Cannot is apparently preferable in English.
> OK, changed.
> 
> Best regards,
>   Goetz.
> 
> 
> > -Original Message-
> > From: coleen.phillim...@oracle.com 
> > Sent: Mittwoch, 8. Mai 2019 17:32
> > To: Lindenmaier, Goetz ; Schmelter, Ralf 
> > ; Java Core Libs 
> > ; 
> > hotspot-runtime-...@openjdk.java.net; Harold David Seigel 
> > 
> > Subject: Re: RFR(L): 8218628: Add detailed message to 
> > NullPointerException describing what is null.
> >
> >
> >
> > On 5/7/19 9:36 AM, Lindenmaier, Goetz wrote:
> > > Hi,
> > >
> > > Please have a look at this further improved webrev:
> > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
> > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-
> > incremental/
> > >
> > > Harold, Coleen, thanks for pointing me to those methods.
> > > I'm using them now. This simplifies the helper methods 
> > > considerably.
> > >
> > > Ralf, thanks for looking at the messages!
> > > I now suppress the "static " and
> > > "The return value of '" strings in the array subscript expressions 
> > > and added corresponding test cases.
> > >
> > > About "can not" versus "cannot", what I find in the net "cannot" 
> > > is to be preferred.  Any comments on that?
> > > By native speakers?
> > > See example messages here:
> > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> > NPE/09/output_with_debug_info.txt
> >
> > Cannot is apparently preferable in English.  Native speaker (only
> > language) but somebody had to tell me.
> >
> > Coleen
> > >
> > > Further, I fixed a build issue with the solaris compiler.
> > > All handling of bci is now unsigned.
> > >
> > > Best regards,
> > >Goetz.
> > >
> > >
> > >
> > >
> > >> -Original Message-
> > >> From: Schmelter, Ralf
> > >> Sent: Dienstag, 7. Mai 2019 14:35
> > >> To: Lindenmaier, Goetz ; Java Core 
> > >> Libs
> >  > >> libs-...@openjdk.java.net>; hotspot-runtime-...@openjdk.java.net;
> > Coleen
> > >> Phillimore (coleen.phillim...@oracle.com)
> 
> > >> Subject: RE: RFR(L): 8218628: Add detailed message to
> NullPointerException
> > >> describing what is null.
> > >>
> > >> Hi Goetz,
> > >>
> > >> I've played with the messages a little bit and they generally 
> > >> look good. But
> > I've
> > >> come across two which look strange:
> > >>   - 'o[static Test.INDEX]' is null. Can not invoke method 'int 
> > >> java.lang.Object.hashCode()'
> > >>   - 'o[The return value of 'int java.lang.String.hashCode()]' is 
> > >> null. Can not
> > invoke
> > >> method 'int java.lang.Object.hashCode()'.
> > >>
> > >> Here is the test program to reproduce these:
> > >> public class Test {
> > >>  public static int INDEX = 2;
> > >>
> > >>  public static void main(String[] args) {
> > >>  Object[] o = new Object[10];
> > >>  try {
> > >>   o[INDEX].hashCode();
> > >>  } catch (Exception e) {
> > >>  e.printStackTrace();
> > >>  }
> > >>  try {
> > >>   o["".hashCode()].hashCode();
> > >>  } catch (Exception e) {
> > >>  e.printStackTrace();
> > >>  }
> > >>  }
> > >> }
> > >>
> > >> And 'Can not' should be 'Cannot'?
> > >>
> > >> Best regards,
> > >> Ralf

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

here a webrev which uses 'cannot'. Nothing else changed.
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/10-cannot/

Best regards,
  Goetz.

> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Donnerstag, 9. Mai 2019 08:55
> To: coleen.phillim...@oracle.com; Schmelter, Ralf ;
> Java Core Libs ; hotspot-runtime-
> d...@openjdk.java.net; Harold David Seigel 
> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi,
> 
> > Cannot is apparently preferable in English.
> OK, changed.
> 
> Best regards,
>   Goetz.
> 
> 
> > -Original Message-
> > From: coleen.phillim...@oracle.com 
> > Sent: Mittwoch, 8. Mai 2019 17:32
> > To: Lindenmaier, Goetz ; Schmelter, Ralf
> > ; Java Core Libs ;
> > hotspot-runtime-...@openjdk.java.net; Harold David Seigel
> > 
> > Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> > describing what is null.
> >
> >
> >
> > On 5/7/19 9:36 AM, Lindenmaier, Goetz wrote:
> > > Hi,
> > >
> > > Please have a look at this further improved webrev:
> > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
> > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-
> > incremental/
> > >
> > > Harold, Coleen, thanks for pointing me to those methods.
> > > I'm using them now. This simplifies the helper methods
> > > considerably.
> > >
> > > Ralf, thanks for looking at the messages!
> > > I now suppress the "static " and
> > > "The return value of '" strings in the array subscript
> > > expressions and added corresponding test cases.
> > >
> > > About "can not" versus "cannot", what I find in the
> > > net "cannot" is to be preferred.  Any comments on that?
> > > By native speakers?
> > > See example messages here:
> > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> > NPE/09/output_with_debug_info.txt
> >
> > Cannot is apparently preferable in English.  Native speaker (only
> > language) but somebody had to tell me.
> >
> > Coleen
> > >
> > > Further, I fixed a build issue with the solaris compiler.
> > > All handling of bci is now unsigned.
> > >
> > > Best regards,
> > >Goetz.
> > >
> > >
> > >
> > >
> > >> -Original Message-
> > >> From: Schmelter, Ralf
> > >> Sent: Dienstag, 7. Mai 2019 14:35
> > >> To: Lindenmaier, Goetz ; Java Core Libs
> >  > >> libs-...@openjdk.java.net>; hotspot-runtime-...@openjdk.java.net;
> > Coleen
> > >> Phillimore (coleen.phillim...@oracle.com)
> 
> > >> Subject: RE: RFR(L): 8218628: Add detailed message to
> NullPointerException
> > >> describing what is null.
> > >>
> > >> Hi Goetz,
> > >>
> > >> I've played with the messages a little bit and they generally look good. 
> > >> But
> > I've
> > >> come across two which look strange:
> > >>   - 'o[static Test.INDEX]' is null. Can not invoke method 'int
> > >> java.lang.Object.hashCode()'
> > >>   - 'o[The return value of 'int java.lang.String.hashCode()]' is null. 
> > >> Can not
> > invoke
> > >> method 'int java.lang.Object.hashCode()'.
> > >>
> > >> Here is the test program to reproduce these:
> > >> public class Test {
> > >>  public static int INDEX = 2;
> > >>
> > >>  public static void main(String[] args) {
> > >>  Object[] o = new Object[10];
> > >>  try {
> > >>   o[INDEX].hashCode();
> > >>  } catch (Exception e) {
> > >>  e.printStackTrace();
> > >>  }
> > >>  try {
> > >>   o["".hashCode()].hashCode();
> > >>  } catch (Exception e) {
> > >>  e.printStackTrace();
> > >>  }
> > >>  }
> > >> }
> > >>
> > >> And 'Can not' should be 'Cannot'?
> > >>
> > >> Best regards,
> > >> Ralf

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

> Cannot is apparently preferable in English.  
OK, changed. 

Best regards,
  Goetz.


> -Original Message-
> From: coleen.phillim...@oracle.com 
> Sent: Mittwoch, 8. Mai 2019 17:32
> To: Lindenmaier, Goetz ; Schmelter, Ralf
> ; Java Core Libs ;
> hotspot-runtime-...@openjdk.java.net; Harold David Seigel
> 
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> 
> 
> On 5/7/19 9:36 AM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > Please have a look at this further improved webrev:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-
> incremental/
> >
> > Harold, Coleen, thanks for pointing me to those methods.
> > I'm using them now. This simplifies the helper methods
> > considerably.
> >
> > Ralf, thanks for looking at the messages!
> > I now suppress the "static " and
> > "The return value of '" strings in the array subscript
> > expressions and added corresponding test cases.
> >
> > About "can not" versus "cannot", what I find in the
> > net "cannot" is to be preferred.  Any comments on that?
> > By native speakers?
> > See example messages here:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> NPE/09/output_with_debug_info.txt
> 
> Cannot is apparently preferable in English.  Native speaker (only
> language) but somebody had to tell me.
> 
> Coleen
> >
> > Further, I fixed a build issue with the solaris compiler.
> > All handling of bci is now unsigned.
> >
> > Best regards,
> >Goetz.
> >
> >
> >
> >
> >> -----Original Message-----
> >> From: Schmelter, Ralf
> >> Sent: Dienstag, 7. Mai 2019 14:35
> >> To: Lindenmaier, Goetz ; Java Core Libs
>  >> libs-...@openjdk.java.net>; hotspot-runtime-...@openjdk.java.net;
> Coleen
> >> Phillimore (coleen.phillim...@oracle.com) 
> >> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> >> describing what is null.
> >>
> >> Hi Goetz,
> >>
> >> I've played with the messages a little bit and they generally look good. 
> >> But
> I've
> >> come across two which look strange:
> >>   - 'o[static Test.INDEX]' is null. Can not invoke method 'int
> >> java.lang.Object.hashCode()'
> >>   - 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can 
> >> not
> invoke
> >> method 'int java.lang.Object.hashCode()'.
> >>
> >> Here is the test program to reproduce these:
> >> public class Test {
> >>  public static int INDEX = 2;
> >>
> >>  public static void main(String[] args) {
> >>  Object[] o = new Object[10];
> >>  try {
> >>   o[INDEX].hashCode();
> >>  } catch (Exception e) {
> >>  e.printStackTrace();
> >>  }
> >>  try {
> >>   o["".hashCode()].hashCode();
> >>  } catch (Exception e) {
> >>  e.printStackTrace();
> >>  }
> >>  }
> >> }
> >>
> >> And 'Can not' should be 'Cannot'?
> >>
> >> Best regards,
> >> Ralf

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[coleen . phillimore]

On 5/7/19 9:36 AM, Lindenmaier, Goetz wrote:

Hi,

Please have a look at this further improved webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-incremental/

Harold, Coleen, thanks for pointing me to those methods.
I'm using them now. This simplifies the helper methods
considerably.

Ralf, thanks for looking at the messages!
I now suppress the "static " and
"The return value of '" strings in the array subscript
expressions and added corresponding test cases.

About "can not" versus "cannot", what I find in the
net "cannot" is to be preferred.  Any comments on that?
By native speakers?
See example messages here:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/output_with_debug_info.txt


Cannot is apparently preferable in English.  Native speaker (only 
language) but somebody had to tell me.


Coleen


Further, I fixed a build issue with the solaris compiler.
All handling of bci is now unsigned.

Best regards,
   Goetz.





-Original Message-
From: Schmelter, Ralf
Sent: Dienstag, 7. Mai 2019 14:35
To: Lindenmaier, Goetz ; Java Core Libs ; hotspot-runtime-...@openjdk.java.net; Coleen
Phillimore (coleen.phillim...@oracle.com) 
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

Hi Goetz,

I've played with the messages a little bit and they generally look good. But 
I've
come across two which look strange:
  - 'o[static Test.INDEX]' is null. Can not invoke method 'int
java.lang.Object.hashCode()'
  - 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can not 
invoke
method 'int java.lang.Object.hashCode()'.

Here is the test program to reproduce these:
public class Test {
 public static int INDEX = 2;

 public static void main(String[] args) {
 Object[] o = new Object[10];
 try {
  o[INDEX].hashCode();
 } catch (Exception e) {
 e.printStackTrace();
 }
 try {
  o["".hashCode()].hashCode();
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
}

And 'Can not' should be 'Cannot'?

Best regards,
Ralf

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Schmelter, Ralf]

Hi Goetz,

I've played with the messages a little bit and they generally look good. But 
I've come across two which look strange:
 - 'o[static Test.INDEX]' is null. Can not invoke method 'int 
java.lang.Object.hashCode()'
 - 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can not 
invoke method 'int java.lang.Object.hashCode()'.

Here is the test program to reproduce these:
public class Test {
public static int INDEX = 2;

public static void main(String[] args) {
Object[] o = new Object[10];
try {
 o[INDEX].hashCode();
} catch (Exception e) {
e.printStackTrace();
}
try {
 o["".hashCode()].hashCode();
} catch (Exception e) {
e.printStackTrace();
}
}
}

And 'Can not' should be 'Cannot'?

Best regards,
Ralf

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

Please have a look at this further improved webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-incremental/

Harold, Coleen, thanks for pointing me to those methods.
I'm using them now. This simplifies the helper methods
considerably.

Ralf, thanks for looking at the messages!
I now suppress the "static " and 
"The return value of '" strings in the array subscript
expressions and added corresponding test cases.

About "can not" versus "cannot", what I find in the 
net "cannot" is to be preferred.  Any comments on that?
By native speakers?
See example messages here: 
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/output_with_debug_info.txt

Further, I fixed a build issue with the solaris compiler.
All handling of bci is now unsigned.

Best regards,
  Goetz.




> -Original Message-
> From: Schmelter, Ralf
> Sent: Dienstag, 7. Mai 2019 14:35
> To: Lindenmaier, Goetz ; Java Core Libs  libs-...@openjdk.java.net>; hotspot-runtime-...@openjdk.java.net; Coleen
> Phillimore (coleen.phillim...@oracle.com) 
> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> 
> I've played with the messages a little bit and they generally look good. But 
> I've
> come across two which look strange:
>  - 'o[static Test.INDEX]' is null. Can not invoke method 'int
> java.lang.Object.hashCode()'
>  - 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can not 
> invoke
> method 'int java.lang.Object.hashCode()'.
> 
> Here is the test program to reproduce these:
> public class Test {
> public static int INDEX = 2;
> 
> public static void main(String[] args) {
> Object[] o = new Object[10];
> try {
>  o[INDEX].hashCode();
> } catch (Exception e) {
> e.printStackTrace();
> }
> try {
>  o["".hashCode()].hashCode();
> } catch (Exception e) {
> e.printStackTrace();
> }
> }
> }
> 
> And 'Can not' should be 'Cannot'?
> 
> Best regards,
> Ralf

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

incorporating comments by Coleen, I prepared a new webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/08/ 
incremental webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/08-incremental/

I renamed the following new classes: 
  TrackingStackCreator  -> ExcpetionMessageBuilder
  SimulatedOperandStack -> SimulatedOperandStack
  StackSlotAnalysisData -> StackSlotAnalysisData
and moved their declaration to the .cpp file.

A new wrapper class BytecodeUtils contains the 
method that is called from jvm.cpp. Some code
is moved from jvm.cpp to bytecodeUtils.cpp
so the internal data structures are no more exposed.

StackSlotAnalysisData now has two fields getting
rid of / and %. This also makes debugging more simple.

Lots of comments were added or improved.

I adapted test langtools/jdk/jshell/ToolSimpleTest.java
to the new message.

Coleen, thanks for your help!

Best regards,
  Goetz.



> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Freitag, 12. April 2019 12:33
> To: Mandy Chung ; Roger Riggs
> 
> Cc: Java Core Libs ; hotspot-runtime-
> d...@openjdk.java.net
> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi,
> 
> while waiting for progress on corresponding the JEP, I improved
> the implementation of generating the NPE message.  It now uses
> a single outputStream. This removes several allocations of temporary
> data. I also removed TrackingStackSource. The analysis code originally
> addressed several use cases, for NullPointerExceptions this is
> not needed.  I cleaned up bytecodeUtils from some code not (really) needed.
> 
> I split get_null_pointer_slot() into two methods: get_NPE_null_slot()
> and print_NPE_failed_action(). This simplifies the
> implementation, and streamlines it more with the text in the JEP.
> 
> I print methods using the code added in
> "8221470: Print methods in exception messages in java-like Syntax.",
> so it now prints 'void m(int)' instead of 'm(I)V'.
> 
> I implemented a row of new test cases, and rearranged the test
> to test the message part of print_NPE_failed_action() and
> print_NPE_cause() separated. I made sure all bytecodes
> handled in these methods are covered.
> Further I arranged the tests in methods according to the
> functional properties as discussed in the JEP.
> 
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/07
> 
> Best regards,
>   Goetz.
> 
> 
> 
> 
> 
> > -Original Message-
> > From: Lindenmaier, Goetz
> > Sent: Donnerstag, 14. März 2019 21:56
> > To: 'Mandy Chung' ; 'Roger Riggs'
> > 
> > Cc: 'Java Core Libs' ; 'hotspot-runtime-
> > d...@openjdk.java.net' 
> > Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> > describing what is null.
> >
> > Hi,
> >
> > I had promised to work on a better wording of the messages.
> >
> > This I deliver with this webrev:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> > otherMessages/
> >
> > The test in the webrev is modified to just print messages along with the
> > code that raised the messages.
> >
> > Please have a look at these files with test output contained in the webrev:
> > Messages with debug information:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> > otherMessages/output_with_debug_info.txt
> > Messages without debug information:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> > otherMessages/output_no_debug_info.txt
> >
> > Especially look at the first few messages, they point out the usefulness
> > of this change.  They precisely say what was null in a chain of 
> > dereferences.
> >
> > Best regards,
> >   Goetz.
> >
> >
> > > -Original Message-
> > > From: Lindenmaier, Goetz
> > > Sent: Wednesday, February 13, 2019 10:09 AM
> > > To: 'Mandy Chung' ; Roger Riggs
> > > 
> > > Cc: Java Core Libs ; hotspot-runtime-
> > > d...@openjdk.java.net
> > > Subject: RE: RFR(L): 8218628: Add detailed message to
> NullPointerException
> > > describing what is null.
> > >
> > > Hi Mandy,
> > >
> > > Thanks for supporting my intend of adding the message as such!
> > > I'll start implementing this in Java and come back with a webrev
> > > in a while.
> > >
> > > In parallel, I would like to continue discussing the other
> > > topics, e.g., the wording of the message. I will probably come up
> > > with a separate webrev for that.
> > >
> > > Best rega

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Jason Mehrens]

Hi Goetz,

Looking at the test cases I didn't see any tests for the single argument 
java.util.Objects.requireNonNull.  Using this prototype is that method treated 
like a hidden frame?

Cheers,

Jason

From: core-libs-dev  on behalf of 
Lindenmaier, Goetz 
Sent: Friday, April 12, 2019 5:33 AM
To: Mandy Chung; Roger Riggs
Cc: Java Core Libs; hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi,

while waiting for progress on corresponding the JEP, I improved
the implementation of generating the NPE message.  It now uses
a single outputStream. This removes several allocations of temporary
data. I also removed TrackingStackSource. The analysis code originally
addressed several use cases, for NullPointerExceptions this is
not needed.  I cleaned up bytecodeUtils from some code not (really) needed.

I split get_null_pointer_slot() into two methods: get_NPE_null_slot()
and print_NPE_failed_action(). This simplifies the
implementation, and streamlines it more with the text in the JEP.

I print methods using the code added in
"8221470: Print methods in exception messages in java-like Syntax.",
so it now prints 'void m(int)' instead of 'm(I)V'.

I implemented a row of new test cases, and rearranged the test
to test the message part of print_NPE_failed_action() and
print_NPE_cause() separated. I made sure all bytecodes
handled in these methods are covered.
Further I arranged the tests in methods according to the
functional properties as discussed in the JEP.

http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/07

Best regards,
  Goetz.





> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Donnerstag, 14. März 2019 21:56
> To: 'Mandy Chung' ; 'Roger Riggs'
> 
> Cc: 'Java Core Libs' ; 'hotspot-runtime-
> d...@openjdk.java.net' 
> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
>
> Hi,
>
> I had promised to work on a better wording of the messages.
>
> This I deliver with this webrev:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/
>
> The test in the webrev is modified to just print messages along with the
> code that raised the messages.
>
> Please have a look at these files with test output contained in the webrev:
> Messages with debug information:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/output_with_debug_info.txt
> Messages without debug information:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/output_no_debug_info.txt
>
> Especially look at the first few messages, they point out the usefulness
> of this change.  They precisely say what was null in a chain of dereferences.
>
> Best regards,
>   Goetz.
>
>
> > -Original Message-
> > From: Lindenmaier, Goetz
> > Sent: Wednesday, February 13, 2019 10:09 AM
> > To: 'Mandy Chung' ; Roger Riggs
> > 
> > Cc: Java Core Libs ; hotspot-runtime-
> > d...@openjdk.java.net
> > Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> > describing what is null.
> >
> > Hi Mandy,
> >
> > Thanks for supporting my intend of adding the message as such!
> > I'll start implementing this in Java and come back with a webrev
> > in a while.
> >
> > In parallel, I would like to continue discussing the other
> > topics, e.g., the wording of the message. I will probably come up
> > with a separate webrev for that.
> >
> > Best regards,
> >   Goetz.
> >
> >
> >
> > > -Original Message-
> > > From: core-libs-dev  On Behalf
> > > Of Mandy Chung
> > > Sent: Tuesday, February 12, 2019 7:32 PM
> > > To: Roger Riggs 
> > > Cc: Java Core Libs ; hotspot-runtime-
> > > d...@openjdk.java.net
> > > Subject: Re: RFR(L): 8218628: Add detailed message to
> > NullPointerException
> > > describing what is null.
> > >
> > > On 2/8/19 11:46 AM, Roger Riggs wrote:
> > > > Hi,
> > > >
> > > > A few higher level issues should be considered, though the details
> > > > of the webrev captured my immediate attention.
> > > >
> > > > Is this the right feature and is this the right level of implementation
> > > > (C++/native)?
> > > > :
> > > > How much of this can be done in Java code with StackWalker and other
> > > > java APIs?
> > > > It would be a shame to add this much native code if there was a more
> > > robust
> > > > way to implement it using AP

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

while waiting for progress on corresponding the JEP, I improved 
the implementation of generating the NPE message.  It now uses
a single outputStream. This removes several allocations of temporary
data. I also removed TrackingStackSource. The analysis code originally 
addressed several use cases, for NullPointerExceptions this is 
not needed.  I cleaned up bytecodeUtils from some code not (really) needed.

I split get_null_pointer_slot() into two methods: get_NPE_null_slot() 
and print_NPE_failed_action(). This simplifies the
implementation, and streamlines it more with the text in the JEP.

I print methods using the code added in 
"8221470: Print methods in exception messages in java-like Syntax.",
so it now prints 'void m(int)' instead of 'm(I)V'.

I implemented a row of new test cases, and rearranged the test 
to test the message part of print_NPE_failed_action() and 
print_NPE_cause() separated. I made sure all bytecodes 
handled in these methods are covered.  
Further I arranged the tests in methods according to the
functional properties as discussed in the JEP.

http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/07

Best regards,
  Goetz.





> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Donnerstag, 14. März 2019 21:56
> To: 'Mandy Chung' ; 'Roger Riggs'
> 
> Cc: 'Java Core Libs' ; 'hotspot-runtime-
> d...@openjdk.java.net' 
> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi,
> 
> I had promised to work on a better wording of the messages.
> 
> This I deliver with this webrev:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/
> 
> The test in the webrev is modified to just print messages along with the
> code that raised the messages.
> 
> Please have a look at these files with test output contained in the webrev:
> Messages with debug information:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/output_with_debug_info.txt
> Messages without debug information:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/output_no_debug_info.txt
> 
> Especially look at the first few messages, they point out the usefulness
> of this change.  They precisely say what was null in a chain of dereferences.
> 
> Best regards,
>   Goetz.
> 
> 
> > -Original Message-
> > From: Lindenmaier, Goetz
> > Sent: Wednesday, February 13, 2019 10:09 AM
> > To: 'Mandy Chung' ; Roger Riggs
> > 
> > Cc: Java Core Libs ; hotspot-runtime-
> > d...@openjdk.java.net
> > Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> > describing what is null.
> >
> > Hi Mandy,
> >
> > Thanks for supporting my intend of adding the message as such!
> > I'll start implementing this in Java and come back with a webrev
> > in a while.
> >
> > In parallel, I would like to continue discussing the other
> > topics, e.g., the wording of the message. I will probably come up
> > with a separate webrev for that.
> >
> > Best regards,
> >   Goetz.
> >
> >
> >
> > > -Original Message-
> > > From: core-libs-dev  On Behalf
> > > Of Mandy Chung
> > > Sent: Tuesday, February 12, 2019 7:32 PM
> > > To: Roger Riggs 
> > > Cc: Java Core Libs ; hotspot-runtime-
> > > d...@openjdk.java.net
> > > Subject: Re: RFR(L): 8218628: Add detailed message to
> > NullPointerException
> > > describing what is null.
> > >
> > > On 2/8/19 11:46 AM, Roger Riggs wrote:
> > > > Hi,
> > > >
> > > > A few higher level issues should be considered, though the details
> > > > of the webrev captured my immediate attention.
> > > >
> > > > Is this the right feature and is this the right level of implementation
> > > > (C++/native)?
> > > > :
> > > > How much of this can be done in Java code with StackWalker and other
> > > > java APIs?
> > > > It would be a shame to add this much native code if there was a more
> > > robust
> > > > way to implement it using APIs with more leverage.
> > >
> > > Improving the NPE message for better diagnosability is helpful while
> > > I share the same concern Roger raised.
> > >
> > > Implementing this feature in Java and the library would be a better
> > > choice as this isn't absolutely required to be done in VM in native.
> > >
> > > NPE keeps a backtrace capturing the method id and bci of each stack
> > > frame.  One option to explore is to have StackWalker to accept a
> > > Throwable object that returns a stream of StackFrame which allows
> > > you to get the method and BCI and also code source (I started a
> > > prototype for JDK-8189752 some time ago).  It can use the bytecode
> > > library e.g. ASM to read the bytecode.  For NPE message, you can
> > > implement a specialized StackFrameTraverser just for building
> > > an exception message purpose.
> > >
> > > Mandy

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Maurizio, 

I put your java PoC into a webrev, and added some tests:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/06-java-Maurizio/

Please have a look at NullPointerExceptionTestMini4.java.
It is failing, as the class does not reside in a file. 
Do you have an idea how to fix this?

Further, I extended the implementation to cover all necessary bytecodes.
I had hoped it would print a message on each test case in 
NullPointerExceptionTest.
But it seems to lose track of the byte code index at some point
if the method gets bigger.
I have an idea how to fix this, though, so I'm not concerned about 
that.

I'm currently looking at how to identify hidden frames...

Best regards,
  Goetz.

> -Original Message-
> From: Maurizio Cimadamore 
> Sent: Freitag, 15. März 2019 12:33
> To: Lindenmaier, Goetz ; Mandy Chung
> ; Roger Riggs 
> Cc: Java Core Libs ; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> please find the attached ASM-based patch. It is just a PoC, as such it
> does not provide as fine-grained messages as the one discussed in the
> RFE/JEP, but can be enhanced to cover custom debugging attribute, I believe.
> 
> When running this:
> 
> Object o = null;
> o.toString();
> 
> you get:
> 
> Exception in thread "main" java.lang.NullPointerException: attempt to
> dereference 'null' when calling method 'toString'
>      at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)
> 
> While when running this:
> 
> Foo foo = null;
> int y = foo.x;
> 
> You get this:
> 
> Exception in thread "main" java.lang.NullPointerException: attempt to
> dereference 'null' when accessing field 'x'
>      at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)
> 
> One problem I had is that ASM provides no way to get the instruction
> given a program counter - which means we have to scan all the bytecodes
> and update the sizes as we go along, and, ASM unfortunately doesn’t
> expose opcode sizes either. A more robust solution would be to have a
> big switch which returned the opcode size of any given opcode. Also,
> accessing to StackWalker API on exception creation might not be
> desirable in terms of performances, so this might be one of these area
> where some VM help could be beneficial. Another problem is that we
> cannot distinguish between user-generated exceptions (e.g. `throw new
> NullPointerException`) and genuine NPE issued by the VM.
> 
> But I guess the upshot is that it works to leave all the gory detail of
> bytecode grovelling to a bytecode API - if the logic is applied lazily,
> then the impact on performances should be minimal, and the solution more
> maintainable longer term.
> 
> Cheers
> Maurizio
> 
> On 15/03/2019 07:59, Lindenmaier, Goetz wrote:
> > Yes, it would be nice if you shared that.

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Peter, 

> -Original Message-
> From: Peter Levart 
> Sent: Freitag, 29. März 2019 16:44
> To: Lindenmaier, Goetz ; 'Mandy Chung'
> 
> Cc: core-libs-dev@openjdk.java.net; maurizio.cimadam...@oracle.com;
> hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> 
> 
> On 3/29/19 4:36 PM, Peter Levart wrote:
> >
> >
> > On 3/29/19 8:49 AM, Lindenmaier, Goetz wrote:
> >> So I want to withdraw my claim that NPEs are thrown frequently.
> >> Probably I was biased by my compiler construction background,
> >> remembering NPE checks are all over the place in the code.
> >>
> >> But I think I can still keep the claim that the message is
> >> printed rarely.
> >>
> >> I'll adapt the JEP saying something like this:
> >> "While exceptions are supposed to be thrown rarely, i.e., only
> >> In exceptional situations, most are swallowed without ever
> >> looking at the message. Thus, overhead in getMessage() will
> >> not fall into account."
> >
> > Is this really a realistic assumption? That NPE exceptions are mostly
> > swallowed in most programs despite the fact that swallowing exceptions
> > (and throwing them to control the flow) is an anti-pattern? Is
> > majority of code really so badly written? I would expect that most
> > programs contain an exception handler of some kind that at least logs
> > all unexpected exceptions.
> >
> > I think JDK should assume that NPEs are not frequent in most well
> > written programs. Because in well written programs all unexpected
> > exceptions are at least logged somewhere and this alone guarantees
> > that programs are eventually "fixed" to not throw them frequently...
> >
> > Regards, Peter
> 
> So I would say that there are two kinds of programs (which kind is in
> majority doesn't matter):
> 
> a - programs that throw and catch exceptions for exceptional situations
> only (i.e. non frequently) - they also print the exceptions' messages
> b - programs that throw and swallow exceptions frequently, but they
> mostly don't print their messages
> 
> In either case .getMessage() is not called frequently for kind (a) and
> hopefully also for kind (b).
> 
> Regards, Peter
Hi,

I agree with this, and my numbers show
that the message is not printed frequently in any case.

Best regards,
  Goetz.

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Peter Levart]

On 3/29/19 4:36 PM, Peter Levart wrote:



On 3/29/19 8:49 AM, Lindenmaier, Goetz wrote:

So I want to withdraw my claim that NPEs are thrown frequently.
Probably I was biased by my compiler construction background,
remembering NPE checks are all over the place in the code.

But I think I can still keep the claim that the message is
printed rarely.

I'll adapt the JEP saying something like this:
"While exceptions are supposed to be thrown rarely, i.e., only
In exceptional situations, most are swallowed without ever
looking at the message. Thus, overhead in getMessage() will
not fall into account."


Is this really a realistic assumption? That NPE exceptions are mostly 
swallowed in most programs despite the fact that swallowing exceptions 
(and throwing them to control the flow) is an anti-pattern? Is 
majority of code really so badly written? I would expect that most 
programs contain an exception handler of some kind that at least logs 
all unexpected exceptions.


I think JDK should assume that NPEs are not frequent in most well 
written programs. Because in well written programs all unexpected 
exceptions are at least logged somewhere and this alone guarantees 
that programs are eventually "fixed" to not throw them frequently...


Regards, Peter


So I would say that there are two kinds of programs (which kind is in 
majority doesn't matter):


a - programs that throw and catch exceptions for exceptional situations 
only (i.e. non frequently) - they also print the exceptions' messages
b - programs that throw and swallow exceptions frequently, but they 
mostly don't print their messages


In either case .getMessage() is not called frequently for kind (a) and 
hopefully also for kind (b).


Regards, Peter

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Peter Levart]

On 3/29/19 8:49 AM, Lindenmaier, Goetz wrote:

So I want to withdraw my claim that NPEs are thrown frequently.
Probably I was biased by my compiler construction background,
remembering NPE checks are all over the place in the code.

But I think I can still keep the claim that the message is
printed rarely.

I'll adapt the JEP saying something like this:
"While exceptions are supposed to be thrown rarely, i.e., only
In exceptional situations, most are swallowed without ever
looking at the message. Thus, overhead in getMessage() will
not fall into account."


Is this really a realistic assumption? That NPE exceptions are mostly 
swallowed in most programs despite the fact that swallowing exceptions 
(and throwing them to control the flow) is an anti-pattern? Is majority 
of code really so badly written? I would expect that most programs 
contain an exception handler of some kind that at least logs all 
unexpected exceptions.


I think JDK should assume that NPEs are not frequent in most well 
written programs. Because in well written programs all unexpected 
exceptions are at least logged somewhere and this alone guarantees that 
programs are eventually "fixed" to not throw them frequently...


Regards, Peter



Best regards,
   Goetz.

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy,

I could access the JBS now, so part 2 of my reply.

Yesterday, I added tables to the JEP describing what is printed
for which bytecode.   If this is too verbose for the JEP, it might
be nice in the bug implementing it.

> > This is a design decision not to make the printout too complex.
> This is fine.   The JEP should call this out.
I added this to the table entry for invokes.

> > The algorithm looks at a lot of bytecodes to print the method. I can not 
> > enumerate all combinations of bytecodes, it's millions. I will add a 
> > per-bytecode table to 
> > the "Message content" section describing what is printed for which bytecode.
> This is not what I am suggesting to do.
> The JEP can describe the known cases or pattern (if enumerating these cases 
> is not possible) 
> to help the readers understand the proposed feature and what is not covered.
I added the tables to describe this before I got your mail, see above. 

Best regards,
  Goetz.

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy,

The JBS is offline, so I'll just reply on this item for now:

> > > Since the JEP quotes that NullPointerExceptions are thrown frequently
> > > and swallowed, it would help if you can generate some numbers.
> > > This JEP will help the discussion on the proposed feature and design and 
> > > avoid
> > > any confusion/assumption of the implementation.
> > I'll generate some numbers.  Is running jvm2008 fine?
> A benchmark may not be a good representative.  I'd be interested in a real 
> application
> to support the claim.  

I ran the following applications:
  Jbb2005: 15 NPE raised
  Jvm2008: 5 NPE raised, only in 'check' benchmark
  SAP application: 16138 NPE raised.
None of these called NPE.getMessage().

This SAP application is known to us for bad NPE behavior 
and runs a few hours. It's from our GC stress tests.
I think 16000 is still a small number given the runtime of
the test.

So I want to withdraw my claim that NPEs are thrown frequently.
Probably I was biased by my compiler construction background,
remembering NPE checks are all over the place in the code.

But I think I can still keep the claim that the message is
printed rarely.

I'll adapt the JEP saying something like this:
"While exceptions are supposed to be thrown rarely, i.e., only
In exceptional situations, most are swallowed without ever
looking at the message. Thus, overhead in getMessage() will
not fall into account."

Best regards,
  Goetz.

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

On 3/27/19 7:18 AM, Lindenmaier, Goetz wrote:

, is this example very clear to them that it won't be supported?


You probably meant that for
   n().getNull().m()
it is not printed that getNull() was called on the result of n()?


Yes, I caught my error after I sent my example.

This is a design decision not to make the printout too complex.
  


This is fine.   The JEP should call this out.



The algorithm looks at a lot of bytecodes to print the method. I can not
enumerate all combinations of bytecodes, it's millions. I will add a 
per-bytecode table to
the "Message content" section describing what is printed for which bytecode.


This is not what I am suggesting to do.

The JEP can describe the known cases or pattern (if enumerating these 
cases is not possible) to help the readers understand the proposed 
feature and what is not covered.

This paragraph lists a possible problem -- multiple variants of methods
with the same name and class name -- and how this is solved.
I added a section "Cases not covered by this JEP" and mention it there, too.


Sounds good.

Since the JEP quotes that NullPointerExceptions are thrown frequently
and swallowed, it would help if you can generate some numbers.
This JEP will help the discussion on the proposed feature and design and avoid
any
confusion/assumption of the implementation.

I'll generate some numbers.  Is running jvm2008 fine?


A benchmark may not be a good representative.  I'd be interested in a 
real application

to support the claim.

OTOH, we are in agreement that this change should not incur any 
significant overhead to the construction cost of throwable.  I asked the 
data because of the claim of many NPE thrown get swallowed.



Mandy

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy,

>and also the cases when it requires to look at its caller 
> frame.
> 
>   Never.  Only the method that was executed when the exception is
>   thrown is needed. I only mention the backtrace because it happens to
>   store the top method and the corresponding bytecode position in the
>   top method.
> 
> 
> That explains why I don't see an example of:
>  getNull().m();
> 
>  public void m() {};
> 
>  static NPE getNull() {
>  return null;
>  }
> 
> For a compiler expert or one who study the data flow analysis paper
Yes, I think compiler people should understand this.

> , is this example very clear to them that it won't be supported?
The example for your code is supported, see[1], examples 11-14. (I should 
enumerate them)
For your code, it will print
"The return value of 'MyClass.getNULL()LNPE;' is null. Can not invoke method 
'MyOtherClass.m()V'".

getNull().m() are not nested calls, but subsequent ones. Therefore you will not 
find
them on the call stack.
The byte code index of the invoke bytecode invoking m() is known from the 
backtrace or
StackWalker/StackFrame.  Looking at the bytecode the part "Can not invoke 
method 'MyOtherClass.m()V"
is printed.
With the result of the data flow analysis it is possible to find the bytecode 
that produced
the reference on which m() is invoked.  This is another invoke bytecode in the 
method.
Looking at this other bytecode, " The return value of 'MyClass.getNULL()LNPE;' 
is null." 
is printed.

You probably meant that for 
  n().getNull().m()
it is not printed that getNull() was called on the result of n()?
This is a design decision not to make the printout too complex.
 
> This is related to my comment w.r.t. the data flow analysis comes from.  I was
> looking for some level of information be spelled out in the JEP that can
> sufficiently give a non-compiler expert an idea but of course no point to
> replicate the paper here.
"The simulated stack" ... contains ..."information about which bytecode pushed 
the value to the stack."
That's all.

> It's good to know that this proposal needs the top frame only.   Please make 
> it
> clear in the JEP.   
I tried to point out more precisely why and when I need the top frame. I 
pushed this down a bit in the text in "basic algorithm".

> The JEP can take out `StackWalker` but instead talks about
> `StackFrame` since doing it in Java means that it only needs to get back a 
> `StackFrame`
> instance representing the top frame stored in `Throwable::backtrace`
Yes. I tried to make the text more precise.

> This will avoid the confusion on calling StackWalker in the NPE constructor 
> and
> constructing many StackFrame and many ResolvedMethodName.  That was
> part of the code review discussion.
Yes, only the top StackFrame is needed.

>   "Computing either part of these messages can fail due to
> insufficient
>   information."
>   What are the cases that it fails to obtain the information? It'd
> be useful
>   to list some examples if not all.
> 
>   (a ? b : c).d
>   You can not know whether the ?-expression yielded b or c, thus
>   you don't know why the access .d failed.  There is a corresponding
> example
>   in [1].
> 
> 
> Please describe in the JEP the known cases that this proposal doesn't cover.
> Conditional expression is one and 

The algorithm looks at a lot of bytecodes to print the method. I can not 
enumerate all combinations of bytecodes, it's millions. I will add a 
per-bytecode table to 
the "Message content" section describing what is printed for which bytecode.

>null receiver if it's the return value from a caller frame (I think this would 
>help the readers).
... this is supported, see above.

>   The "Different bytecode variants of a method" section
> 
>   This section raises the case when a method representing an
> execution stack
>   frame throwing NPE becomes obsolete, it does not have the
> information
>   to compute the message lazily.  So this falls into the
> "insufficient
>   information" category and no improved NPE message can be
> generated.
> 
>   Yes, note this also only addresses the method in which the exception
> was raised.
> 
> This should also go to the list of known cases that this proposal does not 
> cover.

This paragraph lists a possible problem -- multiple variants of methods
with the same name and class name -- and how this is solved. 
I added a section "Cases not covered by this JEP" and mention it there, too.

> Since the JEP quotes that NullPointerExceptions are thrown frequently
> and swallowed, it would help if you can generate some numbers.
> This JEP will help the discussion on the proposed feature and design and avoid
> any
> confusion/assumption of the implementation.
I'll generate some numbers.  Is running jvm2008 fine?

> I think this is related to this JEP.The question 

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

On 3/26/19 4:57 AM, Lindenmaier, Goetz wrote:

  and also the cases when it requires to look at its caller frame.

Never.  Only the method that was executed when the exception is
thrown is needed. I only mention the backtrace because it happens to
store the top method and the corresponding bytecode position in the
top method.


That explains why I don't see an example of:
 getNull().m();

 public void m() {};

 static NPE getNull() {
 return null;
 }

For a compiler expert or one who study the data flow analysis paper, is 
this example very clear to them that it won't be supported?


This is related to my comment w.r.t. the data flow analysis comes from.  
I was looking for some level of information be spelled out in the JEP 
that can sufficiently give a non-compiler expert an idea but of course 
no point to replicate the paper here.


It's good to know that this proposal needs the top frame only. Please 
make it clear in
the JEP.    The JEP can take out `StackWalker` but instead talks about 
`StackFrame`
since doing it in Java means that it only needs to get back a 
`StackFrame` instance

representing the top frame stored in `Throwable::backtrace`

This will avoid the confusion on calling StackWalker in the NPE 
constructor and

constructing many StackFrame and many ResolvedMethodName.  That was part
of the code review discussion.
  

"Computing either part of these messages can fail due to insufficient
information."
What are the cases that it fails to obtain the information? It'd be useful
to list some examples if not all.

(a ? b : c).d
You can not know whether the ?-expression yielded b or c, thus
you don't know why the access .d failed.  There is a corresponding example
in [1].


Please describe in the JEP the known cases that this proposal doesn't 
cover.   Conditional expression is one and null receiver if it's the 
return value from a caller frame (I think this would help the readers).

The "Different bytecode variants of a method" section

This section raises the case when a method representing an execution stack
frame throwing NPE becomes obsolete, it does not have the information
to compute the message lazily.  So this falls into the "insufficient
information" category and no improved NPE message can be generated.

Yes, note this also only addresses the method in which the exception was raised.


This should also go to the list of known cases that this proposal does 
not cover.

The "computation overhead" section

I agree with the requirement to have minimal performance overhead to
the NPE construction cost.

"NullPointerExceptions are thrown frequently."
"Fortunately, most Exceptions are discarded without looking at the message."

This is interesting.  Do you have any statistic on the number of NPE thrown
and swallowed on certain application/environment that you have gathered?

No, I can try to generate some numbers, though.  But this assumption was made by
several others that commented on the previous review thread. For Throwable
in general, it is also the assumption why the intermediate backtrace data
structure is implemented instead of generating the stackFrames right away.


Since the JEP quotes that NullPointerExceptions are thrown frequently
and swallowed, it would help if you can generate some numbers.

This JEP will help the discussion on the proposed feature and design and 
avoid any

confusion/assumption of the implementation.


"If the message is printed based on the backtrace, it must be omitted
  if the top frame was dropped from the backtrace."

If NPE includes the hidden frames, `Throwable::getStackTrace` and
`Throwable::toString` and the VM printing of NPE stack trace needs to
filter the hidden frames.  I think it's appropriate for this JEP to
cover rather than in a separate JBS issue.

Hmm, I don't completely understand what you want to say, please
excuse, I'm a foreign speaker :). I'll give a comprehensive answer:
To my understanding this is a consequence of the existing JVM/class file
implementation that calls methods that should be hidden to the user.
The implementation drops these frames when the backtrace datastructure
is computed. If this happens, the information needed to compute the
message is lost, and it can not be printed.


Yes I am familiar with that.

The fact that this happened must be remembered in the VM
so that printing the message can be suppressed. Else a message for
a completely wrong bytecode is printed.


Yup.

I think the JEP should mention that this is happening.  How this is
implemented and then pushed to the repo is not subject to a JEP,
is it?


I think this is related to this JEP.    The question would be:
- should it include all hidden frames?
- should it include the top frame if it's a hidden frame since this 
feature only requires the top frame?


This JEP requires the hidden frame to be recorded in the backtrace. This 
has

impact to the printing of stack trace that needs to know about hidden frames
and decide if it 

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy, 

thanks for reading the JEP and giving detailed feedback!

> In the "Basic algorithm to compute the message" section:
> 
> "This dataflow analysis walks the bytecodes forward simulating the execution
> stack."
> 
> Please elaborate it.  Given a stack trace, it starts with the most recently
> executed method and bytecode index, I assume it simulates the method from
> the beginning of the method until it hits the instruction with the matching 
> BCI.
> Please describe it
A data flow analysis is a fixpoint iteration on the code. It is a  
basic concept of compiler construction and used in lots of places in 
the hotspot code. I don't think I should further give an explanation
of this. Maybe I could point to Wikipedia? 
https://en.wikipedia.org/wiki/Data-flow_analysis

>  and also the cases when it requires to look at its caller frame.
Never.  Only the method that was executed when the exception is 
thrown is needed. I only mention the backtrace because it happens to 
store the top method and the corresponding bytecode position in the 
top method.

I tried to make this more clear by changing the formulation of the first
two paragraphs.
 
> "Computing either part of these messages can fail due to insufficient
> information."
> What are the cases that it fails to obtain the information? It'd be useful
> to list some examples if not all.
(a ? b : c).d
You can not know whether the ?-expression yielded b or c, thus
you don't know why the access .d failed.  There is a corresponding example 
in [1]. 

> 
> The "Different bytecode variants of a method" section
> 
> This section raises the case when a method representing an execution stack
> frame throwing NPE becomes obsolete, it does not have the information
> to compute the message lazily.  So this falls into the "insufficient
> information" category and no improved NPE message can be generated.
Yes, note this also only addresses the method in which the exception was raised.

> The "computation overhead" section
> 
> I agree with the requirement to have minimal performance overhead to
> the NPE construction cost.
> 
> "NullPointerExceptions are thrown frequently."
> "Fortunately, most Exceptions are discarded without looking at the message."
> 
> This is interesting.  Do you have any statistic on the number of NPE thrown
> and swallowed on certain application/environment that you have gathered?
No, I can try to generate some numbers, though.  But this assumption was made 
by 
several others that commented on the previous review thread. For Throwable
in general, it is also the assumption why the intermediate backtrace data
structure is implemented instead of generating the stackFrames right away.

> "If the message is printed based on the backtrace, it must be omitted
>  if the top frame was dropped from the backtrace."
> 
> If NPE includes the hidden frames, `Throwable::getStackTrace` and
> `Throwable::toString` and the VM printing of NPE stack trace needs to
> filter the hidden frames.  I think it's appropriate for this JEP to
> cover rather than in a separate JBS issue.
Hmm, I don't completely understand what you want to say, please
excuse, I'm a foreign speaker :). I'll give a comprehensive answer:
To my understanding this is a consequence of the existing JVM/class file
implementation that calls methods that should be hidden to the user.
The implementation drops these frames when the backtrace datastructure
is computed. If this happens, the information needed to compute the 
message is lost, and it can not be printed.
The fact that this happened must be remembered in the VM
so that printing the message can be suppressed. Else a message for
a completely wrong bytecode is printed.
I think the JEP should mention that this is happening.  How this is 
implemented and then pushed to the repo is not subject to a JEP,
is it?
I made a webrev of its own for this problem, so that the code
can be looked at isolated of other, orthogonal issues.
http://cr.openjdk.java.net/~goetz/wr19/8221077-hidden_to_frame/

> The "Message persistance" section
> 
> I read this section like an implementation details
 Yes, I had the feeling that I was talking too much about implementation 
details, 
but mostly in the "Basic algorithm" section. 
This section discusses characteristics of NPE that differ from other exceptions
and that were proposed in the course of the review of my 
original webrev. They are visible to the Java implementor. 
So I think this is just the basic information needed in a JEP.

> I think this section
> intends to call out that the message of NPE if constructed by user code
> i.e. via public constructor, should not be altered including no-arg
> constructor or pass a null message to 1-arg constructor.  The enhanced
> NPE message is proposed only when NPE is thrown due to null dereference
> when executing bytecode.
Yes.
 
> I don't understand what you tried to say about "npe.getMessage() ==
> npe.getMessage()".
> Throwable::getMessage does not guarantee to return 

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

Hi Goetz,

On 3/15/19 3:55 AM, Lindenmaier, Goetz wrote:

I followed your advice and created a JEP:
https://bugs.openjdk.java.net/browse/JDK-8220715


This is a good start.   I include my comments as a reader who does not
read TrackingStackCreator and other C++ code.

In the "Basic algorithm to compute the message" section:

"This dataflow analysis walks the bytecodes forward simulating the 
execution stack."


Please elaborate it.  Given a stack trace, it starts with the most recently
executed method and bytecode index, I assume it simulates the method from
the beginning of the method until it hits the instruction with the 
matching BCI.
Please describe it and also the cases when it requires to look at its 
caller frame.


"Computing either part of these messages can fail due to insufficient 
information."


What are the cases that it fails to obtain the information? It'd be useful
to list some examples if not all.

The "Different bytecode variants of a method" section

This section raises the case when a method representing an execution stack
frame throwing NPE becomes obsolete, it does not have the information
to compute the message lazily.  So this falls into the "insufficient
information" category and no improved NPE message can be generated.

The "computation overhead" section

I agree with the requirement to have minimal performance overhead to
the NPE construction cost.

"NullPointerExceptions are thrown frequently."
"Fortunately, most Exceptions are discarded without looking at the message."

This is interesting.  Do you have any statistic on the number of NPE thrown
and swallowed on certain application/environment that you have gathered?

"If the message is printed based on the backtrace, it must be omitted
 if the top frame was dropped from the backtrace."

If NPE includes the hidden frames, `Throwable::getStackTrace` and
`Throwable::toString` and the VM printing of NPE stack trace needs to
filter the hidden frames.  I think it's appropriate for this JEP to
cover rather than in a separate JBS issue.

The "Message persistance" section

I read this section like an implementation details.  I think this section
intends to call out that the message of NPE if constructed by user code
i.e. via public constructor, should not be altered including no-arg
constructor or pass a null message to 1-arg constructor.  The enhanced
NPE message is proposed only when NPE is thrown due to null dereference
when executing bytecode.

I don't understand what you tried to say about "npe.getMessage() == 
npe.getMessage()".

Throwable::getMessage does not guarantee to return same String object for
each invocation.

When deserializing the class bytes may be of a different version, so 
StackTraceElements
are serialized with its string form.  Serializing NPE with a computed 
message

seems to be the only viable solution.

The "Message content" section

I think this section can break down into clear scenarios
1. debug information is present
2. debug information is absent
3. the matching version of bytecode is not available

I agree that the message should be easy for Java developers including the
beginners to understand the error.

You bring up a good question about the exception message format e.g. 
whether with
quotation (none vs single quote vs double quote).  It's good to have a 
guideline

on that while this is out of scope of this JEP.  IAE was updated to include
module name, loader name and improve the error message for the developers
to understand the exception which is a good improvement.

"Alternatives" section

"The current proposal is to implement this in the Java runtime in C++ 
accessing

 directly the available datastructures in the metaspace."

"Also, ASM and StackWalker do not support the full functionality needed.
StackWalker must be called in the NullPointerException constructor."

This is not true as you wrote in the subsequent sentence.  If we choose
the implementation to Java, StackWalker will need to be extended to
read Throwable's backtrace.  Such specialized form can provide the
ability to fetch the bytecode in Method* if appropriate.  This is
the implementation details.  As discussed in the email thread, another
implementation choice could be a hybrid of native in the VM and Java
to accomplish this task (for example fetching the bytecode of the
current version could be done in the VM if we agree supporting
the redefinition case).

For logic that is not strictly needed to be done in the VM, doing it
in Java and library is a good option to consider (putting aside the
amount of new code you have to write).   Looks like you can't evaluate
the alternatives since existing library code requires work in order to
prototype this in Java.  So I'd say more investigation needs to be done
to decide on alternative implementation approaches.

"Testing" section

"The basic implementation is in use in SAP's internal Java virtual 
machine since 2006."


This is good information.  This assumes if the same 

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

On 3/20/19 1:54 AM, Lindenmaier, Goetz wrote:

Should I move the JEP to status 'submitted'?


Per the Process states section  [1]

Draft --- In circulation by the author for initial review and 
consensus-building


I certainly don't want to be the bottleneck.   Others can help do the 
initial review.


Mandy
[1] http://openjdk.java.net/jeps/1#Process-states

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy,

> Sorry I have been busy on other time-critical things.  I will
> get to it hopefully next week.
No problem, I have been improving the text slightly
anyways.
I also pushed my change and a follow up needed to the sandbox 
repos so it can be played with.

Should I move the JEP to status 'submitted'?

Best regards,
  Goetz




> 
> Mandy
> 
> On 3/15/19 3:55 AM, Lindenmaier, Goetz wrote:
> > Hi everybody, Mark,
> >
> > I followed your advice and created a JEP:
> > https://bugs.openjdk.java.net/browse/JDK-8220715
> >
> > Please point me to things I need to improve formally, this is my first
> > JEP. Also feel free to fix the administrative information in the
> > Jira issue if it is wrong.
> >
> > And, naturally, you're welcome to discuss the topic!
> >
> > Best regards,
> >Goetz.
> >
> >> -Original Message-
> >> From: mark.reinh...@oracle.com 
> >> Sent: Donnerstag, 14. März 2019 22:38
> >> To: maurizio.cimadam...@oracle.com; Lindenmaier, Goetz
> >> 
> >> Cc: mandy.ch...@oracle.com; roger.ri...@oracle.com; core-libs-
> >> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
> >> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> >> describing what is null.
> >>
> >> 2019/3/14 8:00:20 -0700, maurizio.cimadam...@oracle.com:
> >>> I second what Mandy says.
> >>>
> >>> First let me start by saying that this enhancement will be a great
> >>> addition to our platform; back in the days when I was teaching some Java
> >>> classes at the university, I was very aware of how hard it is to
> >>> diagnose a NPE for someone novel to Java programming.
> >>
> >> Agreed!
> >>
> >>> ...
> >>>
> >>> I also think that the design space for such an enhancement is non
> >>> trivial, and would best be explored (and captured!) in a medium that is
> >>> something other than a patch. ...
> >>
> >> Agreed, also.
> >>
> >> Goetz -- if, per Mandy’s suggestion, you’re going to write something
> >> up using the JEP template, might I suggest that you then submit it as
> >> an actual JEP?  Giving visibility to, and recording, such design-space
> >> explorations is one of the primary goals of the JEP process.
> >>
> >> - Mark

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

Hi Goetz,

Sorry I have been busy on other time-critical things.  I will
get to it hopefully next week.

Mandy

On 3/15/19 3:55 AM, Lindenmaier, Goetz wrote:

Hi everybody, Mark,

I followed your advice and created a JEP:
https://bugs.openjdk.java.net/browse/JDK-8220715

Please point me to things I need to improve formally, this is my first
JEP. Also feel free to fix the administrative information in the
Jira issue if it is wrong.

And, naturally, you're welcome to discuss the topic!

Best regards,
   Goetz.


-Original Message-
From: mark.reinh...@oracle.com 
Sent: Donnerstag, 14. März 2019 22:38
To: maurizio.cimadam...@oracle.com; Lindenmaier, Goetz

Cc: mandy.ch...@oracle.com; roger.ri...@oracle.com; core-libs-
d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

2019/3/14 8:00:20 -0700, maurizio.cimadam...@oracle.com:

I second what Mandy says.

First let me start by saying that this enhancement will be a great
addition to our platform; back in the days when I was teaching some Java
classes at the university, I was very aware of how hard it is to
diagnose a NPE for someone novel to Java programming.


Agreed!


...

I also think that the design space for such an enhancement is non
trivial, and would best be explored (and captured!) in a medium that is
something other than a patch. ...


Agreed, also.

Goetz -- if, per Mandy’s suggestion, you’re going to write something
up using the JEP template, might I suggest that you then submit it as
an actual JEP?  Giving visibility to, and recording, such design-space
explorations is one of the primary goals of the JEP process.

- Mark

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy,

> ... This brings up one
> issue that when constructing the enhanced NPE message, a method
> might become obsolete due to redefintion, it should handle
> gracefully.
The C++ solution has access to the proper version of redefined codes,
so this is no problem.  It is attached to the backtrace.
And yes, in the Java variant we could just skip the message if we don't 
have the proper variant of the bytecode at hand. But we need 
a way to find out that this is the case.  It's unclear to me how this 
should work ... it also depends on the way we retrieve the bytecodes
to analyze.

Best regards,
  Goetz.



> Mandy
> 
> On 3/15/19 10:34 AM, Maurizio Cimadamore wrote:
> > You touch an important point here - who is this feature for? Is it for
> > the casual developer in need of some hand holding? Or is it for
> > diagnosing complex instrumented stacks? I think the two use cases are
> > not necessarily similar and might be served by different sets of
> > enhancements. In the latter case it might be ok, for instance, to say
> > "and, btw, the NPE came from local[1]". But for users in the former
> > bucket (and most users, really), this level of detail will simply be
> > unacceptable (since now they have to understand the JVMS to parse the
> > message :-)).
> >
> > I suggest we separate the use cases, at least for the purpose of the
> > design discussion.
> >
> > Maurizio
> >
> > On 15/03/2019 13:11, Lindenmaier, Goetz wrote:
> >> Hi Maurizio,
> >>
> >> thanks for sharing your patch!  This is about what I thought about
> >> so far, just that it's working already :)
> >>
> >> It prints the information of the failing bytecode.  Adding the dataflow
> >> analysis, the other information could be printed as well.
> >>
> >> The major problem I see is here:
> >> +    File f = new File(location.getFile(),
> >> +    clazz.getCanonicalName().replaceAll("\\.", "/") +
> >> ".class");
> >> +    ClassReader cr = new ClassReader(new FileInputStream(f));
> >> +    ClassNode classNode = new ClassNode();
> >> +    cr.accept(classNode, 0);
> >>
> >> As I understand, this reads the Bytecode from the classfile.
> >> The bytecode in the classfile must not match that executed
> >> by the VM, i.e, the pc you get from the stackWalker will not
> >> reference the bytecode that caused the NPE.
> >>
> >> Other issues have been discussed in the mail thread before.
> >> Good that you point this out, I'll add it to the JEP.
> >>
> >> Best regards,
> >>    Goetz
> >>
> >> ... Sorry, I missed the "reply all" on my first reply.
> >>
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: Maurizio Cimadamore 
> >>> Sent: Freitag, 15. März 2019 12:33
> >>> To: Lindenmaier, Goetz ; Mandy Chung
> >>> ; Roger Riggs 
> >>> Cc: Java Core Libs ; hotspot-runtime-
> >>> d...@openjdk.java.net
> >>> Subject: Re: RFR(L): 8218628: Add detailed message to
> >>> NullPointerException
> >>> describing what is null.
> >>>
> >>> Hi Goetz,
> >>> please find the attached ASM-based patch. It is just a PoC, as such it
> >>> does not provide as fine-grained messages as the one discussed in the
> >>> RFE/JEP, but can be enhanced to cover custom debugging attribute, I
> >>> believe.
> >>>
> >>> When running this:
> >>>
> >>> Object o = null;
> >>> o.toString();
> >>>
> >>> you get:
> >>>
> >>> Exception in thread "main" java.lang.NullPointerException: attempt to
> >>> dereference 'null' when calling method 'toString'
> >>>       at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)
> >>>
> >>> While when running this:
> >>>
> >>> Foo foo = null;
> >>> int y = foo.x;
> >>>
> >>> You get this:
> >>>
> >>> Exception in thread "main" java.lang.NullPointerException: attempt to
> >>> dereference 'null' when accessing field 'x'
> >>>       at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)
> >>>
> >>> One problem I had is that ASM provides no way to get the instruction
> >>> given a program counter - which means we have to scan all the bytecodes
> >>> an

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

> You touch an important point here - who is this feature for? Is it for
> the casual developer in need of some hand holding? Or is it for
> diagnosing complex instrumented stacks? I think the two use cases are
> not necessarily similar and might be served by different sets of
> enhancements.
Do you propose to have two different messages, to be selected by some 
flag or the like?
To my knowledge exceptions have only one message, and it should
be as helpful as possible I think.

>  In the latter case it might be ok, for instance, to say
> "and, btw, the NPE came from local[1]". But for users in the former
> bucket (and most users, really), this level of detail will simply be
> unacceptable 

>(since now they have to understand the JVMS to parse the
> message :-)).
Can you please elaborate why they have to understand the JVMS?

And why would information you don't need be unacceptable?
You can just ignore it ... 

Best regards,
  Goetz.
 
> I suggest we separate the use cases, at least for the purpose of the
> design discussion.
> 
> Maurizio
> 
> On 15/03/2019 13:11, Lindenmaier, Goetz wrote:
> > Hi Maurizio,
> >
> > thanks for sharing your patch!  This is about what I thought about
> > so far, just that it's working already :)
> >
> > It prints the information of the failing bytecode.  Adding the dataflow
> > analysis, the other information could be printed as well.
> >
> > The major problem I see is here:
> > +File f = new File(location.getFile(),
> > +clazz.getCanonicalName().replaceAll("\\.", "/") + 
> > ".class");
> > +ClassReader cr = new ClassReader(new FileInputStream(f));
> > +ClassNode classNode = new ClassNode();
> > +cr.accept(classNode, 0);
> >
> > As I understand, this reads the Bytecode from the classfile.
> > The bytecode in the classfile must not match that executed
> > by the VM, i.e, the pc you get from the stackWalker will not
> > reference the bytecode that caused the NPE.
> >
> > Other issues have been discussed in the mail thread before.
> > Good that you point this out, I'll add it to the JEP.
> >
> > Best regards,
> >Goetz
> >
> > ... Sorry, I missed the "reply all" on my first reply.
> >
> >
> >
> >
> >> -Original Message-----
> >> From: Maurizio Cimadamore 
> >> Sent: Freitag, 15. März 2019 12:33
> >> To: Lindenmaier, Goetz ; Mandy Chung
> >> ; Roger Riggs 
> >> Cc: Java Core Libs ; hotspot-runtime-
> >> d...@openjdk.java.net
> >> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> >> describing what is null.
> >>
> >> Hi Goetz,
> >> please find the attached ASM-based patch. It is just a PoC, as such it
> >> does not provide as fine-grained messages as the one discussed in the
> >> RFE/JEP, but can be enhanced to cover custom debugging attribute, I
> believe.
> >>
> >> When running this:
> >>
> >> Object o = null;
> >> o.toString();
> >>
> >> you get:
> >>
> >> Exception in thread "main" java.lang.NullPointerException: attempt to
> >> dereference 'null' when calling method 'toString'
> >>       at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)
> >>
> >> While when running this:
> >>
> >> Foo foo = null;
> >> int y = foo.x;
> >>
> >> You get this:
> >>
> >> Exception in thread "main" java.lang.NullPointerException: attempt to
> >> dereference 'null' when accessing field 'x'
> >>       at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)
> >>
> >> One problem I had is that ASM provides no way to get the instruction
> >> given a program counter - which means we have to scan all the bytecodes
> >> and update the sizes as we go along, and, ASM unfortunately doesn’t
> >> expose opcode sizes either. A more robust solution would be to have a
> >> big switch which returned the opcode size of any given opcode. Also,
> >> accessing to StackWalker API on exception creation might not be
> >> desirable in terms of performances, so this might be one of these area
> >> where some VM help could be beneficial. Another problem is that we
> >> cannot distinguish between user-generated exceptions (e.g. `throw new
> >> NullPointerException`) and genuine NPE issued by the VM.
> >>
> >> But I guess the upshot is that it works to leave all the gory detail of
> >> bytecode grovelling to a bytecode API - if the logic is applied lazily,
> >> then the impact on performances should be minimal, and the solution more
> >> maintainable longer term.
> >>
> >> Cheers
> >> Maurizio
> >>
> >> On 15/03/2019 07:59, Lindenmaier, Goetz wrote:
> >>> Yes, it would be nice if you shared that.

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

That's a good suggestion.  The JDK out-of-the-box case and the
complex environment case with instrumentation agents transforming
the bytecodes are two different use cases.  This brings up one
issue that when constructing the enhanced NPE message, a method
might become obsolete due to redefintion, it should handle
gracefully.

Mandy

On 3/15/19 10:34 AM, Maurizio Cimadamore wrote:
You touch an important point here - who is this feature for? Is it for 
the casual developer in need of some hand holding? Or is it for 
diagnosing complex instrumented stacks? I think the two use cases are 
not necessarily similar and might be served by different sets of 
enhancements. In the latter case it might be ok, for instance, to say 
"and, btw, the NPE came from local[1]". But for users in the former 
bucket (and most users, really), this level of detail will simply be 
unacceptable (since now they have to understand the JVMS to parse the 
message :-)).


I suggest we separate the use cases, at least for the purpose of the 
design discussion.


Maurizio

On 15/03/2019 13:11, Lindenmaier, Goetz wrote:

Hi Maurizio,

thanks for sharing your patch!  This is about what I thought about
so far, just that it's working already :)

It prints the information of the failing bytecode.  Adding the dataflow
analysis, the other information could be printed as well.

The major problem I see is here:
+    File f = new File(location.getFile(),
+    clazz.getCanonicalName().replaceAll("\\.", "/") + 
".class");

+    ClassReader cr = new ClassReader(new FileInputStream(f));
+    ClassNode classNode = new ClassNode();
+    cr.accept(classNode, 0);

As I understand, this reads the Bytecode from the classfile.
The bytecode in the classfile must not match that executed
by the VM, i.e, the pc you get from the stackWalker will not
reference the bytecode that caused the NPE.

Other issues have been discussed in the mail thread before.
Good that you point this out, I'll add it to the JEP.

Best regards,
   Goetz

... Sorry, I missed the "reply all" on my first reply.





-Original Message-
From: Maurizio Cimadamore 
Sent: Freitag, 15. März 2019 12:33
To: Lindenmaier, Goetz ; Mandy Chung
; Roger Riggs 
Cc: Java Core Libs ; hotspot-runtime-
d...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to 
NullPointerException

describing what is null.

Hi Goetz,
please find the attached ASM-based patch. It is just a PoC, as such it
does not provide as fine-grained messages as the one discussed in the
RFE/JEP, but can be enhanced to cover custom debugging attribute, I 
believe.


When running this:

Object o = null;
o.toString();

you get:

Exception in thread "main" java.lang.NullPointerException: attempt to
dereference 'null' when calling method 'toString'
      at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)

While when running this:

Foo foo = null;
int y = foo.x;

You get this:

Exception in thread "main" java.lang.NullPointerException: attempt to
dereference 'null' when accessing field 'x'
      at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)

One problem I had is that ASM provides no way to get the instruction
given a program counter - which means we have to scan all the bytecodes
and update the sizes as we go along, and, ASM unfortunately doesn’t
expose opcode sizes either. A more robust solution would be to have a
big switch which returned the opcode size of any given opcode. Also,
accessing to StackWalker API on exception creation might not be
desirable in terms of performances, so this might be one of these area
where some VM help could be beneficial. Another problem is that we
cannot distinguish between user-generated exceptions (e.g. `throw new
NullPointerException`) and genuine NPE issued by the VM.

But I guess the upshot is that it works to leave all the gory detail of
bytecode grovelling to a bytecode API - if the logic is applied lazily,
then the impact on performances should be minimal, and the solution more
maintainable longer term.

Cheers
Maurizio

On 15/03/2019 07:59, Lindenmaier, Goetz wrote:

Yes, it would be nice if you shared that.

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Maurizio Cimadamore]

You touch an important point here - who is this feature for? Is it for 
the casual developer in need of some hand holding? Or is it for 
diagnosing complex instrumented stacks? I think the two use cases are 
not necessarily similar and might be served by different sets of 
enhancements. In the latter case it might be ok, for instance, to say 
"and, btw, the NPE came from local[1]". But for users in the former 
bucket (and most users, really), this level of detail will simply be 
unacceptable (since now they have to understand the JVMS to parse the 
message :-)).


I suggest we separate the use cases, at least for the purpose of the 
design discussion.


Maurizio

On 15/03/2019 13:11, Lindenmaier, Goetz wrote:

Hi Maurizio,

thanks for sharing your patch!  This is about what I thought about
so far, just that it's working already :)

It prints the information of the failing bytecode.  Adding the dataflow
analysis, the other information could be printed as well.

The major problem I see is here:
+File f = new File(location.getFile(),
+clazz.getCanonicalName().replaceAll("\\.", "/") + 
".class");
+ClassReader cr = new ClassReader(new FileInputStream(f));
+ClassNode classNode = new ClassNode();
+cr.accept(classNode, 0);

As I understand, this reads the Bytecode from the classfile.
The bytecode in the classfile must not match that executed
by the VM, i.e, the pc you get from the stackWalker will not
reference the bytecode that caused the NPE.

Other issues have been discussed in the mail thread before.
Good that you point this out, I'll add it to the JEP.

Best regards,
   Goetz

... Sorry, I missed the "reply all" on my first reply.





-Original Message-
From: Maurizio Cimadamore 
Sent: Freitag, 15. März 2019 12:33
To: Lindenmaier, Goetz ; Mandy Chung
; Roger Riggs 
Cc: Java Core Libs ; hotspot-runtime-
d...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

Hi Goetz,
please find the attached ASM-based patch. It is just a PoC, as such it
does not provide as fine-grained messages as the one discussed in the
RFE/JEP, but can be enhanced to cover custom debugging attribute, I believe.

When running this:

Object o = null;
o.toString();

you get:

Exception in thread "main" java.lang.NullPointerException: attempt to
dereference 'null' when calling method 'toString'
      at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)

While when running this:

Foo foo = null;
int y = foo.x;

You get this:

Exception in thread "main" java.lang.NullPointerException: attempt to
dereference 'null' when accessing field 'x'
      at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)

One problem I had is that ASM provides no way to get the instruction
given a program counter - which means we have to scan all the bytecodes
and update the sizes as we go along, and, ASM unfortunately doesn’t
expose opcode sizes either. A more robust solution would be to have a
big switch which returned the opcode size of any given opcode. Also,
accessing to StackWalker API on exception creation might not be
desirable in terms of performances, so this might be one of these area
where some VM help could be beneficial. Another problem is that we
cannot distinguish between user-generated exceptions (e.g. `throw new
NullPointerException`) and genuine NPE issued by the VM.

But I guess the upshot is that it works to leave all the gory detail of
bytecode grovelling to a bytecode API - if the logic is applied lazily,
then the impact on performances should be minimal, and the solution more
maintainable longer term.

Cheers
Maurizio

On 15/03/2019 07:59, Lindenmaier, Goetz wrote:

Yes, it would be nice if you shared that.

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Maurizio,

thanks for sharing your patch!  This is about what I thought about
so far, just that it's working already :)

It prints the information of the failing bytecode.  Adding the dataflow
analysis, the other information could be printed as well.

The major problem I see is here:
+File f = new File(location.getFile(),
+clazz.getCanonicalName().replaceAll("\\.", "/") + 
".class");
+ClassReader cr = new ClassReader(new FileInputStream(f));
+ClassNode classNode = new ClassNode();
+cr.accept(classNode, 0);

As I understand, this reads the Bytecode from the classfile.
The bytecode in the classfile must not match that executed
by the VM, i.e, the pc you get from the stackWalker will not
reference the bytecode that caused the NPE.

Other issues have been discussed in the mail thread before.
Good that you point this out, I'll add it to the JEP.

Best regards,
  Goetz

... Sorry, I missed the "reply all" on my first reply.




> -Original Message-
> From: Maurizio Cimadamore 
> Sent: Freitag, 15. März 2019 12:33
> To: Lindenmaier, Goetz ; Mandy Chung
> ; Roger Riggs 
> Cc: Java Core Libs ; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> please find the attached ASM-based patch. It is just a PoC, as such it
> does not provide as fine-grained messages as the one discussed in the
> RFE/JEP, but can be enhanced to cover custom debugging attribute, I believe.
> 
> When running this:
> 
> Object o = null;
> o.toString();
> 
> you get:
> 
> Exception in thread "main" java.lang.NullPointerException: attempt to
> dereference 'null' when calling method 'toString'
>      at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)
> 
> While when running this:
> 
> Foo foo = null;
> int y = foo.x;
> 
> You get this:
> 
> Exception in thread "main" java.lang.NullPointerException: attempt to
> dereference 'null' when accessing field 'x'
>      at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)
> 
> One problem I had is that ASM provides no way to get the instruction
> given a program counter - which means we have to scan all the bytecodes
> and update the sizes as we go along, and, ASM unfortunately doesn’t
> expose opcode sizes either. A more robust solution would be to have a
> big switch which returned the opcode size of any given opcode. Also,
> accessing to StackWalker API on exception creation might not be
> desirable in terms of performances, so this might be one of these area
> where some VM help could be beneficial. Another problem is that we
> cannot distinguish between user-generated exceptions (e.g. `throw new
> NullPointerException`) and genuine NPE issued by the VM.
> 
> But I guess the upshot is that it works to leave all the gory detail of
> bytecode grovelling to a bytecode API - if the logic is applied lazily,
> then the impact on performances should be minimal, and the solution more
> maintainable longer term.
> 
> Cheers
> Maurizio
> 
> On 15/03/2019 07:59, Lindenmaier, Goetz wrote:
> > Yes, it would be nice if you shared that.

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Maurizio Cimadamore]

Hi Goetz,
please find the attached ASM-based patch. It is just a PoC, as such it 
does not provide as fine-grained messages as the one discussed in the 
RFE/JEP, but can be enhanced to cover custom debugging attribute, I believe.


When running this:

Object o = null;
o.toString();

you get:

Exception in thread "main" java.lang.NullPointerException: attempt to 
dereference 'null' when calling method 'toString'

    at org.oracle.npe.NPEHandler.main(NPEHandler.java:103)

While when running this:

Foo foo = null;
int y = foo.x;

You get this:

Exception in thread "main" java.lang.NullPointerException: attempt to 
dereference 'null' when accessing field 'x'

    at org.oracle.npe.NPEHandler.main(NPEHandler.java:105)

One problem I had is that ASM provides no way to get the instruction 
given a program counter - which means we have to scan all the bytecodes 
and update the sizes as we go along, and, ASM unfortunately doesn’t 
expose opcode sizes either. A more robust solution would be to have a 
big switch which returned the opcode size of any given opcode. Also, 
accessing to StackWalker API on exception creation might not be 
desirable in terms of performances, so this might be one of these area 
where some VM help could be beneficial. Another problem is that we 
cannot distinguish between user-generated exceptions (e.g. `throw new 
NullPointerException`) and genuine NPE issued by the VM.


But I guess the upshot is that it works to leave all the gory detail of 
bytecode grovelling to a bytecode API - if the logic is applied lazily, 
then the impact on performances should be minimal, and the solution more 
maintainable longer term.


Cheers
Maurizio

On 15/03/2019 07:59, Lindenmaier, Goetz wrote:

Yes, it would be nice if you shared that.
diff -r 03461dde9ace src/java.base/share/classes/java/lang/NullPointerException.java
--- a/src/java.base/share/classes/java/lang/NullPointerException.java	Wed Mar 06 22:09:34 2019 +0100
+++ b/src/java.base/share/classes/java/lang/NullPointerException.java	Fri Mar 08 01:51:22 2019 +
@@ -25,6 +25,24 @@
 
 package java.lang;
 
+import jdk.internal.org.objectweb.asm.ClassReader;
+import jdk.internal.org.objectweb.asm.MethodVisitor;
+import jdk.internal.org.objectweb.asm.Opcodes;
+import jdk.internal.org.objectweb.asm.commons.CodeSizeEvaluator;
+import jdk.internal.org.objectweb.asm.tree.AbstractInsnNode;
+import jdk.internal.org.objectweb.asm.tree.ClassNode;
+import jdk.internal.org.objectweb.asm.tree.MethodNode;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.lang.invoke.MethodType;
+import java.net.URL;
+import java.security.CodeSource;
+import java.security.ProtectionDomain;
+import java.util.Iterator;
+import java.util.stream.Stream;
+
 /**
  * Thrown when an application attempts to use {@code null} in a
  * case where an object is required. These include:
@@ -53,11 +71,17 @@
 class NullPointerException extends RuntimeException {
 private static final long serialVersionUID = 5162710183389028792L;
 
+transient Class clazz;
+transient String methodName;
+transient MethodType methodType;
+transient int pc;
+
 /**
  * Constructs a {@code NullPointerException} with no detail message.
  */
 public NullPointerException() {
 super();
+init();
 }
 
 /**
@@ -68,5 +92,85 @@
  */
 public NullPointerException(String s) {
 super(s);
+init();
+}
+
+void init() {
+StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
+walker.walk(s -> {
+StackWalker.StackFrame frame = s.filter(fr -> fr.getDeclaringClass() != NullPointerException.class)
+.findFirst().get();
+clazz = frame.getDeclaringClass();
+pc = frame.getByteCodeIndex();
+methodName = frame.getMethodName();
+methodType = frame.getMethodType();
+return null;
+});
+}
+
+@Override
+public String getMessage() {
+ProtectionDomain domain = clazz.getProtectionDomain();
+CodeSource codeSource = domain.getCodeSource();
+if (domain.getCodeSource() == null) {
+return super.getMessage();
+}
+URL location = codeSource.getLocation();
+if (location == null) {
+return super.getMessage();
+}
+try {
+File f = new File(location.getFile(),
+clazz.getCanonicalName().replaceAll("\\.", "/") + ".class");
+ClassReader cr = new ClassReader(new FileInputStream(f));
+ClassNode classNode = new ClassNode();
+cr.accept(classNode, 0);
+MethodNode meth = classNode.methods.stream()
+.filter(mn -> mn.name.equals(methodName) &&
+mn.desc.equals(methodType.descriptorString()))
+.findFirst().get();
+InsnVisitor insnVisitor = 

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi everybody, Mark,

I followed your advice and created a JEP:
https://bugs.openjdk.java.net/browse/JDK-8220715

Please point me to things I need to improve formally, this is my first
JEP. Also feel free to fix the administrative information in the 
Jira issue if it is wrong.

And, naturally, you're welcome to discuss the topic!

Best regards,
  Goetz.

> -Original Message-
> From: mark.reinh...@oracle.com 
> Sent: Donnerstag, 14. März 2019 22:38
> To: maurizio.cimadam...@oracle.com; Lindenmaier, Goetz
> 
> Cc: mandy.ch...@oracle.com; roger.ri...@oracle.com; core-libs-
> d...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> 2019/3/14 8:00:20 -0700, maurizio.cimadam...@oracle.com:
> > I second what Mandy says.
> >
> > First let me start by saying that this enhancement will be a great
> > addition to our platform; back in the days when I was teaching some Java
> > classes at the university, I was very aware of how hard it is to
> > diagnose a NPE for someone novel to Java programming.
> 
> Agreed!
> 
> > ...
> >
> > I also think that the design space for such an enhancement is non
> > trivial, and would best be explored (and captured!) in a medium that is
> > something other than a patch. ...
> 
> Agreed, also.
> 
> Goetz -- if, per Mandy’s suggestion, you’re going to write something
> up using the JEP template, might I suggest that you then submit it as
> an actual JEP?  Giving visibility to, and recording, such design-space
> explorations is one of the primary goals of the JEP process.
> 
> - Mark

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Maurizio,

> 
> I second what Mandy says.
> 
> First let me start by saying that this enhancement will be a great
> addition to our platform; back in the days when I was teaching some Java
> classes at the university, I was very aware of how hard it is to
> diagnose a NPE for someone novel to Java programming. A trained eye of
> course can quickly scan the line where an exception occurs, and quickly
> isolate the couple of problematic spots which could have caused an
> exception. But it takes time to get to that point, and having some
> helpful messages to help you to get to that point is, IMHO a valuable
> goal in itself.
Thanks!

> I also think that the design space for such an enhancement is non
> trivial, and would best be explored (and captured!) in a medium that is
> something other than a patch. What kind of improvements should we add to
> the NPE exception? What happens if the NPE already has an user-provided
> details message? Should the enhancement make use of optional debugging
> classfile attributes (you touch this in your nice RFE). And again, what
> are the performance considerations we deem important for this work to be
> declared successful? And, maintenance-wise, what is the right way to
> implement the enhancement? Should we implement that as a pure VM
> feature, should we implement it on top of the existing VM, using some
> classfile manipulation (**) ? Or a combination of those?
I'm working on a text... 

> As you can see, there are so many question this enhancement raises that
> I think going straight for a code review can be a premature move; people
> will be coming at the review table with different answers to the above
> set of questions (and maybe additional questions too :-)), and that
> could make the review process hard and frustrating for all the parties
> involved. So I warmly suggest we take a step back from the code, and
> formulate a proposal for enhancing NPE messages in Java; in this sense,
> a JEP seems to me the most natural way to move forward.
Well, I had the code around, so I started this enhancement just showing our 
code.
I guess a prototype is always a good starting point.

> (**) I have a quick and dirty prototype of that built using ASM which
> I'm happy to share, in case you are interested taking a look when
> evaluating alternatives.
Yes, it would be nice if you shared that.

Best regards,
  Goetz.


> 
> Cheers
> Maurizio
> 
> 
> On 14/03/2019 00:42, Mandy Chung wrote:
> > Hi Goetz,
> >
> > Roger, Coleen, Maurizio and I talked about this proposed feature.
> > We all think that improving NPE message is a useful enhancement for
> > the platform and helps developers to tell what causes NPE.
> >
> > This is not a small enhancement.  Diving into a large code review
> > would not be the best way to move this forward as you can see the
> > discussion so far.
> >
> > It would help if you can start with writing down the problem and
> > the proposal like what improvements are proposed and under what
> > circumstances may be acceptable that NPE message won't be improved.
> > This would get the discussion on the proposal feature and then
> > the discussion of the best way to to implement it in the VM, library,
> > or combination.  You can consider using the JEP template that gives
> > you a good structure to follow for the write up.
> >
> > What do you think?
> >
> > Mandy

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[mark . reinhold]

2019/3/14 8:00:20 -0700, maurizio.cimadam...@oracle.com:
> I second what Mandy says.
> 
> First let me start by saying that this enhancement will be a great 
> addition to our platform; back in the days when I was teaching some Java 
> classes at the university, I was very aware of how hard it is to 
> diagnose a NPE for someone novel to Java programming.

Agreed!

> ...
> 
> I also think that the design space for such an enhancement is non 
> trivial, and would best be explored (and captured!) in a medium that is 
> something other than a patch. ...

Agreed, also.

Goetz -- if, per Mandy’s suggestion, you’re going to write something
up using the JEP template, might I suggest that you then submit it as
an actual JEP?  Giving visibility to, and recording, such design-space
explorations is one of the primary goals of the JEP process.

- Mark

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Coleen, 

thanks for looking at my change..

> For the record, I think the C++ implementation is more straightforward
> than trying to use the Stackwalker and ASM because there's other code
> just like it right here. You have all the information you need directly
> in the Throwable.backtrace field. Walking it makes sense to me.  Also
> the StackWalker has a cost because it creates ResolvedMethodNames that
> must be interned in the native code in case of redefinition.
Yes, I also think that a Java implementation has more costs at 
runtime.  But as this is only encountered if the message is actually
accessed, it's not that much of a concern.

> I didn't make it through the code for bytecodeUtils but I think it
> should be in the interpreter directory where all the other bytecode
> iterating methods are.  
I moved it to the interpreter directory.
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-otherMessages/

> It does seem expensive when printing an NPE
> message.  I wonder if there's some easily helpful variable names that
> you could pick out and not have to do all this work for every sort of
> bytecode. 
I'll describe the algorithm in some detail in the paper requested. 
This might answer this point. 

> Also the file uses non hotspot names also like
> 'createInvalidSource' that should be fixed.
I still need to fix these. I’ll leave this to a final review.  The 
code still might be discarded in favour of another implementation.

> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> NPE/02/src/hotspot/share/classfile/javaClasses.cpp.udiff.html
> 
> There's a 'version' in the backtrace.  If the method->version() doesn't
> match the one in the backtrace, this should return false, because the
> method was redefined and the bci probably won't match. There's a bunch
> of code in javaClasses that does the similar thing.
Starting with the back trace, I will always get the right bytecodes.
The backtrace keeps the method alive as I understand.

The problem I mentioned in other mails is to get that accessible in Java code 
... 
for the C-implementation I have all the proper information 
just per design of the metaspace/backtrace etc. at hand.

Best regards,
  Goetz.



> 
> This isn't a full review.
> 
> Thanks,
> Coleen
> 
> >
> > Thanks!
> >Goetz
> >
> >> Also, it was not strictly correct when I said all that is retained is
> >> the method bytecodes. Exception tables, line number tables and local var
> >> name & type tables are also retained.
> >>
> >> regards,
> >>
> >>
> >> Andrew Dinn
> >> ---
> >> Senior Principal Software Engineer
> >> Red Hat UK Ltd
> >> Registered in England and Wales under Company Registration No.
> 03798903
> >> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Maurizio Cimadamore]

I second what Mandy says.

First let me start by saying that this enhancement will be a great 
addition to our platform; back in the days when I was teaching some Java 
classes at the university, I was very aware of how hard it is to 
diagnose a NPE for someone novel to Java programming. A trained eye of 
course can quickly scan the line where an exception occurs, and quickly 
isolate the couple of problematic spots which could have caused an 
exception. But it takes time to get to that point, and having some 
helpful messages to help you to get to that point is, IMHO a valuable 
goal in itself.


I also think that the design space for such an enhancement is non 
trivial, and would best be explored (and captured!) in a medium that is 
something other than a patch. What kind of improvements should we add to 
the NPE exception? What happens if the NPE already has an user-provided 
details message? Should the enhancement make use of optional debugging 
classfile attributes (you touch this in your nice RFE). And again, what 
are the performance considerations we deem important for this work to be 
declared successful? And, maintenance-wise, what is the right way to 
implement the enhancement? Should we implement that as a pure VM 
feature, should we implement it on top of the existing VM, using some 
classfile manipulation (**) ? Or a combination of those?


As you can see, there are so many question this enhancement raises that 
I think going straight for a code review can be a premature move; people 
will be coming at the review table with different answers to the above 
set of questions (and maybe additional questions too :-)), and that 
could make the review process hard and frustrating for all the parties 
involved. So I warmly suggest we take a step back from the code, and 
formulate a proposal for enhancing NPE messages in Java; in this sense, 
a JEP seems to me the most natural way to move forward.


(**) I have a quick and dirty prototype of that built using ASM which 
I'm happy to share, in case you are interested taking a look when 
evaluating alternatives.


Cheers
Maurizio


On 14/03/2019 00:42, Mandy Chung wrote:

Hi Goetz,

Roger, Coleen, Maurizio and I talked about this proposed feature.
We all think that improving NPE message is a useful enhancement for
the platform and helps developers to tell what causes NPE.

This is not a small enhancement.  Diving into a large code review
would not be the best way to move this forward as you can see the
discussion so far.

It would help if you can start with writing down the problem and
the proposal like what improvements are proposed and under what
circumstances may be acceptable that NPE message won't be improved.
This would get the discussion on the proposal feature and then
the discussion of the best way to to implement it in the VM, library,
or combination.  You can consider using the JEP template that gives
you a good structure to follow for the write up.

What do you think?

Mandy

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi, 

> Roger, Coleen, Maurizio and I talked about this proposed feature.
> We all think that improving NPE message is a useful enhancement for
> the platform and helps developers to tell what causes NPE.
Thanks for this positive feedback :)!

> This is not a small enhancement.  Diving into a large code review
> would not be the best way to move this forward as you can see the
> discussion so far.
> 
> It would help if you can start with writing down the problem and
> the proposal like what improvements are proposed and under what
> circumstances may be acceptable that NPE message won't be improved.
> This would get the discussion on the proposal feature and then
> the discussion of the best way to to implement it in the VM, library,
> or combination.  You can consider using the JEP template that gives
> you a good structure to follow for the write up.

Yes, I can write down the overall design of this.  This probably is a good
idea.
I already started editing, but please give me a day or two.

The past days I have been working on the C-code. I incorporated 
Coleens comments, but mostly changed the messages to print 
different text as it was proposed in several reviews. 
I'm about to publish a webrev with this new coding. This could 
be a new, better basis for discussing the wording of the message.

Best regards,
  Goetz.


 


> 
> What do you think?
> 
> Mandy

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

Hi Goetz,

Roger, Coleen, Maurizio and I talked about this proposed feature.
We all think that improving NPE message is a useful enhancement for
the platform and helps developers to tell what causes NPE.

This is not a small enhancement.  Diving into a large code review
would not be the best way to move this forward as you can see the
discussion so far.

It would help if you can start with writing down the problem and
the proposal like what improvements are proposed and under what
circumstances may be acceptable that NPE message won't be improved.
This would get the discussion on the proposal feature and then
the discussion of the best way to to implement it in the VM, library,
or combination.  You can consider using the JEP template that gives
you a good structure to follow for the write up.

What do you think?

Mandy

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Jason Mehrens]

Hi Goetz,

For NullPointerException, I don't think you want writeReplace() to call a 
public method that can be overridden.  You probably should create a private 
helper method that both getMessage and writeReplace() call into.

Jason


From: core-libs-dev  on behalf of 
Lindenmaier, Goetz 
Sent: Tuesday, March 12, 2019 10:04 AM
To: Peter Levart; Roger Riggs; Java Core Libs
Cc: hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi Peter,

>56 private static final String MESSAGE_PLACEHOLDER =
> "NPE_MESSAGE_PLACEHOLDER";
> Here, the initializer of that constant should create a private instance
> of String:
...
>  private static final String MESSAGE_PLACEHOLDER = new String();
Ok, I understand.

>81 public String getMessage() {
>82 String message = super.getMessage();
>83 if (message == MESSAGE_PLACEHOLDER) {
>84 message = getExtendedNPEMessage();
>85 setMessage(message);
>86 }
>87 return super.getMessage();
>88 }
>
> Why not simply returning 'message' local variable here?
As I removed the synchronization, this would reduce the chance to
get different objects in case of a race.  Highly unlikely though.

I changed the webrev with this solution to include these your remarks:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-PeterLevart/
(updated in-place).

Nevertheless, I think I will follow Roger's demand to not modify the 
defaultMessage
field.

Best regards,
  Goetz.


>
>
> Regards, Peter
>
> On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:
> > Hi Roger,
> >
> >> Maybe 10 years ago, when native was the only solution.
> >> Now there are tools to analyze bytecode in java.
> > I'm working on a Java implementation.
> >
> >>> Peter Levart proposed to initialize the message with a sentinel instead.
> >>> I'll implement this as a proposal.
> >> That's an extra overhead too, any assignment is.
> > Yes, any code requires some run time. But I think this really is negligible
> > in comparison to generating the backtrace, which happens on every
> > exception.  But I'm fine with the 'always recompute, no serialization'
> > solution. If it turns out to be limited, it still can be changed easily.
> >
> >>> I guess we can go without.
> >>> It would be possible to construct a case where two threads
> >>> do getMessage() on the same NPE Object but the string is not
> >>> the same.
> >> Really!, if the bci is the same, the bytecode is the same, what could be
> different
> >> that would change the data used to create the exception?
> > e.getMessage().equals(e.getMessage()) will hold, but
> > e.getMessage() != e.getMessage().
> >
> > I just implemented the two variants of computing the message, they
> > basically differ in NullPointerException.java:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-
> PeterLevart/
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-
> RogerRiggs/
> >
> > I also cleaned up the parameters passed as proposed.
> >
> >>> We uses this code since 2006, it needed only few changes.  I would like to
> >>> contribute a follow up for hidden frames of lambdas.
> >> Valhalla and evolution of the byte code needs to be considered.
> >> Just because its worked for years does not mean it's the best approach
> >> today.  Dropping it in now means maintaining it in its current form
> >> or needing to re-write it later.
> > Well, yes, that is true. For any change.  For any project.  We have heard
> > this argument for many of our changes. I don't really think it's a good
> > argument. As I understand Valhalla is not targeted for jdk13, and
> > holding up changes for some future projects not really is the process
> > of OpenJDK, is it?
> >
> >>>> The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
> >>>> question
> >>>> about how useful the information is,  developers should not need to know
> >>>> about "slot 1".
> >>> Developers should work with class files with debug information and then
> >>> they would get the name printed. If exceptions are thrown in production
> >>> code, any information is helpful.  Should I just write "a local"?
> >> Go back to who you think is going to benefit from it, it seems targeted
> >> at a fairly novice developer, so exposing low level details may be more
> &

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

ere it was running to see what is wrong ...
> >
> > E.g., com/sun/java/swing/plaf/windows/AltFocusIssueTest.java
> > is failing in a headless environment with an NPE on this line:
> >      SwingUtilities.invokeAndWait(() -> frame.dispose());
> > With this change it said "while trying to invoke the method
> javax.swing.JFrame.dispose(()V) of a null object loaded from static field
> AltFocusIssueTest.frame" and I figured it's the fact we run headless that
> > makes the test fail without even looking at the code.
> >
> > Best regards,
> >Goetz
> >
> >
> >
> >
> >
> >
> >
> >
> > From: Roger Riggs 
> > Sent: Dienstag, 12. Februar 2019 17:03
> > To: Lindenmaier, Goetz ; Java Core Libs  libs-...@openjdk.java.net>
> > Cc: hotspot-runtime-...@openjdk.java.net
> > Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> >
> > Hi,
> > On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:
> > Hi Roger,
> >
> > thanks for looking at my change!
> >
> > A few higher level issues should be considered, though the details of
> > the webrev captured my immediate attention.
> >
> > Is this the right feature
> > Yes!!
> > Maybe, that's what debuggers are for.
> >
> >
> >
> >   and is this the right level of implementation (C++/native)?
> > See below.
> > Maybe 10 years ago, when native was the only solution.
> > Now there are tools to analyze bytecode in java.
> >
> >
> >
> >   From a security perspective, adding field names to exceptions exposes
> > information to callers that they could not otherwise get and that breaks
> > encapsulation.
> > That needs to be evaluated.
> > I can not comment on that. How can I trigger an evaluation of this?
> > Who needs to evaluate this?
> >
> > I think there are ways to make this easier to implement and be easier to
> > maintain and perform better.
> >
> > NullPointerException:
> > 28: unused import of Field
> > Removed
> >
> > 64: The lazyComputeMessage boolean should be inverted so it does not
> > require
> >      initialization, false is the default, anything else adds overhead.
> >      And it may not be needed.
> > Peter Levart proposed to initialize the message with a sentinel instead.
> > I'll implement this as a proposal.
> > That's an extra overhead too, any assignment is.
> >
> >
> >
> > 91: Why do you think synchonization is necessary?  It is a performance hit.
> >      It is highly unlikely to be called from multiple threads and the
> > value will
> >      be the same whether it is computed once or multiple times by
> > different threads.
> > I guess we can go without.
> > It would be possible to construct a case where two threads
> > do getMessage() on the same NPE Object but the string is not
> > the same.
> > Really!, if the bci is the same, the bytecode is the same, what could be
> different
> > that would change the data used to create the exception?
> >
> >
> >
> > 99: extendedMessage will never be null (so says the javadoc of
> > getExtendedNPEMessage)
> >     Bug: If it does return null, then null is returned from getMessage
> >     regardless of the value of super.getMessage().
> > Fixed.
> >
> > 121: Simplify getExtendedNPEMessage by making it only responsible for
> > the detail
> >     and not concatenation with an existing message.  Put that in
> > getMessage().
> > Fixed. You are right, I only call this when the message is NULL.
> >
> >     Its not strictly necessary to change the updated message with
> > setMessage().
> >     Avoiding that will avoid complexity and a performance hit.
> >     The message will be computed each time it is needed, and that happens
> > infrequently.
> >     (Even in the serialization case, it will be re-computed from the
> > attached stack frames).
> > No, you can not recompute it from the stacktrace because that
> > does not contain the BCI. Also, after deserialization, the bytecode
> > might look different or not available at all. It depends on the actual
> > bytecode that has been running when the exception was thrown.
> > Right, I realized this too when thinking about it over the weekend.
> >
> > If a suitable low impact mechanism can't be found, then just
> > go back to not exposing the extended message and use only the original.
> > Its a bad idea to twist and contort the local design and accessibility

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Andrew, 

> Ok. However, if you have the appropriate method bytecodes and the BCI at
> which the exception occurred then I'm not sure why you would need ASM.
I implemented it in C++ at first. I was asked to investigate an implementation 
in Java by the reviewers.
 
Thanks!
  Goetz

> Also, it was not strictly correct when I said all that is retained is
> the method bytecodes. Exception tables, line number tables and local var
> name & type tables are also retained.
> 
> regards,
> 
> 
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Andrew Dinn]

On 21/02/2019 12:57, Lindenmaier, Goetz wrote:
> thanks for giving this advice!  It confirms the problems I see.

You are welcome.

> For the context:
> The idea was to implement this in Java using StackWalker and 
> ASM. If I had the right bytecodes, and the right starting point, 
> ASM would be helpful to implement the analysis I think.

Ok. However, if you have the appropriate method bytecodes and the BCI at
which the exception occurred then I'm not sure why you would need ASM.

Also, it was not strictly correct when I said all that is retained is
the method bytecodes. Exception tables, line number tables and local var
name & type tables are also retained.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Andrew, 

thanks for giving this advice!  It confirms the problems I see.

For the context:
I proposed better NPE exception messages: 
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/

The original implementation is C++ and walks the metaspace 
given the method* and BCI where the exception occurred.
So it uses only data already sitting in memory.
See JVM_GetExtendedNPEMessage() in jvm.cpp.

The idea was to implement this in Java using StackWalker and 
ASM. If I had the right bytecodes, and the right starting point, 
ASM would be helpful to implement the analysis I think.

I'll look some more into the reconstituter code...

Thanks,
  Goetz.


> -Original Message-
> From: Andrew Dinn 
> Sent: Mittwoch, 20. Februar 2019 18:29
> To: Lindenmaier, Goetz 
> Cc: core-libs-dev ; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> 
> Forgive me for jumping in here and, possibly, misunderstanding what you
> want -- I may have misunderstood what you are trying to do as I have not
> been following this thread carefully.
> 
> On 20/02/2019 16:36, Lindenmaier, Goetz wrote:
> > 1. As I understand, it would be simple to extend ASM to deliver the initial
> > BCIs when the stream is parsed.  They could be stored separately and
> > discarded once something gets modified (similar to InsnList.cache).
> 
> Non-method bytecodes are normally dropped by the JVM during parsing.
> Only method bytecode is retained in the metadata model. One exception is
> when a registered ClassFileTransformer changes the loaded bytecode at
> load time. The original bytecode for the class is saved when that happens.
> 
> Note that retaining method bytecode does not imply retaining the
> original classpool in bytecode format. Indices into the classpool are
> interpreted as indices into a metadata version of the classpool. Amongst
> other things, that allows sharing of symbols that occur in more than one
> class files. This avoids a /vast/ amount of storage costs.
> 
> > 2. Is there any possibility to access the live datastructures/bytecode in 
> > the
> VM?
> > I was pointed to former JvmtiClassFileReconstitutor ... something like that?
> JvmtiClassFileReconstitutor exists precisely to recreate the full class
> bytecode from the metadata when needed. I believe it is only ever needed
> to allow a ClassFileTransformer to retransform a loaded class that was
> not transformed at load time. Using it seems like overkill to me (also
> see below). If you want to locate a specific bytecode instruction in a
> method you should be able to do so by pulling the method bytecodes out
> of the metadata.
> 
> Also, if you want your message to reflect the bytecode that is actually
> in use when the exception occurs then you really need to do it by
> pulling the bytecodes out of the method metadata. The bytecode returned
> by JvmtiClassFileReconstitutor will not include any bytecode changes
> that were installed by a ClassFileTransformer.
> 
> However, this is a potential can of worms because old and new versions
> of a method and associated bytecode can exist at the same time. You need
> to be sure which version of the method and, hence, bytecode the
> exception was generated from. If you are trying to do this from Java by
> calling into the JVM then I think you are going to have problems.
> 
> regards,
> 
> 
> Andrew Dinn
> ---
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Andrew Dinn]

Hi Goetz,

Forgive me for jumping in here and, possibly, misunderstanding what you
want -- I may have misunderstood what you are trying to do as I have not
been following this thread carefully.

On 20/02/2019 16:36, Lindenmaier, Goetz wrote:
> 1. As I understand, it would be simple to extend ASM to deliver the initial 
> BCIs when the stream is parsed.  They could be stored separately and
> discarded once something gets modified (similar to InsnList.cache).

Non-method bytecodes are normally dropped by the JVM during parsing.
Only method bytecode is retained in the metadata model. One exception is
when a registered ClassFileTransformer changes the loaded bytecode at
load time. The original bytecode for the class is saved when that happens.

Note that retaining method bytecode does not imply retaining the
original classpool in bytecode format. Indices into the classpool are
interpreted as indices into a metadata version of the classpool. Amongst
other things, that allows sharing of symbols that occur in more than one
class files. This avoids a /vast/ amount of storage costs.

> 2. Is there any possibility to access the live datastructures/bytecode in the 
> VM?
> I was pointed to former JvmtiClassFileReconstitutor ... something like that?
JvmtiClassFileReconstitutor exists precisely to recreate the full class
bytecode from the metadata when needed. I believe it is only ever needed
to allow a ClassFileTransformer to retransform a loaded class that was
not transformed at load time. Using it seems like overkill to me (also
see below). If you want to locate a specific bytecode instruction in a
method you should be able to do so by pulling the method bytecodes out
of the metadata.

Also, if you want your message to reflect the bytecode that is actually
in use when the exception occurs then you really need to do it by
pulling the bytecodes out of the method metadata. The bytecode returned
by JvmtiClassFileReconstitutor will not include any bytecode changes
that were installed by a ClassFileTransformer.

However, this is a potential can of worms because old and new versions
of a method and associated bytecode can exist at the same time. You need
to be sure which version of the method and, hence, bytecode the
exception was generated from. If you are trying to do this from Java by
calling into the JVM then I think you are going to have problems.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Remi,

> yes, it's a feature of ASM, offer the right abstraction to do bytecode
> generation/transformation i.e. a stream of opcodes that are more abstract
> than the real bytecode, so there is no way to get a direct access to the
> bytecode at a specific BCI.
For the generation of the NPE message I intend the abstraction would be 
helpful. But I need to identify the bytecode that raised the NPE to start
with, and I need to operate on the exact bytecode that was executed when 
the NPE occurred.

Thus, two questions:

1. As I understand, it would be simple to extend ASM to deliver the initial 
BCIs when the stream is parsed.  They could be stored separately and
discarded once something gets modified (similar to InsnList.cache).

2. Is there any possibility to access the live datastructures/bytecode in the 
VM?
I was pointed to former JvmtiClassFileReconstitutor ... something like that?

Thanks and best regards,
  Goetz.

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Remi Forax]

with my ASM hat,

- Mail original -
> De: "Lindenmaier, Goetz" 
> À: "Peter Levart" , "Roger Riggs" 
> , "core-libs-dev"
> 
> Cc: hotspot-runtime-...@openjdk.java.net
> Envoyé: Mardi 19 Février 2019 17:08:07
> Objet: RE: RFR(L): 8218628: Add detailed message to NullPointerException 
> describing what is null.

> Hi,
> 
> I have been looking at how to implement this in Java using
> StackWalker and ASM.
> Either I oversee something or there is a row of deficiencies with
> these tools to solve my issue.
> 
> StackWalker allows me to collect class name, method name, method
> description and BCI. Given the current implementation, this must
> happen in the NPE constructor which is very inefficient, but
> works. Mandy, should I really change Throwable/StackWalker to
> overcome this? Alternatively, I could JNI-call into the VM
> and get the info when I need it.
> 
> ASM allows me to access the class and the bytecode of the method.
> 
> Unfortunately, ASM has no notion of BCI. It also does not
> externally expose the bytecodes of the methods. It simplifies
> the bytecodes, which is useful to manipulate them, but does
> not allow me to recompute the BCI of instructions, say, iterating
> the InsnList.

yes, it's a feature of ASM, offer the right abstraction to do bytecode 
generation/transformation i.e. a stream of opcodes that are more abstract than 
the real bytecode, so there is no way to get a direct access to the bytecode at 
a specific BCI.

> 
> Further, as I understand, the bytecode I can access in ASM
> is the bytecode as it resides in the class file, not the
> bytecode used by hotspot (bitwise). Hotspot rewrites bytecodes when
> it loads them. And the BCI returned by StackWalker corresponds
> to the bytecode used by hotspot, not that in the class file.
> 
> If a method has been rewritten, there even might be several
> instances of the same method/bytecode, the old still needed for
> running executions and the new, rewritten one. The BCI
> is only correct for one of these.
> 
> So I don't see how I should find the ASM representation of
> the bytecode that caused the NPE.  Do you have any advice?
> 
> Best regards,
>  Goetz.
> 

Rémi

> 
>> -Original Message-----
>> From: Peter Levart 
>> Sent: Freitag, 15. Februar 2019 18:38
>> To: Lindenmaier, Goetz ; Roger Riggs
>> ; Java Core Libs 
>> Cc: hotspot-runtime-...@openjdk.java.net
>> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
>> describing what is null.
>> 
>> Hi Goetz,
>> 
>> Just a couple of things if you go that route (of placeholder String):
>> 
>> 
>>    56 private static final String MESSAGE_PLACEHOLDER =
>> "NPE_MESSAGE_PLACEHOLDER";
>> 
>> Here, the initializer of that constant should create a private instance
>> of String:
>> 
>>      private static final String MESSAGE_PLACEHOLDER = new
>> String("NPE_MESSAGE_PLACEHOLDER");
>> 
>> or even (since the content of the placeholder string doesn't matter -
>> the identity does):
>> 
>>      private static final String MESSAGE_PLACEHOLDER = new String();
>> 
>> ...or else anybody calling new
>> NullPointerException("NPE_MESSAGE_PLACEHOLDER") will also be the subject
>> of replacement since the string literals are interned constants. I.e.:
>> 
>> "NPE_MESSAGE_PLACEHOLDER" == "NPE_MESSAGE_PLACEHOLDER"
>> 
>> 
>>    81 public String getMessage() {
>>    82 String message = super.getMessage();
>>    83 if (message == MESSAGE_PLACEHOLDER) {
>>    84 message = getExtendedNPEMessage();
>>    85 setMessage(message);
>>    86 }
>>    87 return super.getMessage();
>>    88 }
>> 
>> Why not simply returning 'message' local variable here?
>> 
>> 
>> Regards, Peter
>> 
>> On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:
>> > Hi Roger,
>> >
>> >> Maybe 10 years ago, when native was the only solution.
>> >> Now there are tools to analyze bytecode in java.
>> > I'm working on a Java implementation.
>> >
>> >>> Peter Levart proposed to initialize the message with a sentinel instead.
>> >>> I'll implement this as a proposal.
>> >> That's an extra overhead too, any assignment is.
>> > Yes, any code requires some run time. But I think this really is negligible
>> > in comparison to generating the backtrace, which happens on every
>> > exception.  But I'm fine w

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

On 2/19/19 8:08 AM, Lindenmaier, Goetz wrote:

Hi,

I have been looking at how to implement this in Java using
StackWalker and ASM.
Either I oversee something or there is a row of deficiencies with
these tools to solve my issue.

StackWalker allows me to collect class name, method name, method
description and BCI. Given the current implementation, this must
happen in the NPE constructor which is very inefficient, but
works. Mandy, should I really change Throwable/StackWalker to
overcome this? Alternatively, I could JNI-call into the VM
and get the info when I need it.



One idea is to make a call to VM to fill StackFrameInfo array
for the backtrace stored in NPE (that is in VM native representation)
and then iterate StackFrameInfo and extract the code/local variable etc
from the classfile per class/method and BCI for each frame.

I don't think you have to modify Throwable/StackWalker API to do so
but keep it implementation details.


ASM allows me to access the class and the bytecode of the method.

Unfortunately, ASM has no notion of BCI. It also does not
externally expose the bytecodes of the methods. It simplifies
the bytecodes, which is useful to manipulate them, but does
not allow me to recompute the BCI of instructions, say, iterating
the InsnList.

Further, as I understand, the bytecode I can access in ASM
is the bytecode as it resides in the class file, not the
bytecode used by hotspot (bitwise). Hotspot rewrites bytecodes when
it loads them. And the BCI returned by StackWalker corresponds
to the bytecode used by hotspot, not that in the class file.


When the class file is redefined, it's reasonable to report the line
number is not available.  If you want to keep the previous versions
of class byte, an instrumentation agent can do this work.


If a method has been rewritten, there even might be several
instances of the same method/bytecode, the old still needed for
running executions and the new, rewritten one. The BCI
is only correct for one of these.


The old method would be GC'ed when it becomes unreachable, right?
I expect you won't be able to reliably report the info when the method
has been redefined.


So I don't see how I should find the ASM representation of
the bytecode that caused the NPE.  Do you have any advice?


Have you looked at the Code attribute?

Mandy


Best regards,
   Goetz.



-Original Message-
From: Peter Levart 
Sent: Freitag, 15. Februar 2019 18:38
To: Lindenmaier, Goetz ; Roger Riggs
; Java Core Libs 
Cc: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

Hi Goetz,

Just a couple of things if you go that route (of placeholder String):


    56 private static final String MESSAGE_PLACEHOLDER =
"NPE_MESSAGE_PLACEHOLDER";

Here, the initializer of that constant should create a private instance
of String:

      private static final String MESSAGE_PLACEHOLDER = new
String("NPE_MESSAGE_PLACEHOLDER");

or even (since the content of the placeholder string doesn't matter -
the identity does):

      private static final String MESSAGE_PLACEHOLDER = new String();

...or else anybody calling new
NullPointerException("NPE_MESSAGE_PLACEHOLDER") will also be the subject
of replacement since the string literals are interned constants. I.e.:

"NPE_MESSAGE_PLACEHOLDER" == "NPE_MESSAGE_PLACEHOLDER"


    81 public String getMessage() {
    82 String message = super.getMessage();
    83 if (message == MESSAGE_PLACEHOLDER) {
    84 message = getExtendedNPEMessage();
    85 setMessage(message);
    86 }
    87 return super.getMessage();
    88 }

Why not simply returning 'message' local variable here?


Regards, Peter

On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:

Hi Roger,


Maybe 10 years ago, when native was the only solution.
Now there are tools to analyze bytecode in java.

I'm working on a Java implementation.


Peter Levart proposed to initialize the message with a sentinel instead.
I'll implement this as a proposal.

That's an extra overhead too, any assignment is.

Yes, any code requires some run time. But I think this really is negligible
in comparison to generating the backtrace, which happens on every
exception.  But I'm fine with the 'always recompute, no serialization'
solution. If it turns out to be limited, it still can be changed easily.


I guess we can go without.
It would be possible to construct a case where two threads
do getMessage() on the same NPE Object but the string is not
the same.

Really!, if the bci is the same, the bytecode is the same, what could be

different

that would change the data used to create the exception?

e.getMessage().equals(e.getMessage()) will hold, but
e.getMessage() != e.getMessage().

I just implemented the two variants of computing the message, they
basically differ in NullPointerException.java:
http://cr

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi, 

I have been looking at how to implement this in Java using
StackWalker and ASM.
Either I oversee something or there is a row of deficiencies with 
these tools to solve my issue.

StackWalker allows me to collect class name, method name, method
description and BCI. Given the current implementation, this must
happen in the NPE constructor which is very inefficient, but
works. Mandy, should I really change Throwable/StackWalker to
overcome this? Alternatively, I could JNI-call into the VM
and get the info when I need it.

ASM allows me to access the class and the bytecode of the method.

Unfortunately, ASM has no notion of BCI. It also does not 
externally expose the bytecodes of the methods. It simplifies
the bytecodes, which is useful to manipulate them, but does
not allow me to recompute the BCI of instructions, say, iterating
the InsnList.

Further, as I understand, the bytecode I can access in ASM
is the bytecode as it resides in the class file, not the 
bytecode used by hotspot (bitwise). Hotspot rewrites bytecodes when
it loads them. And the BCI returned by StackWalker corresponds
to the bytecode used by hotspot, not that in the class file.

If a method has been rewritten, there even might be several
instances of the same method/bytecode, the old still needed for 
running executions and the new, rewritten one. The BCI
is only correct for one of these.

So I don't see how I should find the ASM representation of 
the bytecode that caused the NPE.  Do you have any advice?

Best regards,
  Goetz.


> -Original Message-
> From: Peter Levart 
> Sent: Freitag, 15. Februar 2019 18:38
> To: Lindenmaier, Goetz ; Roger Riggs
> ; Java Core Libs 
> Cc: hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> 
> Just a couple of things if you go that route (of placeholder String):
> 
> 
>    56 private static final String MESSAGE_PLACEHOLDER =
> "NPE_MESSAGE_PLACEHOLDER";
> 
> Here, the initializer of that constant should create a private instance
> of String:
> 
>      private static final String MESSAGE_PLACEHOLDER = new
> String("NPE_MESSAGE_PLACEHOLDER");
> 
> or even (since the content of the placeholder string doesn't matter -
> the identity does):
> 
>      private static final String MESSAGE_PLACEHOLDER = new String();
> 
> ...or else anybody calling new
> NullPointerException("NPE_MESSAGE_PLACEHOLDER") will also be the subject
> of replacement since the string literals are interned constants. I.e.:
> 
> "NPE_MESSAGE_PLACEHOLDER" == "NPE_MESSAGE_PLACEHOLDER"
> 
> 
>    81 public String getMessage() {
>    82 String message = super.getMessage();
>    83 if (message == MESSAGE_PLACEHOLDER) {
>    84 message = getExtendedNPEMessage();
>    85 setMessage(message);
>    86 }
>    87 return super.getMessage();
>    88 }
> 
> Why not simply returning 'message' local variable here?
> 
> 
> Regards, Peter
> 
> On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:
> > Hi Roger,
> >
> >> Maybe 10 years ago, when native was the only solution.
> >> Now there are tools to analyze bytecode in java.
> > I'm working on a Java implementation.
> >
> >>> Peter Levart proposed to initialize the message with a sentinel instead.
> >>> I'll implement this as a proposal.
> >> That's an extra overhead too, any assignment is.
> > Yes, any code requires some run time. But I think this really is negligible
> > in comparison to generating the backtrace, which happens on every
> > exception.  But I'm fine with the 'always recompute, no serialization'
> > solution. If it turns out to be limited, it still can be changed easily.
> >
> >>> I guess we can go without.
> >>> It would be possible to construct a case where two threads
> >>> do getMessage() on the same NPE Object but the string is not
> >>> the same.
> >> Really!, if the bci is the same, the bytecode is the same, what could be
> different
> >> that would change the data used to create the exception?
> > e.getMessage().equals(e.getMessage()) will hold, but
> > e.getMessage() != e.getMessage().
> >
> > I just implemented the two variants of computing the message, they
> > basically differ in NullPointerException.java:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-
> PeterLevart/
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-
> RogerRiggs/
> >
> > I also cleaned up the parameters passed as proposed.
> >
> >>> We uses this co

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Peter Levart]

Hi Goetz,

Just a couple of things if you go that route (of placeholder String):


  56 private static final String MESSAGE_PLACEHOLDER = 
"NPE_MESSAGE_PLACEHOLDER";


Here, the initializer of that constant should create a private instance 
of String:


    private static final String MESSAGE_PLACEHOLDER = new 
String("NPE_MESSAGE_PLACEHOLDER");


or even (since the content of the placeholder string doesn't matter - 
the identity does):


    private static final String MESSAGE_PLACEHOLDER = new String();

...or else anybody calling new 
NullPointerException("NPE_MESSAGE_PLACEHOLDER") will also be the subject 
of replacement since the string literals are interned constants. I.e.:


"NPE_MESSAGE_PLACEHOLDER" == "NPE_MESSAGE_PLACEHOLDER"


  81 public String getMessage() {
  82 String message = super.getMessage();
  83 if (message == MESSAGE_PLACEHOLDER) {
  84 message = getExtendedNPEMessage();
  85 setMessage(message);
  86 }
  87 return super.getMessage();
  88 }

Why not simply returning 'message' local variable here?


Regards, Peter

On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:

Hi Roger,


Maybe 10 years ago, when native was the only solution.
Now there are tools to analyze bytecode in java.

I'm working on a Java implementation.


Peter Levart proposed to initialize the message with a sentinel instead.
I'll implement this as a proposal.

That's an extra overhead too, any assignment is.

Yes, any code requires some run time. But I think this really is negligible
in comparison to generating the backtrace, which happens on every
exception.  But I'm fine with the 'always recompute, no serialization'
solution. If it turns out to be limited, it still can be changed easily.


I guess we can go without.
It would be possible to construct a case where two threads
do getMessage() on the same NPE Object but the string is not
the same.

Really!, if the bci is the same, the bytecode is the same, what could be 
different
that would change the data used to create the exception?

e.getMessage().equals(e.getMessage()) will hold, but
e.getMessage() != e.getMessage().

I just implemented the two variants of computing the message, they
basically differ in NullPointerException.java:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-PeterLevart/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-RogerRiggs/

I also cleaned up the parameters passed as proposed.


We uses this code since 2006, it needed only few changes.  I would like to
contribute a follow up for hidden frames of lambdas.

Valhalla and evolution of the byte code needs to be considered.
Just because its worked for years does not mean it's the best approach
today.  Dropping it in now means maintaining it in its current form
or needing to re-write it later.

Well, yes, that is true. For any change.  For any project.  We have heard
this argument for many of our changes. I don't really think it's a good
argument. As I understand Valhalla is not targeted for jdk13, and
holding up changes for some future projects not really is the process
of OpenJDK, is it?


The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
question
about how useful the information is,  developers should not need to know
about "slot 1".

Developers should work with class files with debug information and then
they would get the name printed. If exceptions are thrown in production
code, any information is helpful.  Should I just write "a local"?

Go back to who you think is going to benefit from it, it seems targeted
at a fairly novice developer, so exposing low level details may be more
confusing that just giving a line number and NPE.

Especially on our improved NPE messages we always got good feedback
from the developers within SAP. And there are quite some experienced
people.  Any message requires some background to be helpful.  And I
like to get any information I can have in first place. Attaching the
debugger often is a big overhead. E.g., I look at about 50 failing jtreg
tests every day, I don't want to get each into the debugger every time
in the setup where it was running to see what is wrong ...

E.g., com/sun/java/swing/plaf/windows/AltFocusIssueTest.java
is failing in a headless environment with an NPE on this line:
 SwingUtilities.invokeAndWait(() -> frame.dispose());
With this change it said "while trying to invoke the method 
javax.swing.JFrame.dispose(()V) of a null object loaded from static field 
AltFocusIssueTest.frame" and I figured it's the fact we run headless that
makes the test fail without even looking at the code.

Best regards,
   Goetz








From: Roger Riggs 
Sent: Dienstag, 12. Februar 2019 17:03
To: Lindenmaier, Goetz ; Java Core Libs 

Cc: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException 
describing wha

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Roger,

> Maybe 10 years ago, when native was the only solution.
> Now there are tools to analyze bytecode in java.
I'm working on a Java implementation. 

> > Peter Levart proposed to initialize the message with a sentinel instead.
> > I'll implement this as a proposal.
> That's an extra overhead too, any assignment is.
Yes, any code requires some run time. But I think this really is negligible
in comparison to generating the backtrace, which happens on every
exception.  But I'm fine with the 'always recompute, no serialization' 
solution. If it turns out to be limited, it still can be changed easily.

> > I guess we can go without. 
> > It would be possible to construct a case where two threads
> > do getMessage() on the same NPE Object but the string is not 
> > the same.
> Really!, if the bci is the same, the bytecode is the same, what could be 
> different
> that would change the data used to create the exception?
e.getMessage().equals(e.getMessage()) will hold, but
e.getMessage() != e.getMessage().

I just implemented the two variants of computing the message, they 
basically differ in NullPointerException.java:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-PeterLevart/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-RogerRiggs/

I also cleaned up the parameters passed as proposed.

> > We uses this code since 2006, it needed only few changes.  I would like to 
> > contribute a follow up for hidden frames of lambdas.
> Valhalla and evolution of the byte code needs to be considered.
> Just because its worked for years does not mean it's the best approach
> today.  Dropping it in now means maintaining it in its current form
> or needing to re-write it later.
Well, yes, that is true. For any change.  For any project.  We have heard
this argument for many of our changes. I don't really think it's a good
argument. As I understand Valhalla is not targeted for jdk13, and 
holding up changes for some future projects not really is the process
of OpenJDK, is it? 

> > > The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
> > > question
> > > about how useful the information is,  developers should not need to know
> > > about "slot 1". 
> > Developers should work with class files with debug information and then
> > they would get the name printed. If exceptions are thrown in production
> > code, any information is helpful.  Should I just write "a local"?
> Go back to who you think is going to benefit from it, it seems targeted
> at a fairly novice developer, so exposing low level details may be more
> confusing that just giving a line number and NPE.
Especially on our improved NPE messages we always got good feedback
from the developers within SAP. And there are quite some experienced
people.  Any message requires some background to be helpful.  And I
like to get any information I can have in first place. Attaching the 
debugger often is a big overhead. E.g., I look at about 50 failing jtreg
tests every day, I don't want to get each into the debugger every time
in the setup where it was running to see what is wrong ... 

E.g., com/sun/java/swing/plaf/windows/AltFocusIssueTest.java
is failing in a headless environment with an NPE on this line:
SwingUtilities.invokeAndWait(() -> frame.dispose());
With this change it said "while trying to invoke the method 
javax.swing.JFrame.dispose(()V) of a null object loaded from static field 
AltFocusIssueTest.frame" and I figured it's the fact we run headless that 
makes the test fail without even looking at the code.  

Best regards,
  Goetz








From: Roger Riggs  
Sent: Dienstag, 12. Februar 2019 17:03
To: Lindenmaier, Goetz ; Java Core Libs 

Cc: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi,
On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:
Hi Roger, 

thanks for looking at my change!

A few higher level issues should be considered, though the details of
the webrev captured my immediate attention.

Is this the right feature
Yes!!
Maybe, that's what debuggers are for.



 and is this the right level of implementation (C++/native)?
See below.
Maybe 10 years ago, when native was the only solution.
Now there are tools to analyze bytecode in java.



 From a security perspective, adding field names to exceptions exposes
information to callers that they could not otherwise get and that breaks
encapsulation.
That needs to be evaluated.
I can not comment on that. How can I trigger an evaluation of this?
Who needs to evaluate this?

I think there are ways to make this easier to implement and be easier to
maintain and perform better.

NullPointerException:
28: unused import of Field
Removed

64: The lazyComputeMessage boolean should be inverted so it does no

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

I have to say that the example NPE messages this patch has
are like stuffing as much relevant information in the message
but they are not developer-friendly as Roger and David has
commented.  Exposing bytecode-level details is very confusing
to Joe's developer while, from your perspective, anything
would be useful because this is helpful for production support
purpose.  It's a tradeoff.  That's why I think the default message
should include an informative and developer-friendly message whereas
the field support needs the customization ability for better
serviceability at production environment.

Prototyping this in Java will allow us to explore other possibility
for example enabling a plugin to format a custom exception message
rather than hardcoding this in the default message.   This RFE
and JDK-8211152 may better be addressed by providing the ability
to customize the exception message format (that's the idea in
my mind but I have not had time to pursue further).

Mandy

On 2/13/19 1:09 AM, Lindenmaier, Goetz wrote:

Hi Mandy,

Thanks for supporting my intend of adding the message as such!
I'll start implementing this in Java and come back with a webrev
in a while.

In parallel, I would like to continue discussing the other
topics, e.g., the wording of the message. I will probably come up
with a separate webrev for that.

Best regards,
   Goetz.




-Original Message-
From: core-libs-dev  On Behalf
Of Mandy Chung
Sent: Tuesday, February 12, 2019 7:32 PM
To: Roger Riggs 
Cc: Java Core Libs ; hotspot-runtime-
d...@openjdk.java.net
Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

On 2/8/19 11:46 AM, Roger Riggs wrote:

Hi,

A few higher level issues should be considered, though the details
of the webrev captured my immediate attention.

Is this the right feature and is this the right level of implementation
(C++/native)?
:
How much of this can be done in Java code with StackWalker and other
java APIs?
It would be a shame to add this much native code if there was a more

robust

way to implement it using APIs with more leverage.


Improving the NPE message for better diagnosability is helpful while
I share the same concern Roger raised.

Implementing this feature in Java and the library would be a better
choice as this isn't absolutely required to be done in VM in native.

NPE keeps a backtrace capturing the method id and bci of each stack
frame.  One option to explore is to have StackWalker to accept a
Throwable object that returns a stream of StackFrame which allows
you to get the method and BCI and also code source (I started a
prototype for JDK-8189752 some time ago).  It can use the bytecode
library e.g. ASM to read the bytecode.  For NPE message, you can
implement a specialized StackFrameTraverser just for building
an exception message purpose.

Mandy

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Bernd, 

I think this is a feasible idea, while I'm not sure whether 
this is really critical information. 
The stack trace already contains the names of Classes,
Inner classes etc.. Field names should be not that more
sensible information I guess, if at all.

> but maybe as  a default?
You mean to enable it per default?  Yes.

Best regards,
  Goetz.


> -Original Message-
> From: core-libs-dev  On Behalf
> Of Bernd Eckenfels
> Sent: Tuesday, February 12, 2019 8:45 PM
> To: Java Core Libs 
> Cc: hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> For security reasons I would add it to `jdk.includeInExceptions`, but maybe as
> a default?
> 
> Gruss
> Bernd
> --
> http://bernd.eckenfels.net
> 
> 
> Von: core-libs-dev  im Auftrag
> von Roger Riggs 
> Gesendet: Dienstag, Februar 12, 2019 8:07 PM
> An: Lindenmaier, Goetz; Java Core Libs
> Cc: hotspot-runtime-...@openjdk.java.net
> Betreff: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi,
> 
> On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:
> > Hi Roger,
> >
> > thanks for looking at my change!
> >
> >> A few higher level issues should be considered, though the details of
> >> the webrev captured my immediate attention.
> >>
> >> Is this the right feature
> > Yes!!
> Maybe, that's what debuggers are for.
> >
> >> and is this the right level of implementation (C++/native)?
> > See below.
> Maybe 10 years ago, when native was the only solution.
> Now there are tools to analyze bytecode in java.
> >
> >> From a security perspective, adding field names to exceptions exposes
> >> information to callers that they could not otherwise get and that breaks
> >> encapsulation.
> >> That needs to be evaluated.
> > I can not comment on that. How can I trigger an evaluation of this?
> > Who needs to evaluate this?
> >
> >> I think there are ways to make this easier to implement and be easier to
> >> maintain and perform better.
> >>
> >> NullPointerException:
> >> 28: unused import of Field
> > Removed
> >
> >> 64: The lazyComputeMessage boolean should be inverted so it does not
> >> require
> >>initialization, false is the default, anything else adds overhead.
> >>And it may not be needed.
> > Peter Levart proposed to initialize the message with a sentinel instead.
> > I'll implement this as a proposal.
> That's an extra overhead too, any assignment is.
> >
> >> 91: Why do you think synchonization is necessary?  It is a performance hit.
> >>It is highly unlikely to be called from multiple threads and the
> >> value will
> >>be the same whether it is computed once or multiple times by
> >> different threads.
> > I guess we can go without.
> > It would be possible to construct a case where two threads
> > do getMessage() on the same NPE Object but the string is not
> > the same.
> Really!, if the bci is the same, the bytecode is the same, what could be
> different
> that would change the data used to create the exception?
> >
> >> 99: extendedMessage will never be null (so says the javadoc of
> >> getExtendedNPEMessage)
> >>   Bug: If it does return null, then null is returned from getMessage
> >>   regardless of the value of super.getMessage().
> > Fixed.
> >
> >> 121: Simplify getExtendedNPEMessage by making it only responsible for
> >> the detail
> >>   and not concatenation with an existing message.  Put that in
> >> getMessage().
> > Fixed. You are right, I only call this when the message is NULL.
> >
> >>   Its not strictly necessary to change the updated message with
> >> setMessage().
> >>   Avoiding that will avoid complexity and a performance hit.
> >>   The message will be computed each time it is needed, and that happens
> >> infrequently.
> >>   (Even in the serialization case, it will be re-computed from the
> >> attached stack frames).
> > No, you can not recompute it from the stacktrace because that
> > does not contain the BCI. Also, after deserialization, the bytecode
> > might look different or not available at all. It depends on the actual
> > bytecode that has been running when the exception was thrown.
> Right, I realized this too when thinking about it over the weekend.
> 
> If a suitable low impact mechanism can't b

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Mandy, 

Thanks for supporting my intend of adding the message as such!
I'll start implementing this in Java and come back with a webrev
in a while. 

In parallel, I would like to continue discussing the other 
topics, e.g., the wording of the message. I will probably come up 
with a separate webrev for that.

Best regards,
  Goetz.



> -Original Message-
> From: core-libs-dev  On Behalf
> Of Mandy Chung
> Sent: Tuesday, February 12, 2019 7:32 PM
> To: Roger Riggs 
> Cc: Java Core Libs ; hotspot-runtime-
> d...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> On 2/8/19 11:46 AM, Roger Riggs wrote:
> > Hi,
> >
> > A few higher level issues should be considered, though the details
> > of the webrev captured my immediate attention.
> >
> > Is this the right feature and is this the right level of implementation
> > (C++/native)?
> > :
> > How much of this can be done in Java code with StackWalker and other
> > java APIs?
> > It would be a shame to add this much native code if there was a more
> robust
> > way to implement it using APIs with more leverage.
> 
> Improving the NPE message for better diagnosability is helpful while
> I share the same concern Roger raised.
> 
> Implementing this feature in Java and the library would be a better
> choice as this isn't absolutely required to be done in VM in native.
> 
> NPE keeps a backtrace capturing the method id and bci of each stack
> frame.  One option to explore is to have StackWalker to accept a
> Throwable object that returns a stream of StackFrame which allows
> you to get the method and BCI and also code source (I started a
> prototype for JDK-8189752 some time ago).  It can use the bytecode
> library e.g. ASM to read the bytecode.  For NPE message, you can
> implement a specialized StackFrameTraverser just for building
> an exception message purpose.
> 
> Mandy

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Bernd Eckenfels]

For security reasons I would add it to `jdk.includeInExceptions`, but maybe as 
a default?

Gruss
Bernd
--
http://bernd.eckenfels.net


Von: core-libs-dev  im Auftrag von 
Roger Riggs 
Gesendet: Dienstag, Februar 12, 2019 8:07 PM
An: Lindenmaier, Goetz; Java Core Libs
Cc: hotspot-runtime-...@openjdk.java.net
Betreff: Re: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi,

On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:
> Hi Roger,
>
> thanks for looking at my change!
>
>> A few higher level issues should be considered, though the details of
>> the webrev captured my immediate attention.
>>
>> Is this the right feature
> Yes!!
Maybe, that's what debuggers are for.
>
>> and is this the right level of implementation (C++/native)?
> See below.
Maybe 10 years ago, when native was the only solution.
Now there are tools to analyze bytecode in java.
>
>> From a security perspective, adding field names to exceptions exposes
>> information to callers that they could not otherwise get and that breaks
>> encapsulation.
>> That needs to be evaluated.
> I can not comment on that. How can I trigger an evaluation of this?
> Who needs to evaluate this?
>
>> I think there are ways to make this easier to implement and be easier to
>> maintain and perform better.
>>
>> NullPointerException:
>> 28: unused import of Field
> Removed
>
>> 64: The lazyComputeMessage boolean should be inverted so it does not
>> require
>>initialization, false is the default, anything else adds overhead.
>>And it may not be needed.
> Peter Levart proposed to initialize the message with a sentinel instead.
> I'll implement this as a proposal.
That's an extra overhead too, any assignment is.
>
>> 91: Why do you think synchonization is necessary?  It is a performance hit.
>>It is highly unlikely to be called from multiple threads and the
>> value will
>>be the same whether it is computed once or multiple times by
>> different threads.
> I guess we can go without.
> It would be possible to construct a case where two threads
> do getMessage() on the same NPE Object but the string is not
> the same.
Really!, if the bci is the same, the bytecode is the same, what could be
different
that would change the data used to create the exception?
>
>> 99: extendedMessage will never be null (so says the javadoc of
>> getExtendedNPEMessage)
>>   Bug: If it does return null, then null is returned from getMessage
>>   regardless of the value of super.getMessage().
> Fixed.
>
>> 121: Simplify getExtendedNPEMessage by making it only responsible for
>> the detail
>>   and not concatenation with an existing message.  Put that in
>> getMessage().
> Fixed. You are right, I only call this when the message is NULL.
>
>>   Its not strictly necessary to change the updated message with
>> setMessage().
>>   Avoiding that will avoid complexity and a performance hit.
>>   The message will be computed each time it is needed, and that happens
>> infrequently.
>>   (Even in the serialization case, it will be re-computed from the
>> attached stack frames).
> No, you can not recompute it from the stacktrace because that
> does not contain the BCI. Also, after deserialization, the bytecode
> might look different or not available at all. It depends on the actual
> bytecode that has been running when the exception was thrown.
Right, I realized this too when thinking about it over the weekend.

If a suitable low impact mechanism can't be found, then just
go back to not exposing the extended message and use only the original.
Its a bad idea to twist and contort the local design and accessibility
just for serialization. What remote or delayed client needs to know this?

>
>>   And it avoids adding setMessage() to Throwable, that's significant
>> change since
>>   the current implementation only allows the message to be initialized
>> not updated.
>>   Immutable is an important characteristic to maintain.
> Yes, I don't like I have to set this. But it follows the same pattern
> as with the stack trace which is only computed on demand. Thus,
> Throwable is not immutable anyways. Before, I implemented this by a
> JNI call hiding this in the Java code.
> I proposed implementing setting the field by reflection, Christoph
> proposed a shared secret. David favored the package private setter.
> Please advice what is best.
All of them are bad. Private needs to mean private.

And making it mutable, also means that synchronization or volatile
is needed to ensure a consistent value for getMessage().
That turns into a performance issue for all throw

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Mandy Chung]

On 2/8/19 11:46 AM, Roger Riggs wrote:

Hi,

A few higher level issues should be considered, though the details
of the webrev captured my immediate attention.

Is this the right feature and is this the right level of implementation 
(C++/native)?

:
How much of this can be done in Java code with StackWalker and other 
java APIs?

It would be a shame to add this much native code if there was a more robust
way to implement it using APIs with more leverage.


Improving the NPE message for better diagnosability is helpful while
I share the same concern Roger raised.

Implementing this feature in Java and the library would be a better
choice as this isn't absolutely required to be done in VM in native.

NPE keeps a backtrace capturing the method id and bci of each stack
frame.  One option to explore is to have StackWalker to accept a
Throwable object that returns a stream of StackFrame which allows
you to get the method and BCI and also code source (I started a
prototype for JDK-8189752 some time ago).  It can use the bytecode
library e.g. ASM to read the bytecode.  For NPE message, you can
implement a specialized StackFrameTraverser just for building
an exception message purpose.

Mandy

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Roger Riggs]

Hi,

On 02/12/2019 08:14 AM, Lindenmaier, Goetz wrote:

Hi Roger,

thanks for looking at my change!


A few higher level issues should be considered, though the details of
the webrev captured my immediate attention.

Is this the right feature

Yes!!

Maybe, that's what debuggers are for.



  and is this the right level of implementation (C++/native)?

See below.

Maybe 10 years ago, when native was the only solution.
Now there are tools to analyze bytecode in java.



  From a security perspective, adding field names to exceptions exposes
information to callers that they could not otherwise get and that breaks
encapsulation.
That needs to be evaluated.

I can not comment on that. How can I trigger an evaluation of this?
Who needs to evaluate this?


I think there are ways to make this easier to implement and be easier to
maintain and perform better.

NullPointerException:
28: unused import of Field

Removed


64: The lazyComputeMessage boolean should be inverted so it does not
require
     initialization, false is the default, anything else adds overhead.
     And it may not be needed.

Peter Levart proposed to initialize the message with a sentinel instead.
I'll implement this as a proposal.

That's an extra overhead too, any assignment is.



91: Why do you think synchonization is necessary?  It is a performance hit.
     It is highly unlikely to be called from multiple threads and the
value will
     be the same whether it is computed once or multiple times by
different threads.

I guess we can go without.
It would be possible to construct a case where two threads
do getMessage() on the same NPE Object but the string is not
the same.
Really!, if the bci is the same, the bytecode is the same, what could be 
different

that would change the data used to create the exception?



99: extendedMessage will never be null (so says the javadoc of
getExtendedNPEMessage)
    Bug: If it does return null, then null is returned from getMessage
    regardless of the value of super.getMessage().

Fixed.


121: Simplify getExtendedNPEMessage by making it only responsible for
the detail
    and not concatenation with an existing message.  Put that in
getMessage().

Fixed. You are right, I only call this when the message is NULL.
  

    Its not strictly necessary to change the updated message with
setMessage().
    Avoiding that will avoid complexity and a performance hit.
    The message will be computed each time it is needed, and that happens
infrequently.
    (Even in the serialization case, it will be re-computed from the
attached stack frames).

No, you can not recompute it from the stacktrace because that
does not contain the BCI. Also, after deserialization, the bytecode
might look different or not available at all. It depends on the actual
bytecode that has been running when the exception was thrown.

Right, I realized this too when thinking about it over the weekend.

If a suitable low impact mechanism can't be found, then just
go back to not exposing the extended message and use only the original.
Its a bad idea to twist and contort the local design and accessibility
just for serialization. What remote or delayed client needs to know this?




    And it avoids adding setMessage() to Throwable, that's significant
change since
    the current implementation only allows the message to be initialized
not updated.
    Immutable is an important characteristic to maintain.

Yes, I don't like I have to set this. But it follows the same pattern
as with the stack trace which is only computed on demand. Thus,
Throwable is not immutable anyways.  Before, I implemented this by a
JNI call hiding this in the Java code.
I proposed implementing setting the field by reflection, Christoph
proposed a shared secret.  David favored the package private setter.
Please advice what is best.

All of them are bad. Private needs to mean private.

And making it mutable, also means that synchronization or volatile
is needed to ensure a consistent value for getMessage().
That turns into a performance issue for all throwables.
None of the above.
  

  That makes the writeReplace() unnecessary too.

No. I need this, see above.

See above, but is not essential to the core feature.



Additional command line arguments are an unnecessary complexity,
documentation, and maintenance overhead.  If the feature is as valuable as
you suggest, it should be enabled all the time.

Removed.


I'm assuming there are no tests broken by the modification of the message.
It is likely that somewhere an application or test looks at the message
itself.
Has that been verified?

Our nightly testing runs the headless jdk and hostspot jtreg tests, jck tests 
and some
other applications. They are all unaffected by this change after adapting the
message in jtreg/vmTestbase/jit/t/t104/t104.gold.
(win-x86_64, linux: ppc64, ppc64le, s390, x86_64, aarch, aix, solaris-sparc, 
mac)
Also, I modified quite some messages before which didn't cause any follow-up
breakages to my 

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Daniel, 

> public int getByteCodeIndex(); [1]
thanks for this info. I missed that.

> But I am not sure how you would use that in this case.
> I mean - IIUC the stack would have to be walked when the exception
> is raised - not when getMessage() is called. It's possible that
Well, I will have to call StackWalker in the constructor and remember 
the data I need.

> It's possible that
> the BCI is encoded in the backtrace data though. I have not
> checked.
Yes it is. That is what I'm using currently, in the C++ implementation.
See get_method_and_bci in javaClasses.cpp.

Best regards,
  Goetz.

> -Original Message-
> From: Daniel Fuchs 
> Sent: Dienstag, 12. Februar 2019 14:55
> To: Lindenmaier, Goetz ; Roger Riggs
> ; Java Core Libs 
> Cc: hotspot-runtime-...@openjdk.java.net
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> 
> On 12/02/2019 14:14, Lindenmaier, Goetz wrote:
> > StackWalker operates on the Java stack tracke, which does not contain the
> BCI
> > to my knowledge.
> 
> For what it's worth, StackWalker::StackFrame has the BCI: returned by
> 
> public int getByteCodeIndex(); [1]
> 
> But I am not sure how you would use that in this case.
> I mean - IIUC the stack would have to be walked when the exception
> is raised - not when getMessage() is called. It's possible that
> the BCI is encoded in the backtrace data though. I have not
> checked.
> 
> best regards,
> 
> -- daniel
> 
> [1]
> https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Stac
> kWalker.StackFrame.html#getByteCodeIndex()

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Daniel Fuchs]

Hi Goetz,

On 12/02/2019 14:14, Lindenmaier, Goetz wrote:

StackWalker operates on the Java stack tracke, which does not contain the BCI
to my knowledge. 


For what it's worth, StackWalker::StackFrame has the BCI: returned by

   public int getByteCodeIndex(); [1]

But I am not sure how you would use that in this case.
I mean - IIUC the stack would have to be walked when the exception
is raised - not when getMessage() is called. It's possible that
the BCI is encoded in the backtrace data though. I have not
checked.

best regards,

-- daniel

[1] 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/StackWalker.StackFrame.html#getByteCodeIndex()

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Roger, 

thanks for looking at my change!

> A few higher level issues should be considered, though the details of
> the webrev captured my immediate attention.
> 
> Is this the right feature
Yes!!

>  and is this the right level of implementation (C++/native)?
See below.

>  From a security perspective, adding field names to exceptions exposes
> information to callers that they could not otherwise get and that breaks
> encapsulation.
> That needs to be evaluated.
I can not comment on that. How can I trigger an evaluation of this?
Who needs to evaluate this?

> I think there are ways to make this easier to implement and be easier to
> maintain and perform better.
>
> NullPointerException:
> 28: unused import of Field
Removed

> 64: The lazyComputeMessage boolean should be inverted so it does not
> require
>     initialization, false is the default, anything else adds overhead.
>     And it may not be needed.
Peter Levart proposed to initialize the message with a sentinel instead.
I'll implement this as a proposal.

> 91: Why do you think synchonization is necessary?  It is a performance hit.
>     It is highly unlikely to be called from multiple threads and the
> value will
>     be the same whether it is computed once or multiple times by
> different threads.
I guess we can go without. 
It would be possible to construct a case where two threads
do getMessage() on the same NPE Object but the string is not 
the same.

> 99: extendedMessage will never be null (so says the javadoc of
> getExtendedNPEMessage)
>    Bug: If it does return null, then null is returned from getMessage
>    regardless of the value of super.getMessage().
Fixed.  

> 121: Simplify getExtendedNPEMessage by making it only responsible for
> the detail
>    and not concatenation with an existing message.  Put that in
> getMessage().
Fixed. You are right, I only call this when the message is NULL.
 
>    Its not strictly necessary to change the updated message with
> setMessage().
>    Avoiding that will avoid complexity and a performance hit.
>    The message will be computed each time it is needed, and that happens
> infrequently.
>    (Even in the serialization case, it will be re-computed from the
> attached stack frames).
No, you can not recompute it from the stacktrace because that
does not contain the BCI. Also, after deserialization, the bytecode
might look different or not available at all. It depends on the actual 
bytecode that has been running when the exception was thrown.

>    And it avoids adding setMessage() to Throwable, that's significant
> change since
>    the current implementation only allows the message to be initialized
> not updated.
>    Immutable is an important characteristic to maintain.
Yes, I don't like I have to set this. But it follows the same pattern
as with the stack trace which is only computed on demand. Thus, 
Throwable is not immutable anyways.  Before, I implemented this by a 
JNI call hiding this in the Java code.  
I proposed implementing setting the field by reflection, Christoph
proposed a shared secret.  David favored the package private setter.
Please advice what is best.
 
>  That makes the writeReplace() unnecessary too.
No. I need this, see above.

> Additional command line arguments are an unnecessary complexity,
> documentation, and maintenance overhead.  If the feature is as valuable as
> you suggest, it should be enabled all the time.
Removed.  

> I'm assuming there are no tests broken by the modification of the message.
> It is likely that somewhere an application or test looks at the message
> itself.
> Has that been verified?
Our nightly testing runs the headless jdk and hostspot jtreg tests, jck tests 
and some
other applications. They are all unaffected by this change after adapting the 
message in jtreg/vmTestbase/jit/t/t104/t104.gold.
(win-x86_64, linux: ppc64, ppc64le, s390, x86_64, aarch, aix, solaris-sparc, 
mac)
Also, I modified quite some messages before which didn't cause any follow-up
breakages to my knowledge.
 
> I can't speak for the hotspot code, but that seems to add a lot of code
> to support
> only this convenience feature.  That's a maintenance overhead and burden.
We uses this code since 2006, it needed only few changes.  I would like to 
contribute a follow up for hidden frames of lambdas.

> The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
> question
> about how useful the information is,  developers should not need to know
> about "slot 1". 
Developers should work with class files with debug information and then
they would get the name printed. If exceptions are thrown in production
code, any information is helpful.  Should I just write "a local"?

> The test output of NullPointerExceptionTest shows
> "java.lang.Object.hashCode(()I)".
> Why is the argument type for Integer shown as "()I";  that's not the
> conventional
> representation of a primitive int.
I already discussed this with David Holmes:

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Roger Riggs]

Hi,

A few higher level issues should be considered, though the details of 
the webrev

captured my immediate attention.

Is this the right feature and is this the right level of implementation 
(C++/native)?


From a security perspective, adding field names to exceptions exposes
information to callers that they could not otherwise get and that breaks 
encapsulation.

That needs to be evaluated.


I think there are ways to make this easier to implement and be easier to 
maintain

and perform better.

NullPointerException:

28: unused import of Field

64: The lazyComputeMessage boolean should be inverted so it does not require
   initialization, false is the default, anything else adds overhead.
   And it may not be needed.

91: Why do you think synchonization is necessary?  It is a performance hit.
   It is highly unlikely to be called from multiple threads and the 
value will
   be the same whether it is computed once or multiple times by 
different threads.


99: extendedMessage will never be null (so says the javadoc of 
getExtendedNPEMessage)

  Bug: If it does return null, then null is returned from getMessage
  regardless of the value of super.getMessage().

121: Simplify getExtendedNPEMessage by making it only responsible for 
the detail
  and not concatenation with an existing message.  Put that in 
getMessage().


  Its not strictly necessary to change the updated message with 
setMessage().

  Avoiding that will avoid complexity and a performance hit.
  The message will be computed each time it is needed, and that happens 
infrequently.
  (Even in the serialization case, it will be re-computed from the 
attached stack frames).


  And it avoids adding setMessage() to Throwable, that's significant 
change since
  the current implementation only allows the message to be initialized 
not updated.

  Immutable is an important characteristic to maintain.

  That makes the writeReplace() unnecessary too.

Additional command line arguments are an unnecessary complexity,
documentation, and maintenance overhead.  If the feature is as valuable as
you suggest, it should be enabled all the time.

I'm assuming there are no tests broken by the modification of the message.
It is likely that somewhere an application or test looks at the message 
itself.

Has that been verified?

I can't speak for the hotspot code, but that seems to add a lot of code 
to support

only this convenience feature.  That's a maintenance overhead and burden.

The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a 
question

about how useful the information is,  developers should not need to know
about "slot 1".

The test output of NullPointerExceptionTest shows 
"java.lang.Object.hashCode(()I)".
Why is the argument type for Integer shown as "()I";  that's not the 
conventional

representation of a primitive int.

Generally, the explanations are too verbose.
Method names and field names would be easier to recognize if they were
quoted, as is done with 'this'.  Argument numbers should be digits,
not English words (first, second, etc) to make them easier to pick out.
And has it limits that do not read easily, e.g. "nr. 9".
I would not describe 'this' as a local variable.

Tests, especially new tests should compile without warnings.
NullPointerExceptionTest.java:125 newInstance has been deprecated.

The messages can be easier to understand, e.g.
'field a1 is null, trying to invoke a1.hashCode...' instead of:

"while trying to invoke the method java.lang.Object.hashCode(()I) on a 
null reference loaded from local variable 'a1'"


It will easier to understand if it looks more like the code.
BTW, what does this look like for fully optimized code?
Does it bear any resemblance to the source code at all?
Or does it have to be deoptimized to come up with a sensible source 
reference.


How much of this can be done in Java code with StackWalker and other 
java APIs?

It would be a shame to add this much native code if there was a more robust
way to implement it using APIs with more leverage.

Regards, Roger


On 02/08/2019 09:13 AM, Langer, Christoph wrote:

Hi Goetz,


ok, so here a new webrev just adding a setter for the field:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02

... and incorporating the other comments.

Looks better 

I have a few additions to 
src/java.base/share/classes/java/lang/NullPointerException.java:

28 import java.lang.reflect.Field;
29
-> can be removed now.

91 synchronized (this) {
-> I think this is not needed here. The transient modifier for 
lazyComputeMessage already implies the lock on the Object, I think.

108 return extendedMessage;
-> I guess it would be more correct if you returned super.getMessage() here.

Thanks
Christoph

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Roger Riggs]

Hi Goetz,


On 02/08/2019 06:51 AM, Lindenmaier, Goetz wrote:
... 

though I think trying to produce signatures within the message is going
to be very awkward in the general case. The key part is the "method
NPE.b() ... returned from NPE.a()"

Actually, I have left out code that changes the signatures to the
Java source code wording. I already left that out in my former
exception message contributions.  For example see the messages in
jtreg/runtime/exceptionMsgs/IllegalAccessError/IllegalAccessErrorTest.java,
they have the same bad format:
"class test.Runner4 tried to access private method 
test.IllegalAccessErrorTest.iae4_m()V"

I would like to fix them altogether in a follow-up, is that acceptable to
you?
  


Please no partial solutions, it all needs to be complete.

I'll take a look at it today.

Thanks, Roger

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Peter Levart]

...just one thing if you go that route. Make sure to initialize the
NPE_MESSAGE_PENDING to a new String("something") or else you may be
sharing this constant reference with somebody else via string
interning...

2019-02-08 16:01 GMT+01.00, Peter Levart :
> Hi Goetz,
>
> In NPE:
>
>  97 String extendedMessage =
> getExtendedNPEMessage(super.getMessage());
>
> ...do you expect super.getMessage() to return anything else than null?
> Wouldn't that only occur when there was a data race between two
> threads observing that lazyComputeMessage is strill true in the
> synchronized block before that and then one thread proceeding to
> compute and set the message while the other reading the set message
> via data race? Or are you planing to replace default message in some
> cases with computed one?
>
> I would just pass null there or even get rid of this parameter if that
> is not the case.
>
> If you do that, there is an alternative to having a boolean field in
> NPE. You could create a private static final String constant, call it
> NPE_MESSAGE_PENDING for example and pass it to super constructor in
> NPE no-arg constructor. Then instead of testing for
> lazyComputeMessage, you could test for super.getMessage() ==
> NPE_MESSAGE_PENDING...
>
> Not that this buys much space as NPEs are not numbered. Just a thought...
>
>
> Regards, Peter
>
> 2019-02-08 14:39 GMT+01.00, Lindenmaier, Goetz :
>> Hi,
>>
>> ok, so here a new webrev just adding a setter for the field:
>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02
>>
>> ... and incorporating the other comments.
>>
>> Best regards,
>>   Goetz.
>>
>>
>>
>>
>>
>>> -----Original Message-----
>>> From: David Holmes 
>>> Sent: Freitag, 8. Februar 2019 13:31
>>> To: Lindenmaier, Goetz ; hotspot-runtime-
>>> d...@openjdk.java.net; Java Core Libs 
>>> Subject: Re: RFR(L): 8218628: Add detailed message to
>>> NullPointerException
>>> describing what is null.
>>>
>>> Hi Goetz,
>>>
>>> Just one follow up for now:
>>>
>>> >   * Add package visible "void setMessage (String msg)" to Throwable.
>>>
>>> Yes, just use package accessibility to deal with this, no need to jump
>>> through hoops (or the VM :) ).
>>>
>>> Thanks,
>>> David
>>>
>>> On 8/02/2019 9:51 pm, Lindenmaier, Goetz wrote:
>>> > Hi David,
>>> >
>>> >> Hi Volker,
>>> > ... I assume Volker could have contributed this as well, but actually
>>> > I must mention Ralf Schmelter as the original author of this :)
>>> >
>>> >> You know I'm not going to be a big fan of this :), but as long as we
>>> >> don't pay for it if we don't want it, then that's okay. (I'm still
>>> >> trying to gauge that)
>>> >>
>>> >> I have a little test for this that I ran through your patch:
>>> >>
>>> >> public class NPE {
>>> >> static class B {
>>> >>   C b() { return null; }
>>> >> }
>>> >> static class C {
>>> >>   int c(Object o, String s) {  return 0; }
>>> >> }
>>> >>
>>> >> public static void main(String[] args) {
>>> >>   int x  = a().b().c(d(), e().toString());
>>> >> }
>>> >>
>>> >> static B a() { return null; }
>>> >> static Object d() { return null; }
>>> >> static Object e() { return null; }
>>> >>
>>> >> }
>>> >>
>>> >> and the result was a bit confusing for me:
>>> >>
>>> >> java.lang.NullPointerException: while trying to invoke the method
>>> >> NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)
>>> >>
>>> >> The placement and format of the return type descriptors obfuscates
>>> >> things to me - especially the Lxxx; format. Can we make that more
>>> >> Java
>>> >> programmer friendly eg:
>>> >>
>>> >> "while trying to invoke the method 'NPE$C NPE$B.b()' ..."
>>> >>
>>> >> though I think trying to produce signatures within the message is
>>> >> going
>>> >> to be very awkward in the general case. The key part is the "method
>>> >> NPE.b() ... returned from NPE.a()"
>>>

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Peter Levart]

Hi Goetz,

In NPE:

 97 String extendedMessage = getExtendedNPEMessage(super.getMessage());

...do you expect super.getMessage() to return anything else than null?
Wouldn't that only occur when there was a data race between two
threads observing that lazyComputeMessage is strill true in the
synchronized block before that and then one thread proceeding to
compute and set the message while the other reading the set message
via data race? Or are you planing to replace default message in some
cases with computed one?

I would just pass null there or even get rid of this parameter if that
is not the case.

If you do that, there is an alternative to having a boolean field in
NPE. You could create a private static final String constant, call it
NPE_MESSAGE_PENDING for example and pass it to super constructor in
NPE no-arg constructor. Then instead of testing for
lazyComputeMessage, you could test for super.getMessage() ==
NPE_MESSAGE_PENDING...

Not that this buys much space as NPEs are not numbered. Just a thought...


Regards, Peter

2019-02-08 14:39 GMT+01.00, Lindenmaier, Goetz :
> Hi,
>
> ok, so here a new webrev just adding a setter for the field:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02
>
> ... and incorporating the other comments.
>
> Best regards,
>   Goetz.
>
>
>
>
>
>> -Original Message-
>> From: David Holmes 
>> Sent: Freitag, 8. Februar 2019 13:31
>> To: Lindenmaier, Goetz ; hotspot-runtime-
>> d...@openjdk.java.net; Java Core Libs 
>> Subject: Re: RFR(L): 8218628: Add detailed message to
>> NullPointerException
>> describing what is null.
>>
>> Hi Goetz,
>>
>> Just one follow up for now:
>>
>> >   * Add package visible "void setMessage (String msg)" to Throwable.
>>
>> Yes, just use package accessibility to deal with this, no need to jump
>> through hoops (or the VM :) ).
>>
>> Thanks,
>> David
>>
>> On 8/02/2019 9:51 pm, Lindenmaier, Goetz wrote:
>> > Hi David,
>> >
>> >> Hi Volker,
>> > ... I assume Volker could have contributed this as well, but actually
>> > I must mention Ralf Schmelter as the original author of this :)
>> >
>> >> You know I'm not going to be a big fan of this :), but as long as we
>> >> don't pay for it if we don't want it, then that's okay. (I'm still
>> >> trying to gauge that)
>> >>
>> >> I have a little test for this that I ran through your patch:
>> >>
>> >> public class NPE {
>> >> static class B {
>> >>   C b() { return null; }
>> >> }
>> >> static class C {
>> >>   int c(Object o, String s) {  return 0; }
>> >> }
>> >>
>> >> public static void main(String[] args) {
>> >>   int x  = a().b().c(d(), e().toString());
>> >> }
>> >>
>> >> static B a() { return null; }
>> >> static Object d() { return null; }
>> >> static Object e() { return null; }
>> >>
>> >> }
>> >>
>> >> and the result was a bit confusing for me:
>> >>
>> >> java.lang.NullPointerException: while trying to invoke the method
>> >> NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)
>> >>
>> >> The placement and format of the return type descriptors obfuscates
>> >> things to me - especially the Lxxx; format. Can we make that more Java
>> >> programmer friendly eg:
>> >>
>> >> "while trying to invoke the method 'NPE$C NPE$B.b()' ..."
>> >>
>> >> though I think trying to produce signatures within the message is
>> >> going
>> >> to be very awkward in the general case. The key part is the "method
>> >> NPE.b() ... returned from NPE.a()"
>> > Actually, I have left out code that changes the signatures to the
>> > Java source code wording. I already left that out in my former
>> > exception message contributions.  For example see the messages in
>> > jtreg/runtime/exceptionMsgs/IllegalAccessError/IllegalAccessErrorTest.java,
>> > they have the same bad format:
>> > "class test.Runner4 tried to access private method
>> test.IllegalAccessErrorTest.iae4_m()V"
>> >
>> > I would like to fix them altogether in a follow-up, is that acceptable
>> > to
>> > you?
>> >
>> >> Also "of a null object" would read better as "on a null reference".
>> >

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Langer, Christoph]

Hi Goetz,

> 91 synchronized (this) {
> -> I think this is not needed here. The transient modifier for
> lazyComputeMessage already implies the lock on the Object, I think.

please ignore this comment, I'm wrong here. I confused transient with 
volatile...

/Christoph

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Langer, Christoph]

Hi Goetz,

> ok, so here a new webrev just adding a setter for the field:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02
> 
> ... and incorporating the other comments.

Looks better 

I have a few additions to 
src/java.base/share/classes/java/lang/NullPointerException.java:

28 import java.lang.reflect.Field;
29
-> can be removed now.

91 synchronized (this) {
-> I think this is not needed here. The transient modifier for 
lazyComputeMessage already implies the lock on the Object, I think.

108 return extendedMessage;
-> I guess it would be more correct if you returned super.getMessage() here.

Thanks
Christoph

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi,

ok, so here a new webrev just adding a setter for the field:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/02

... and incorporating the other comments.

Best regards,
  Goetz.





> -Original Message-
> From: David Holmes 
> Sent: Freitag, 8. Februar 2019 13:31
> To: Lindenmaier, Goetz ; hotspot-runtime-
> d...@openjdk.java.net; Java Core Libs 
> Subject: Re: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
> 
> Hi Goetz,
> 
> Just one follow up for now:
> 
> >   * Add package visible "void setMessage (String msg)" to Throwable.
> 
> Yes, just use package accessibility to deal with this, no need to jump
> through hoops (or the VM :) ).
> 
> Thanks,
> David
> 
> On 8/02/2019 9:51 pm, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> >> Hi Volker,
> > ... I assume Volker could have contributed this as well, but actually
> > I must mention Ralf Schmelter as the original author of this :)
> >
> >> You know I'm not going to be a big fan of this :), but as long as we
> >> don't pay for it if we don't want it, then that's okay. (I'm still
> >> trying to gauge that)
> >>
> >> I have a little test for this that I ran through your patch:
> >>
> >> public class NPE {
> >> static class B {
> >>   C b() { return null; }
> >> }
> >> static class C {
> >>   int c(Object o, String s) {  return 0; }
> >> }
> >>
> >> public static void main(String[] args) {
> >>   int x  = a().b().c(d(), e().toString());
> >> }
> >>
> >> static B a() { return null; }
> >> static Object d() { return null; }
> >> static Object e() { return null; }
> >>
> >> }
> >>
> >> and the result was a bit confusing for me:
> >>
> >> java.lang.NullPointerException: while trying to invoke the method
> >> NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)
> >>
> >> The placement and format of the return type descriptors obfuscates
> >> things to me - especially the Lxxx; format. Can we make that more Java
> >> programmer friendly eg:
> >>
> >> "while trying to invoke the method 'NPE$C NPE$B.b()' ..."
> >>
> >> though I think trying to produce signatures within the message is going
> >> to be very awkward in the general case. The key part is the "method
> >> NPE.b() ... returned from NPE.a()"
> > Actually, I have left out code that changes the signatures to the
> > Java source code wording. I already left that out in my former
> > exception message contributions.  For example see the messages in
> > jtreg/runtime/exceptionMsgs/IllegalAccessError/IllegalAccessErrorTest.java,
> > they have the same bad format:
> > "class test.Runner4 tried to access private method
> test.IllegalAccessErrorTest.iae4_m()V"
> >
> > I would like to fix them altogether in a follow-up, is that acceptable to
> > you?
> >
> >> Also "of a null object" would read better as "on a null reference".
> > Makes sense, fixed.
> >
> > But I'm not that sure about changing these:
> > "while trying to read the field '%s' of a null object"
> > --> "while trying to read the field '%s' from a null reference"
> > "while trying to write the field %s of a null object"
> > --> "while trying to write the field %s  of a null reference"
> >
> >> First you will need to file a CSR request for the new product flags.
> > I'm not sure whether I need the product flags altogether. I would
> > prefer removing them.
> >
> >> Second, I don't understand why you need to call into the VM with
> >> JVM_SetDefaultMessage, to set a field in the Java object? Why isn't that
> >> done in Java?
> > Obviously, the problem is that the field is private.
> > As Christoph points out, there are several ways to implement this.
> > Please give advice:
> >* reflection
> >* shared secret
> >* Add package visible "void setMessage (String msg)" to Throwable.
> >
> > Best regards,
> >Goetz
> >
> >
> >>
> >> Thanks,
> >> David
> >>
> >> On 8/02/2019 2:43 am, Lindenmaier, Goetz wrote:
> >>> Hi,
> >>>
> >>> since Java 5, our internal VM reports verbose null pointer exception
> >>> messages. I would like to contribute this feature to OpenJDK.
> >>>
> >>> With this change, messages as
> >>>  "java.lang.NullPointerException: while trying to load from a null int
> array
> >> loaded from local variable 'ia1'"
> >>> are printed.  For more examples see the JBS bug or the test included.
> >>> https://bugs.openjdk.java.net/browse/JDK-8218628
> >>>
> >>> The messages are generated by parsing the bytecodes. For not to have any
> >> overhead when the
> >>> NPE is allocated, the message is only generated when it is accessed by
> >> getMessage() or
> >>> serialization. For this I added a field to NPE to indicate that the 
> >>> message
> still
> >> needs to be
> >>> computed lazily.
> >>>
> >>> Please review:
> >>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
> >>> I'm happy to incorporate your comments.
> >>>
> >>> Best regards,
> >>> Goetz
> >>>
> >>>

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[David Holmes]

Hi Goetz,

Just one follow up for now:

  * Add package visible "void setMessage (String msg)" to Throwable. 


Yes, just use package accessibility to deal with this, no need to jump 
through hoops (or the VM :) ).


Thanks,
David

On 8/02/2019 9:51 pm, Lindenmaier, Goetz wrote:

Hi David,


Hi Volker,

... I assume Volker could have contributed this as well, but actually
I must mention Ralf Schmelter as the original author of this :)
  

You know I'm not going to be a big fan of this :), but as long as we
don't pay for it if we don't want it, then that's okay. (I'm still
trying to gauge that)

I have a little test for this that I ran through your patch:

public class NPE {
static class B {
  C b() { return null; }
}
static class C {
  int c(Object o, String s) {  return 0; }
}

public static void main(String[] args) {
  int x  = a().b().c(d(), e().toString());
}

static B a() { return null; }
static Object d() { return null; }
static Object e() { return null; }

}

and the result was a bit confusing for me:

java.lang.NullPointerException: while trying to invoke the method
NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)

The placement and format of the return type descriptors obfuscates
things to me - especially the Lxxx; format. Can we make that more Java
programmer friendly eg:

"while trying to invoke the method 'NPE$C NPE$B.b()' ..."

though I think trying to produce signatures within the message is going
to be very awkward in the general case. The key part is the "method
NPE.b() ... returned from NPE.a()"

Actually, I have left out code that changes the signatures to the
Java source code wording. I already left that out in my former
exception message contributions.  For example see the messages in
jtreg/runtime/exceptionMsgs/IllegalAccessError/IllegalAccessErrorTest.java,
they have the same bad format:
"class test.Runner4 tried to access private method 
test.IllegalAccessErrorTest.iae4_m()V"

I would like to fix them altogether in a follow-up, is that acceptable to
you?
  

Also "of a null object" would read better as "on a null reference".

Makes sense, fixed.

But I'm not that sure about changing these:
"while trying to read the field '%s' of a null object"
--> "while trying to read the field '%s' from a null reference"
"while trying to write the field %s of a null object"
--> "while trying to write the field %s  of a null reference"


First you will need to file a CSR request for the new product flags.

I'm not sure whether I need the product flags altogether. I would
prefer removing them.
  

Second, I don't understand why you need to call into the VM with
JVM_SetDefaultMessage, to set a field in the Java object? Why isn't that
done in Java?

Obviously, the problem is that the field is private.
As Christoph points out, there are several ways to implement this.
Please give advice:
   * reflection
   * shared secret
   * Add package visible "void setMessage (String msg)" to Throwable.

Best regards,
   Goetz




Thanks,
David

On 8/02/2019 2:43 am, Lindenmaier, Goetz wrote:

Hi,

since Java 5, our internal VM reports verbose null pointer exception
messages. I would like to contribute this feature to OpenJDK.

With this change, messages as
 "java.lang.NullPointerException: while trying to load from a null int array

loaded from local variable 'ia1'"

are printed.  For more examples see the JBS bug or the test included.
https://bugs.openjdk.java.net/browse/JDK-8218628

The messages are generated by parsing the bytecodes. For not to have any

overhead when the

NPE is allocated, the message is only generated when it is accessed by

getMessage() or

serialization. For this I added a field to NPE to indicate that the message 
still

needs to be

computed lazily.

Please review:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
I'm happy to incorporate your comments.

Best regards,
Goetz

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi David, 

> Hi Volker,
... I assume Volker could have contributed this as well, but actually 
I must mention Ralf Schmelter as the original author of this :)
 
> You know I'm not going to be a big fan of this :), but as long as we
> don't pay for it if we don't want it, then that's okay. (I'm still
> trying to gauge that)
> 
> I have a little test for this that I ran through your patch:
> 
> public class NPE {
>static class B {
>  C b() { return null; }
>}
>static class C {
>  int c(Object o, String s) {  return 0; }
>}
> 
>public static void main(String[] args) {
>  int x  = a().b().c(d(), e().toString());
>}
> 
>static B a() { return null; }
>static Object d() { return null; }
>static Object e() { return null; }
> 
> }
> 
> and the result was a bit confusing for me:
> 
> java.lang.NullPointerException: while trying to invoke the method
> NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)
> 
> The placement and format of the return type descriptors obfuscates
> things to me - especially the Lxxx; format. Can we make that more Java
> programmer friendly eg:
> 
> "while trying to invoke the method 'NPE$C NPE$B.b()' ..."
> 
> though I think trying to produce signatures within the message is going
> to be very awkward in the general case. The key part is the "method
> NPE.b() ... returned from NPE.a()"
Actually, I have left out code that changes the signatures to the 
Java source code wording. I already left that out in my former 
exception message contributions.  For example see the messages in 
jtreg/runtime/exceptionMsgs/IllegalAccessError/IllegalAccessErrorTest.java, 
they have the same bad format:
"class test.Runner4 tried to access private method 
test.IllegalAccessErrorTest.iae4_m()V"

I would like to fix them altogether in a follow-up, is that acceptable to 
you?
 
> Also "of a null object" would read better as "on a null reference".
Makes sense, fixed.

But I'm not that sure about changing these:
"while trying to read the field '%s' of a null object"
--> "while trying to read the field '%s' from a null reference"
"while trying to write the field %s of a null object"
--> "while trying to write the field %s  of a null reference"

> First you will need to file a CSR request for the new product flags.
I'm not sure whether I need the product flags altogether. I would 
prefer removing them.
 
> Second, I don't understand why you need to call into the VM with
> JVM_SetDefaultMessage, to set a field in the Java object? Why isn't that
> done in Java?
Obviously, the problem is that the field is private.
As Christoph points out, there are several ways to implement this. 
Please give advice:
  * reflection
  * shared secret
  * Add package visible "void setMessage (String msg)" to Throwable. 

Best regards,
  Goetz


> 
> Thanks,
> David
> 
> On 8/02/2019 2:43 am, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > since Java 5, our internal VM reports verbose null pointer exception
> > messages. I would like to contribute this feature to OpenJDK.
> >
> > With this change, messages as
> > "java.lang.NullPointerException: while trying to load from a null int 
> > array
> loaded from local variable 'ia1'"
> > are printed.  For more examples see the JBS bug or the test included.
> > https://bugs.openjdk.java.net/browse/JDK-8218628
> >
> > The messages are generated by parsing the bytecodes. For not to have any
> overhead when the
> > NPE is allocated, the message is only generated when it is accessed by
> getMessage() or
> > serialization. For this I added a field to NPE to indicate that the message 
> > still
> needs to be
> > computed lazily.
> >
> > Please review:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
> > I'm happy to incorporate your comments.
> >
> > Best regards,
> >Goetz
> >
> >

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Lindenmaier, Goetz]

Hi Christoph, 

thanks for looking at this.

> src/java.base/share/classes/java/lang/NullPointerException.java:
> line 56:
> remove a space before the comment
fixed.

> line 62: private transient boolean lazyComputeDefaultMessage;
> I think the name for this variable is not well chosen. I'd prefer
> lazyComputeMessage, because it's not a default message, it's rather "the"
> message of the exception.
OK, removed 'default'

> Then, I think you should initialize the field right away in this line to 
> true, no
> need to do it in the constructor. (Or can't that be done for transient 
> fields?? )
No, it must be initialized to false because there is a second constructor that 
uses a custom message (that was also the reason for the 'default' in the field 
name, it 
only applies to the constructor without custom message)

> 122 private native void setDefaultMessage(String extendedMessage);
> Here, I agree with David, that you should do it in Java. Just use a shared 
> secret,
> then you don't even need reflection...
> It also should not be "setDefaultMessage" but rather "setMessage".
Obviously, the problem is that the field is private. I see various ways to 
implement this.  Please give advice:
  * reflection
  * shared secret
  * Add package visible "void setMessage (String msg)" to throwable. 

Best regards,
  Goetz.


> 
> Best regards
> Christoph
> 
> 
> > -Original Message-
> > From: hotspot-runtime-dev  > boun...@openjdk.java.net> On Behalf Of Lindenmaier, Goetz
> > Sent: Donnerstag, 7. Februar 2019 17:43
> > To: hotspot-runtime-...@openjdk.java.net; Java Core Libs  > d...@openjdk.java.net>
> > Subject: [CAUTION] RFR(L): 8218628: Add detailed message to
> > NullPointerException describing what is null.
> >
> > Hi,
> >
> > since Java 5, our internal VM reports verbose null pointer exception
> > messages. I would like to contribute this feature to OpenJDK.
> >
> > With this change, messages as
> >"java.lang.NullPointerException: while trying to load from a null int 
> > array
> > loaded from local variable 'ia1'"
> > are printed.  For more examples see the JBS bug or the test included.
> > https://bugs.openjdk.java.net/browse/JDK-8218628
> >
> > The messages are generated by parsing the bytecodes. For not to have any
> > overhead when the
> > NPE is allocated, the message is only generated when it is accessed by
> > getMessage() or
> > serialization. For this I added a field to NPE to indicate that the message 
> > still
> > needs to be
> > computed lazily.
> >
> > Please review:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
> > I'm happy to incorporate your comments.
> >
> > Best regards,
> >   Goetz
> >

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Langer, Christoph]

> 122 private native void setDefaultMessage(String extendedMessage);
> Here, I agree with David, that you should do it in Java. Just use a shared
> secret, then you don't even need reflection...

Addition: you should not even need to use a shared secret as Throwable and NPE 
are in the same package. Just add some package private setter to Throwable's 
detailMessage.

RE: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Langer, Christoph]

Hi Goetz,

thanks for bringing this into OpenJDK finally. I know of people that'll be 
quite happy about this feature.

I had a quick glance through the code. Here are a few findings:

src/java.base/share/classes/java/lang/NullPointerException.java:
line 56:
remove a space before the comment

line 62: private transient boolean lazyComputeDefaultMessage;
I think the name for this variable is not well chosen. I'd prefer 
lazyComputeMessage, because it's not a default message, it's rather "the" 
message of the exception.
Then, I think you should initialize the field right away in this line to true, 
no need to do it in the constructor. (Or can't that be done for transient 
fields?? )

122 private native void setDefaultMessage(String extendedMessage);
Here, I agree with David, that you should do it in Java. Just use a shared 
secret, then you don't even need reflection...
It also should not be "setDefaultMessage" but rather "setMessage".

Best regards
Christoph

 
> -Original Message-
> From: hotspot-runtime-dev  boun...@openjdk.java.net> On Behalf Of Lindenmaier, Goetz
> Sent: Donnerstag, 7. Februar 2019 17:43
> To: hotspot-runtime-...@openjdk.java.net; Java Core Libs  d...@openjdk.java.net>
> Subject: [CAUTION] RFR(L): 8218628: Add detailed message to
> NullPointerException describing what is null.
> 
> Hi,
> 
> since Java 5, our internal VM reports verbose null pointer exception
> messages. I would like to contribute this feature to OpenJDK.
> 
> With this change, messages as
>"java.lang.NullPointerException: while trying to load from a null int array
> loaded from local variable 'ia1'"
> are printed.  For more examples see the JBS bug or the test included.
> https://bugs.openjdk.java.net/browse/JDK-8218628
> 
> The messages are generated by parsing the bytecodes. For not to have any
> overhead when the
> NPE is allocated, the message is only generated when it is accessed by
> getMessage() or
> serialization. For this I added a field to NPE to indicate that the message 
> still
> needs to be
> computed lazily.
> 
> Please review:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
> I'm happy to incorporate your comments.
> 
> Best regards,
>   Goetz
> 

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[David Holmes]

In 8/02/2019 10:05 am, David Holmes wrote:

Hi Volker,


Aaarggh!! Sorry Volker and Goetz. No idea where that came from.

David
-

You know I'm not going to be a big fan of this :), but as long as we 
don't pay for it if we don't want it, then that's okay. (I'm still 
trying to gauge that)


I have a little test for this that I ran through your patch:

public class NPE {
   static class B {
     C b() { return null; }
   }
   static class C {
     int c(Object o, String s) {  return 0; }
   }

   public static void main(String[] args) {
     int x  = a().b().c(d(), e().toString());
   }

   static B a() { return null; }
   static Object d() { return null; }
   static Object e() { return null; }

}

and the result was a bit confusing for me:

java.lang.NullPointerException: while trying to invoke the method 
NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)


The placement and format of the return type descriptors obfuscates 
things to me - especially the Lxxx; format. Can we make that more Java 
programmer friendly eg:


"while trying to invoke the method 'NPE$C NPE$B.b()' ..."

though I think trying to produce signatures within the message is going 
to be very awkward in the general case. The key part is the "method 
NPE.b() ... returned from NPE.a()"


Also "of a null object" would read better as "on a null reference".


A couple of initial comments

First you will need to file a CSR request for the new product flags.

Second, I don't understand why you need to call into the VM with 
JVM_SetDefaultMessage, to set a field in the Java object? Why isn't that 
done in Java?


Thanks,
David

On 8/02/2019 2:43 am, Lindenmaier, Goetz wrote:

Hi,

since Java 5, our internal VM reports verbose null pointer exception
messages. I would like to contribute this feature to OpenJDK.

With this change, messages as
    "java.lang.NullPointerException: while trying to load from a null 
int array loaded from local variable 'ia1'"

are printed.  For more examples see the JBS bug or the test included.
https://bugs.openjdk.java.net/browse/JDK-8218628

The messages are generated by parsing the bytecodes. For not to have 
any overhead when the
NPE is allocated, the message is only generated when it is accessed by 
getMessage() or
serialization. For this I added a field to NPE to indicate that the 
message still needs to be

computed lazily.

Please review:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
I'm happy to incorporate your comments.

Best regards,
   Goetz

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[David Holmes]

Hi Volker,

You know I'm not going to be a big fan of this :), but as long as we 
don't pay for it if we don't want it, then that's okay. (I'm still 
trying to gauge that)


I have a little test for this that I ran through your patch:

public class NPE {
  static class B {
C b() { return null; }
  }
  static class C {
int c(Object o, String s) {  return 0; }
  }

  public static void main(String[] args) {
int x  = a().b().c(d(), e().toString());
  }

  static B a() { return null; }
  static Object d() { return null; }
  static Object e() { return null; }

}

and the result was a bit confusing for me:

java.lang.NullPointerException: while trying to invoke the method 
NPE$B.b(()LNPE$C;) of a null object returned from NPE.a(()LNPE$B;)


The placement and format of the return type descriptors obfuscates 
things to me - especially the Lxxx; format. Can we make that more Java 
programmer friendly eg:


"while trying to invoke the method 'NPE$C NPE$B.b()' ..."

though I think trying to produce signatures within the message is going 
to be very awkward in the general case. The key part is the "method 
NPE.b() ... returned from NPE.a()"


Also "of a null object" would read better as "on a null reference".


A couple of initial comments

First you will need to file a CSR request for the new product flags.

Second, I don't understand why you need to call into the VM with 
JVM_SetDefaultMessage, to set a field in the Java object? Why isn't that 
done in Java?


Thanks,
David

On 8/02/2019 2:43 am, Lindenmaier, Goetz wrote:

Hi,

since Java 5, our internal VM reports verbose null pointer exception
messages. I would like to contribute this feature to OpenJDK.

With this change, messages as
"java.lang.NullPointerException: while trying to load from a null int array 
loaded from local variable 'ia1'"
are printed.  For more examples see the JBS bug or the test included.
https://bugs.openjdk.java.net/browse/JDK-8218628

The messages are generated by parsing the bytecodes. For not to have any 
overhead when the
NPE is allocated, the message is only generated when it is accessed by 
getMessage() or
serialization. For this I added a field to NPE to indicate that the message 
still needs to be
computed lazily.

Please review:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/01/
I'm happy to incorporate your comments.

Best regards,
   Goetz