RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi everybody, JEP 358 is targeted to jdk14. If there are no objections, I'll push this change tomorrow or on Wednesday. I'll do one final pass of jdk/submit before pushing. The final webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/21/ Best regards, Goetz. > -Original Message- > From: mark.reinh...@oracle.com > Sent: Donnerstag, 3. Oktober 2019 00:41 > To: Lindenmaier, Goetz > Cc: hotspot-runtime-...@openjdk.java.net; core-libs-dev@openjdk.java.net > Subject: Re: RFR (L, final): 8218626: Add detailed message to > NullPointerException describing what is null. > > 2019/10/2 5:45:10 -0700, goetz.lindenma...@sap.com: > > thanks for looking at my change! Can I add you as reviewer? > > Sure. > > >> This is very nice work! I especially appreciate the thorough tests. > > > > Thanks! But adapting the many tests to changed messages is > > quite cumbersome :) Thanks for supplying the patch right away! > > Nothing that a little Emacs-fu can’t handle. > > >> ... > >> > >> I also noticed that the generated messages use single quotes (‘'’) to > >> quote the names of fields, etc., rather than double quotes (‘"’). > > > > I'm fine with this, but I think hotspot uses "'" for citing code > > quite consistently. E.g., have a look at > > > http://hg.openjdk.java.net/jdk/jdk/file/b25362cec8ce/src/hotspot/share/inter > preter/linkResolver.cpp > > This can easily be changed. I could do a separate change > > for hotspot and change all uses of ' in exceptions to ". What > > do you think? > > I think that’d be a fine clean-up task, for later. > > - Mark
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
2019/10/2 5:45:10 -0700, goetz.lindenma...@sap.com: > thanks for looking at my change! Can I add you as reviewer? Sure. >> This is very nice work! I especially appreciate the thorough tests. > > Thanks! But adapting the many tests to changed messages is > quite cumbersome :) Thanks for supplying the patch right away! Nothing that a little Emacs-fu can’t handle. >> ... >> >> I also noticed that the generated messages use single quotes (‘'’) to >> quote the names of fields, etc., rather than double quotes (‘"’). > > I'm fine with this, but I think hotspot uses "'" for citing code > quite consistently. E.g., have a look at > http://hg.openjdk.java.net/jdk/jdk/file/b25362cec8ce/src/hotspot/share/interpreter/linkResolver.cpp > This can easily be changed. I could do a separate change > for hotspot and change all uses of ' in exceptions to ". What > do you think? I think that’d be a fine clean-up task, for later. - Mark
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Mark, thanks for looking at my change! Can I add you as reviewer? This webrev incorporates your patch as well as two fixes of issues that sneaked in implementing some recent reviews: bytecodeUtils.cpp:449 1 --> 1ULL bytecodeUtils.cpp:460 move assertion to where len is known. http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/21-incremental/ http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/21/ > This is very nice work! I especially appreciate the thorough tests. Thanks! But adapting the many tests to changed messages is quite cumbersome :) Thanks for supplying the patch right away! > Looking at the tests, and the details of the JEP, I noticed that the > generated messages all end in a period (e.g., “... is null.”). This > is pretty unusual in the JDK and jarring to the eye Actually, for me as a foreign speaker, proper punctuation makes it easier to understand. Reading a statement without '.' makes me feel it's incomplete. Intuitively, I try to complete it somehow if the grammar does not make it obvious that it is a complete sentence. But I will follow your advice here if no one else objects the other way. > I also noticed that the generated messages use single quotes (‘'’) to > quote the names of fields, etc., rather than double quotes (‘"’). I'm fine with this, but I think hotspot uses "'" for citing code quite consistently. E.g., have a look at http://hg.openjdk.java.net/jdk/jdk/file/b25362cec8ce/src/hotspot/share/interpreter/linkResolver.cpp This can easily be changed. I could do a separate change for hotspot and change all uses of ' in exceptions to ". What do you think? Best regards, Goetz.
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
2019/9/24 1:13:14 -0700, goetz.lindenma...@sap.com:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/19/
This is very nice work! I especially appreciate the thorough tests.
Looking at the tests, and the details of the JEP, I noticed that the
generated messages all end in a period (e.g., “... is null.”). This
is pretty unusual in the JDK and jarring to the eye, except in the
rare cases in which an exception message is composed of multiple
distinct sentences. I’m surprised that no one noticed this before.
As a data point, of the 17,999 messages that I was able to find in
invocations of exception constructors in the JDK 14 library code just
now, only 998 of them end in periods. (I didn’t look at HotSpot, which
is trickier to grep for such things.)
So, I suggest you drop the trailing periods.
I also noticed that the generated messages use single quotes (‘'’) to
quote the names of fields, etc., rather than double quotes (‘"’).
On this choice, our corpus is less instructive: Of the 17,999 messages
I gathered, 446 of them use quotes of some sort, 227 of those use double
quotes, and 219 use single quotes.
Even so, I’ll propose here that double quotes are preferable: They’re
consistent with the Java language itself, they’re consistent with common
usage in both American and British English, and they’re less apt to be
confused with single quotes used as apostrophes (e.g., "MemoryHandler
can't load handler target \"" + targetName + "\"").
So, I also suggest that you change the single quotes to double quotes.
Fortunately, and because you have such good tests, this is pretty easy
to do. Attached is a patch against your patch that makes these changes,
and after which all the tests still pass.
- Mark
# HG changeset patch
# User mr
# Date 1569968713 25200
# Tue Oct 01 15:25:13 2019 -0700
# Node ID d7d49b8c3c44c527c1b60a590228259c02377738
# Parent 06c12a39584c579e9b9cd9d6bbba01a88b9ff764
Remove periods, and double quotes, for 8218628
diff --git a/src/hotspot/share/interpreter/bytecodeUtils.cpp b/src/hotspot/share/interpreter/bytecodeUtils.cpp
--- a/src/hotspot/share/interpreter/bytecodeUtils.cpp
+++ b/src/hotspot/share/interpreter/bytecodeUtils.cpp
@@ -1168,8 +1168,8 @@
}
bool ExceptionMessageBuilder::print_NPE_cause(outputStream* os, int bci, int slot) {
- if (print_NPE_cause0(os, bci, slot, _max_cause_detail, false, " because '")) {
-os->print("' is null.");
+ if (print_NPE_cause0(os, bci, slot, _max_cause_detail, false, " because \"")) {
+os->print("\" is null");
return true;
}
return false;
@@ -1346,7 +1346,7 @@
case Bytecodes::_invokeinterface: {
int cp_index = Bytes::get_native_u2(code_base + pos) DEBUG_ONLY(+ ConstantPool::CPCACHE_INDEX_TAG);
if (max_detail == _max_cause_detail && !inner_expr) {
-os->print(" because the return value of '");
+os->print(" because the return value of \"");
}
print_method_name(os, _method, cp_index);
return true;
@@ -1417,19 +1417,19 @@
int name_and_type_index = cp->name_and_type_ref_index_at(cp_index);
int name_index = cp->name_ref_index_at(name_and_type_index);
Symbol* name = cp->symbol_at(name_index);
-os->print("Cannot read field '%s'", name->as_C_string());
+os->print("Cannot read field \"%s\"", name->as_C_string());
} break;
case Bytecodes::_putfield: {
int cp_index = Bytes::get_native_u2(code_base + pos) DEBUG_ONLY(+ ConstantPool::CPCACHE_INDEX_TAG);
-os->print("Cannot assign field '%s'", get_field_name(_method, cp_index));
+os->print("Cannot assign field \"%s\"", get_field_name(_method, cp_index));
} break;
case Bytecodes::_invokevirtual:
case Bytecodes::_invokespecial:
case Bytecodes::_invokeinterface: {
int cp_index = Bytes::get_native_u2(code_base+ pos) DEBUG_ONLY(+ ConstantPool::CPCACHE_INDEX_TAG);
-os->print("Cannot invoke '");
+os->print("Cannot invoke \"");
print_method_name(os, _method, cp_index);
-os->print("'");
+os->print("\"");
} break;
default:
@@ -1474,7 +1474,6 @@
if (!emb.print_NPE_cause(ss, bci, slot)) {
// Nothing was printed. End the sentence without the 'because'
// subordinate sentence.
- ss->print(".");
}
}
return true;
diff --git a/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java b/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java
--- a/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java
+++ b/test/hotspot/jtreg/runtime/exceptionMsgs/NullPointerException/NPEInHiddenTopFrameTest.java
@@ -77,7 +77,7 @@
null :
// This is the correct message, but it describes code generated on-the-fly.
// You get it if you disable hiding frames (-XX:+ShowHiddenframes).
-
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Thanks, Ralf! Best regards, Goetz. > -Original Message- > From: Schmelter, Ralf > Sent: Dienstag, 1. Oktober 2019 10:11 > To: Lindenmaier, Goetz ; Hotspot dev runtime > ; Java Core Libs d...@openjdk.java.net> > Subject: RE: RFR (L, final): 8218626: Add detailed message to > NullPointerException describing what is null. > > Hi Goetz, > > thanks for the additional test. Looks good. > > Best regards, > Ralf > > -Original Message- > From: Lindenmaier, Goetz > Sent: Montag, 30. September 2019 18:31 > To: Schmelter, Ralf ; Hotspot dev runtime runtime-...@openjdk.java.net>; Java Core Libs d...@openjdk.java.net> > Subject: RE: RFR (L, final): 8218626: Add detailed message to > NullPointerException describing what is null. > > Hi Ralf, > > > The test should not omit these two bytecodes because the current > > implementation is the same. This can change and it is not much additional > code > > to add the two cases. > I implemented test cases for the missing invokes: > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20-incremental/ > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20/ > > Best regards, > Goetz. > >
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz, thanks for the additional test. Looks good. Best regards, Ralf -Original Message- From: Lindenmaier, Goetz Sent: Montag, 30. September 2019 18:31 To: Schmelter, Ralf ; Hotspot dev runtime ; Java Core Libs Subject: RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null. Hi Ralf, > The test should not omit these two bytecodes because the current > implementation is the same. This can change and it is not much additional code > to add the two cases. I implemented test cases for the missing invokes: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20-incremental/ http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20/ Best regards, Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Ralf, > The test should not omit these two bytecodes because the current > implementation is the same. This can change and it is not much additional code > to add the two cases. I implemented test cases for the missing invokes: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20-incremental/ http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/20/ Best regards, Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Thanks!
Best regards,
Goetz
> -Original Message-
> From: Roger Riggs
> Sent: Dienstag, 24. September 2019 15:54
> To: Lindenmaier, Goetz ; Hotspot dev runtime
> ; Java Core Libs d...@openjdk.java.net>
> Subject: Re: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> Hi Goetz,
>
> Looks good.
> Count me as a (java) Reviewer.
>
> Thanks, Roger
>
>
> On 9/24/19 4:13 AM, Lindenmaier, Goetz wrote:
>
>
> Hi Roger,
>
> thanks for improving the text! Good point to add
> @implNote.
> This webrev includes the fixed comments:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/19/
> Is it ok to add you as reviewer (for the java.base part)?
>
> Best regards,
> Goetz.
>
>
>
> -Original Message-
> From: Roger Riggs
> <mailto:roger.ri...@oracle.com>
> Sent: Montag, 23. September 2019 17:30
> To: Lindenmaier, Goetz
> <mailto:goetz.lindenma...@sap.com> ; Hotspot dev runtime
><mailto:hotspot-
> runtime-...@openjdk.java.net> ; Java Core Libsd...@openjdk.java.net <mailto:d...@openjdk.java.net> >
> Subject: Re: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> Hi Goetz,
>
> A bit of wordsmithing on the javadoc of
> NullPointerException.getMessage
> and separating out the implementation specific description to
> an @implNote
>
>
> 75:
> /**
>* Returns the detail message string of this throwable.
>*
>* If a non-null message was supplied in a constructor it is
> returned.
>* Otherwise, an implementation specific message or {@code
> null} is
> returned.
>* @ImplNote
>* If no explicit message was passed to the constructor, and
> as
>* long as certain internal information is available, a
> verbose
>* description of the null reference is returned.
>* The internal information is not available in deserialized
> NullPointerExceptions.
>*
>* @return the detail message string, which may be {@code
> null}.
>*
> 94-97: The comment on the getExtendedNPEMessage should
> use the normal
> /**... */ syntax.
>
> Thanks, Roger
>
>
>
>
> On 9/17/19 10:18 AM, Lindenmaier, Goetz wrote:
>
>
> @core-libs experts, I would appreciate comments on
> the changes
> to NullPointerException.java, especially wrt. the
> Javadoc comment.
> The change there is S.
>
> Best regards,
> Goetz.
>
>
> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Dienstag, 10. September 2019 11:48
> To: 'Hotspot dev runtime'd...@openjdk.java.net <mailto:d...@openjdk.java.net> >
> <mailto:hotspot-runtime-...@openjdk.java.net> <mailto:hotspot-runtime-
> d...@openjdk.java.net> ; Java
> Core
> Libs
> <mailto:core-libs-dev@openjdk.java.net> <mailto:core-libs-
> d...@openjdk.java.net> <mailto:core-libs-
> d...@openjdk.java.net>
> Subject: RFR (L, final): 8218626: Add detailed
> message to
> NullPointerException
> describing what is null.
>
> Hi,
>
>
>
> this is the implementation of JEP 358: Helpful
> NullPointerExceptions.
>
>
>
> The JEP is in status "Candidate". Coleen (many,
> many thanks!)
> ran
>
> it through the Oracle-internal processes. Now I
> please need
> final reviews
>
> for this change so that I can propose it to
> target jdk 14.
>
>
>
> JEP:
> https://bugs.openjdk.java.net/browse/JDK-8220715
>
> Enhancement:
> https://bu
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz,
Looks good.
Count me as a (java) Reviewer.
Thanks, Roger
On 9/24/19 4:13 AM, Lindenmaier, Goetz wrote:
Hi Roger,
thanks for improving the text! Good point to add
@implNote.
This webrev includes the fixed comments:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/19/
Is it ok to add you as reviewer (for the java.base part)?
Best regards,
Goetz.
-Original Message-
From: Roger Riggs
Sent: Montag, 23. September 2019 17:30
To: Lindenmaier, Goetz ; Hotspot dev runtime
; Java Core Libs
Subject: Re: RFR (L, final): 8218626: Add detailed message to
NullPointerException describing what is null.
Hi Goetz,
A bit of wordsmithing on the javadoc of NullPointerException.getMessage
and separating out the implementation specific description to an @implNote
75:
/**
* Returns the detail message string of this throwable.
*
* If a non-null message was supplied in a constructor it is returned.
* Otherwise, an implementation specific message or {@code null} is
returned.
* @ImplNote
* If no explicit message was passed to the constructor, and as
* long as certain internal information is available, a verbose
* description of the null reference is returned.
* The internal information is not available in deserialized
NullPointerExceptions.
*
* @return the detail message string, which may be {@code null}.
*
94-97: The comment on the getExtendedNPEMessage should use the normal
/**... */ syntax.
Thanks, Roger
On 9/17/19 10:18 AM, Lindenmaier, Goetz wrote:
@core-libs experts, I would appreciate comments on the changes
to NullPointerException.java, especially wrt. the Javadoc comment.
The change there is S.
Best regards,
Goetz.
-Original Message-
From: Lindenmaier, Goetz
Sent: Dienstag, 10. September 2019 11:48
To: 'Hotspot dev runtime' <mailto:hotspot-runtime-...@openjdk.java.net> ; Java
Core
Libs <mailto:core-libs-
d...@openjdk.java.net>
Subject: RFR (L, final): 8218626: Add detailed message to
NullPointerException
describing what is null.
Hi,
this is the implementation of JEP 358: Helpful
NullPointerExceptions.
The JEP is in status "Candidate". Coleen (many, many thanks!)
ran
it through the Oracle-internal processes. Now I please need
final reviews
for this change so that I can propose it to target jdk 14.
JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
Enhancement: https://bugs.openjdk.java.net/browse/JDK-
8218628
webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-
exMsg-NPE/16/
The change ran through a lot of testing, all jtreg and jck
tests to
name
only some. The webrev points to patch
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
NPE/16/enable_NPE_message.patch
that enables the change by default, which was useful for
testing to
assure the code is used in the tests.
I just pushed the change to jdk/submit once more.
Please review.
Thanks!
Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz, just one thing: >> In NullPointerExceptionTest.java: > > > > It seems you don't have tests for invokeinterface or invokespecial calls to > > cause > > an NPE (e.g. by calling a null interface variable or a private non-static > > method > > of a null objects). > That is because the code is the same as for invokevirtual in all places in > bytecodeUtils.cpp. The test should not omit these two bytecodes because the current implementation is the same. This can change and it is not much additional code to add the two cases. Thanks and best regards, Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Roger,
thanks for improving the text! Good point to add
@implNote.
This webrev includes the fixed comments:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/19/
Is it ok to add you as reviewer (for the java.base part)?
Best regards,
Goetz.
> -Original Message-
> From: Roger Riggs
> Sent: Montag, 23. September 2019 17:30
> To: Lindenmaier, Goetz ; Hotspot dev runtime
> ; Java Core Libs d...@openjdk.java.net>
> Subject: Re: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> Hi Goetz,
>
> A bit of wordsmithing on the javadoc of NullPointerException.getMessage
> and separating out the implementation specific description to an @implNote
>
>
> 75:
> /**
> * Returns the detail message string of this throwable.
> *
> * If a non-null message was supplied in a constructor it is returned.
> * Otherwise, an implementation specific message or {@code null} is
> returned.
> * @ImplNote
> * If no explicit message was passed to the constructor, and as
> * long as certain internal information is available, a verbose
> * description of the null reference is returned.
> * The internal information is not available in deserialized
> NullPointerExceptions.
> *
> * @return the detail message string, which may be {@code null}.
> *
> 94-97: The comment on the getExtendedNPEMessage should use the normal
> /**... */ syntax.
>
> Thanks, Roger
>
>
>
>
> On 9/17/19 10:18 AM, Lindenmaier, Goetz wrote:
>
>
> @core-libs experts, I would appreciate comments on the changes
> to NullPointerException.java, especially wrt. the Javadoc comment.
> The change there is S.
>
> Best regards,
> Goetz.
>
>
> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Dienstag, 10. September 2019 11:48
> To: 'Hotspot dev runtime' d...@openjdk.java.net> <mailto:hotspot-runtime-...@openjdk.java.net> ; Java
> Core
> Libs <mailto:core-libs-
> d...@openjdk.java.net>
> Subject: RFR (L, final): 8218626: Add detailed message to
> NullPointerException
> describing what is null.
>
> Hi,
>
>
>
> this is the implementation of JEP 358: Helpful
> NullPointerExceptions.
>
>
>
> The JEP is in status "Candidate". Coleen (many, many thanks!)
> ran
>
> it through the Oracle-internal processes. Now I please need
> final reviews
>
> for this change so that I can propose it to target jdk 14.
>
>
>
> JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
>
> Enhancement: https://bugs.openjdk.java.net/browse/JDK-
> 8218628
>
> webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-
> exMsg-NPE/16/
>
>
>
> The change ran through a lot of testing, all jtreg and jck
> tests to
> name
>
> only some. The webrev points to patch
>
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> NPE/16/enable_NPE_message.patch
>
> that enables the change by default, which was useful for
> testing to
>
> assure the code is used in the tests.
>
> I just pushed the change to jdk/submit once more.
>
>
>
> Please review.
>
>
>
> Thanks!
>
> Goetz.
>
>
>
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Remi,
thanks for the heads up, I incorporated it in the
main webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/19/
Best regards,
Goetz.
> -Original Message-
> From: fo...@univ-mlv.fr
> Sent: Montag, 23. September 2019 18:02
> To: Lindenmaier, Goetz
> Cc: hotspot-runtime-dev ; core-libs-
> dev
> Subject: Re: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> - Mail original -
> > De: "Goetz Lindenmaier"
> > À: "Remi Forax"
> > Cc: "hotspot-runtime-dev" , "core-
> libs-dev"
> > Envoyé: Lundi 23 Septembre 2019 12:03:30
> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> > Hi Remi,
>
> Hi Goetz,
>
> >
> > what do you think about dealing with the problem like this:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18-
> obfuscation/
> > It's at the cost of one 64-bit field per bytecode in the analysis data.
> > Also, if there is a real assignment to a parameter it's not named
> > 'parameteri'
> > after that any more. See the example in the test.
> >
>
> yes, it's a nice approximation, having a method with more than 63/64
> parameters is rare.
>
> patch looks good, thumbs up !
>
> > Best regards,
> > Goetz.
> >
>
> regards,
> Rémi
>
> >
> >
> >> -Original Message-
> >> From: fo...@univ-mlv.fr
> >> Sent: Freitag, 20. September 2019 00:01
> >> To: Lindenmaier, Goetz
> >> Cc: hotspot-runtime-dev ; core-
> libs-
> >> dev
> >> Subject: Re: RFR (L, final): 8218626: Add detailed message to
> >> NullPointerException describing what is null.
> >>
> >> - Mail original -
> >> > De: "Goetz Lindenmaier"
> >> > À: "Remi Forax"
> >> > Cc: "hotspot-runtime-dev" ,
> "core-
> >> libs-dev"
> >> > Envoyé: Mercredi 18 Septembre 2019 09:37:36
> >> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
> >> NullPointerException describing what is null.
> >>
> >> > Hi Remi,
> >>
> >> Hi Goetz,
> >>
> >> >
> >> >> in bytecodeUtils.cpp, in print_local_var(),
> >> >> i believe that the code
> >> >> if (!method->is_static() && (slot == 0)) {
> >> >> os->print("this");
> >> >> }
> >> >> ...
> >> >> is only true if the bytecode is generated by javac and ecj, tools like
> proguard
> >> >> that tries to obfuscate the code will reuse the slot 0 once "this" is
> >> >> not
> >> needed
> >> >> anymore.
> >> >> This is obviously also true for any other parameters, so in my opinion,
> you
> >> >> should not try to be too heroic here and always display "local%d".
> >> > Yes, you are right. I assume the bytecode local slots are mapped 1:1 to
> >> > the parameters and are not reused for other values.
> >> >
> >> >> The other solution is to propagate "this" and "parameter%d" during the
> >> static
> >> >> analysis, so "this" will become "local0" once a store_0 is seen.
> >> > It would be possible to spot the place where "this" is first overwritten.
> >> > For other parameters, this is not feasible, they are not read-only, so
> >> > stores don't indicate obfuscation.
> >>
> >> Java is not the only language to run on the Java plateform so try to detect
> >> "obfuscation" is again akind of shortcut.
> >>
> >> >
> >> > I could mark the whole method as 'obfuscated' once I see a store_0,
> >> > and then print "local" instead of "parameter" in all places.
> >> > This does not work for static methods, though. Nor for methods
> >> > where "this" is live to the end, but the parameter slots are
> >> > reused.
> >>
> >> and you can have a path with a store_0 and one without (the code is not
> >> linear).
> >>
> >> >
> >> > For obfuscated methods I would claim that printing "this" etc
> >> > is fine even if the slot is reused for another value. It just
> >> > adds to the obfuscation! But there might be code
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
- Mail original -
> De: "Goetz Lindenmaier"
> À: "Remi Forax"
> Cc: "hotspot-runtime-dev" ,
> "core-libs-dev"
> Envoyé: Lundi 23 Septembre 2019 12:03:30
> Objet: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
> Hi Remi,
Hi Goetz,
>
> what do you think about dealing with the problem like this:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18-obfuscation/
> It's at the cost of one 64-bit field per bytecode in the analysis data.
> Also, if there is a real assignment to a parameter it's not named 'parameteri'
> after that any more. See the example in the test.
>
yes, it's a nice approximation, having a method with more than 63/64 parameters
is rare.
patch looks good, thumbs up !
> Best regards,
> Goetz.
>
regards,
Rémi
>
>
>> -Original Message-
>> From: fo...@univ-mlv.fr
>> Sent: Freitag, 20. September 2019 00:01
>> To: Lindenmaier, Goetz
>> Cc: hotspot-runtime-dev ; core-libs-
>> dev
>> Subject: Re: RFR (L, final): 8218626: Add detailed message to
>> NullPointerException describing what is null.
>>
>> - Mail original -
>> > De: "Goetz Lindenmaier"
>> > À: "Remi Forax"
>> > Cc: "hotspot-runtime-dev" , "core-
>> libs-dev"
>> > Envoyé: Mercredi 18 Septembre 2019 09:37:36
>> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
>> NullPointerException describing what is null.
>>
>> > Hi Remi,
>>
>> Hi Goetz,
>>
>> >
>> >> in bytecodeUtils.cpp, in print_local_var(),
>> >> i believe that the code
>> >> if (!method->is_static() && (slot == 0)) {
>> >> os->print("this");
>> >> }
>> >> ...
>> >> is only true if the bytecode is generated by javac and ecj, tools like
>> >> proguard
>> >> that tries to obfuscate the code will reuse the slot 0 once "this" is not
>> needed
>> >> anymore.
>> >> This is obviously also true for any other parameters, so in my opinion,
>> >> you
>> >> should not try to be too heroic here and always display "local%d".
>> > Yes, you are right. I assume the bytecode local slots are mapped 1:1 to
>> > the parameters and are not reused for other values.
>> >
>> >> The other solution is to propagate "this" and "parameter%d" during the
>> static
>> >> analysis, so "this" will become "local0" once a store_0 is seen.
>> > It would be possible to spot the place where "this" is first overwritten.
>> > For other parameters, this is not feasible, they are not read-only, so
>> > stores don't indicate obfuscation.
>>
>> Java is not the only language to run on the Java plateform so try to detect
>> "obfuscation" is again akind of shortcut.
>>
>> >
>> > I could mark the whole method as 'obfuscated' once I see a store_0,
>> > and then print "local" instead of "parameter" in all places.
>> > This does not work for static methods, though. Nor for methods
>> > where "this" is live to the end, but the parameter slots are
>> > reused.
>>
>> and you can have a path with a store_0 and one without (the code is not
>> linear).
>>
>> >
>> > For obfuscated methods I would claim that printing "this" etc
>> > is fine even if the slot is reused for another value. It just
>> > adds to the obfuscation! But there might be code
>> > that is just optimized and not meant to be obfuscated.
>>
>> I've used obfuscation as an example, but there are also valid case to reuse
>> slots
>> likeit will consume less stack memory if you are using a very small device.
>>
>> >
>> > Best regards,
>> > Goetz.
>>
>> regards,
>> Rémi
>>
>> >
>> >
>> >
>> >
>> >
>> >
>> >>
>> >> Rémi
>> >>
>> >> - Mail original -
>> >> > De: "Goetz Lindenmaier"
>> >> > À: "hotspot-runtime-dev" ,
>> >> "core-libs-dev"
>> >> > Envoyé: Mardi 17 Septembre 2019 16:18:03
>> >> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
>> >> NullPointerException describing what is null.
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz,
A bit of wordsmithing on the javadoc of NullPointerException.getMessage
and separating out the implementation specific description to an @implNote
75:
/**
* Returns the detail message string of this throwable.
*
* If a non-null message was supplied in a constructor it is returned.
* Otherwise, an implementation specific message or {@code null} is
returned.
* @ImplNote
* If no explicit message was passed to the constructor, and as
* long as certain internal information is available, a verbose
* description of the null reference is returned.
* The internal information is not available in deserialized
NullPointerExceptions.
*
* @return the detail message string, which may be {@code null}.
*
94-97: The comment on the getExtendedNPEMessage should use the normal
/**... */ syntax.
Thanks, Roger
On 9/17/19 10:18 AM, Lindenmaier, Goetz wrote:
@core-libs experts, I would appreciate comments on the changes
to NullPointerException.java, especially wrt. the Javadoc comment.
The change there is S.
Best regards,
Goetz.
-Original Message-
From: Lindenmaier, Goetz
Sent: Dienstag, 10. September 2019 11:48
To: 'Hotspot dev runtime' ; Java Core
Libs
Subject: RFR (L, final): 8218626: Add detailed message to NullPointerException
describing what is null.
Hi,
this is the implementation of JEP 358: Helpful NullPointerExceptions.
The JEP is in status "Candidate". Coleen (many, many thanks!) ran
it through the Oracle-internal processes. Now I please need final reviews
for this change so that I can propose it to target jdk 14.
JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628
webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/
The change ran through a lot of testing, all jtreg and jck tests to name
only some. The webrev points to patch
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
NPE/16/enable_NPE_message.patch
that enables the change by default, which was useful for testing to
assure the code is used in the tests.
I just pushed the change to jdk/submit once more.
Please review.
Thanks!
Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Remi,
what do you think about dealing with the problem like this:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18-obfuscation/
It's at the cost of one 64-bit field per bytecode in the analysis data.
Also, if there is a real assignment to a parameter it's not named 'parameteri'
after that any more. See the example in the test.
Best regards,
Goetz.
> -Original Message-
> From: fo...@univ-mlv.fr
> Sent: Freitag, 20. September 2019 00:01
> To: Lindenmaier, Goetz
> Cc: hotspot-runtime-dev ; core-libs-
> dev
> Subject: Re: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> - Mail original -
> > De: "Goetz Lindenmaier"
> > À: "Remi Forax"
> > Cc: "hotspot-runtime-dev" , "core-
> libs-dev"
> > Envoyé: Mercredi 18 Septembre 2019 09:37:36
> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> > Hi Remi,
>
> Hi Goetz,
>
> >
> >> in bytecodeUtils.cpp, in print_local_var(),
> >> i believe that the code
> >> if (!method->is_static() && (slot == 0)) {
> >> os->print("this");
> >> }
> >> ...
> >> is only true if the bytecode is generated by javac and ecj, tools like
> >> proguard
> >> that tries to obfuscate the code will reuse the slot 0 once "this" is not
> needed
> >> anymore.
> >> This is obviously also true for any other parameters, so in my opinion, you
> >> should not try to be too heroic here and always display "local%d".
> > Yes, you are right. I assume the bytecode local slots are mapped 1:1 to
> > the parameters and are not reused for other values.
> >
> >> The other solution is to propagate "this" and "parameter%d" during the
> static
> >> analysis, so "this" will become "local0" once a store_0 is seen.
> > It would be possible to spot the place where "this" is first overwritten.
> > For other parameters, this is not feasible, they are not read-only, so
> > stores don't indicate obfuscation.
>
> Java is not the only language to run on the Java plateform so try to detect
> "obfuscation" is again akind of shortcut.
>
> >
> > I could mark the whole method as 'obfuscated' once I see a store_0,
> > and then print "local" instead of "parameter" in all places.
> > This does not work for static methods, though. Nor for methods
> > where "this" is live to the end, but the parameter slots are
> > reused.
>
> and you can have a path with a store_0 and one without (the code is not
> linear).
>
> >
> > For obfuscated methods I would claim that printing "this" etc
> > is fine even if the slot is reused for another value. It just
> > adds to the obfuscation! But there might be code
> > that is just optimized and not meant to be obfuscated.
>
> I've used obfuscation as an example, but there are also valid case to reuse
> slots
> likeit will consume less stack memory if you are using a very small device.
>
> >
> > Best regards,
> > Goetz.
>
> regards,
> Rémi
>
> >
> >
> >
> >
> >
> >
> >>
> >> Rémi
> >>
> >> - Mail original -
> >> > De: "Goetz Lindenmaier"
> >> > À: "hotspot-runtime-dev" ,
> >> "core-libs-dev"
> >> > Envoyé: Mardi 17 Septembre 2019 16:18:03
> >> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
> >> NullPointerException describing what is null.
> >>
> >> > @core-libs experts, I would appreciate comments on the changes
> >> > to NullPointerException.java, especially wrt. the Javadoc comment.
> >> > The change there is S.
> >> >
> >> > Best regards,
> >> > Goetz.
> >> >
> >> >> -Original Message-
> >> >> From: Lindenmaier, Goetz
> >> >> Sent: Dienstag, 10. September 2019 11:48
> >> >> To: 'Hotspot dev runtime' ;
> >> Java Core
> >> >> Libs
> >> >> Subject: RFR (L, final): 8218626: Add detailed message to
> >> NullPointerException
> >> >> describing what is null.
> >> >>
> >> >> Hi,
> >> >>
> >> >>
> >> >>
> >> >> this is the impleme
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Ralf,
thanks for looking at this code and this thorough review!
New webrevs:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/18-incremental/
Find my comments inline:
> In javaClasses.cpp:
>
> > #define CLASS_FIELDS_DO(macro) \
> > macro(classRedefinedCount_offset, k, "classRedefinedCount", int_signature,
> false); \
> > macro(_class_loader_offset, k, "classLoader",
> > classloader_signature,
> false); \
>
> The field name should match the other field names. So
> _class_redefined_count_offset instead of classRedefinedCount_offset.
That's right. But the field was there before my change, I just removed
the spurious space. So I don't feel like changing this.
> The method java_lang_Throwable::get_method_and_bci() should be renamed
> java_lang_Throwable::get_top_method_and_bci().
Changed.
> The usage of java.lang.Boolean(true) as the marker pointer leads to more code
> than a simpler, but more hackish solution (e.g. just reusing a reference to
> the
> backtrace array itself). I'm not sure it is really worth it. Considering you
> in the
> end just test for != NULL.
I think the design is clearer the way I did it.
The Boolean should almost never be allocated. In general, hidden frames should
not throw NPEs.
> In jvm.cpp:
>
> The method trim_well_known_class_names() would convert a name like
> test.java.lang.String to test.String. I think you have to be more specific
> when
> replacing a name.
Ah, you are right. A pity, this was nice and compact.
I moved the code into bytecodeUtils.cpp.
> In bytecodeUtils.cpp:
>
> The method get_slotData() should be renamed get_slot_data() in class
> SimulatedOperandStack.
Done.
> The parameter innerExpr should be renamed inner_expr.
Done.
> In method print_local_var() you could initialize param_index to 1 instead of 0
> and remove adding 1 later.
Fixed.
> In ExceptionMessageBuilder::ExceptionMessageBuilder you should add spaces
> in '(len+1)'.
Fixed.
> Since the ExceptionMessageBuilder is only used with a bci >= 0, you could
> remove the code which handles the case for bci < 0 (where we would create
> the simulated stack for every bci).
I'd like to leave this as-is, as the code could well analyze the whole
method.
> In ExceptionMessageBuilder::do_instruction() the dup2 bytecode seems to be
> implemented wrongly, since you seemingly push two times the same value. It
> isn't, since you change the stack with the push, so in the first push you push
> the top of stack - 1 entry, and in the next push the former top of stack. A
> comment should be added to make this clearer or change to the code to use
> temporary variables.
Added comment.
> In ExceptionMessageBuilder::do_instruction() the is_wide variable is never
> used, so it should be removed.
Done.
> In ExceptionMessageBuilder::do_instruction() when handling the invoke*
> bytecodes, there is the following code:
>
> if ((code != Bytecodes::_invokestatic) && (code !=
> Bytecodes::_invokedynamic)) {
> // Pop class.
> stack->pop(1);
> }
>
> The // Pop class comment is misleading, because the receiver is popped in
> reality.
Fixed.
> In ExceptionMessageBuilder::get_NPE_null_slot() the opening brace for the
> invoke* cases should be consistent with the rest of the code.
Fixed.
> In ExceptionMessageBuilder::get_NPE_null_slot() maybe we should use defines
> for -2 and -1 which convey the meaning of that return values.
Done.
> In ExceptionMessageBuilder::print_NPE_failed_action() you assert that the
> method holder is not the NativeConstructorAccessorImpl, noting that this
> should already have been checked In get_NPE_null_slot(). But I don't see any
> code in that message which would check this, it seemed to be check in
> BytecodeUtils::get_NPE_message_at() instead.
I moved that code up when I reviewed my own code.
It seems better placed here, as also the check for
NPE_EXPLICIT_CONSTRUCTED is here.
I removed the assertion.
> In NullPointerExceptionTest.java:
>
> It seems you don't have tests for invokeinterface or invokespecial calls to
> cause
> an NPE (e.g. by calling a null interface variable or a private non-static
> method
> of a null objects).
That is because the code is the same as for invokevirtual in all places in
bytecodeUtils.cpp.
Thanks and best regards,
Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz,
here are my review remarks:
In javaClasses.cpp:
> #define CLASS_FIELDS_DO(macro) \
> macro(classRedefinedCount_offset, k, "classRedefinedCount", int_signature,
> false); \
> macro(_class_loader_offset, k, "classLoader",
> classloader_signature, false); \
The field name should match the other field names. So
_class_redefined_count_offset instead of classRedefinedCount_offset.
The method java_lang_Throwable::get_method_and_bci() should be renamed
java_lang_Throwable::get_top_method_and_bci().
The usage of java.lang.Boolean(true) as the marker pointer leads to more code
than a simpler, but more hackish solution (e.g. just reusing a reference to the
backtrace array itself). I'm not sure it is really worth it. Considering you in
the end just test for != NULL.
In jvm.cpp:
The method trim_well_known_class_names() would convert a name like
test.java.lang.String to test.String. I think you have to be more specific when
replacing a name.
In bytecodeUtils.cpp:
The method get_slotData() should be renamed get_slot_data() in class
SimulatedOperandStack.
The parameter innerExpr should be renamed inner_expr.
In method print_local_var() you could initialize param_index to 1 instead of 0
and remove adding 1 later.
In ExceptionMessageBuilder::ExceptionMessageBuilder you should add spaces in
'(len+1)'.
Since the ExceptionMessageBuilder is only used with a bci >= 0, you could
remove the code which handles the case for bci < 0 (where we would create the
simulated stack for every bci).
In ExceptionMessageBuilder::do_instruction() the dup2 bytecode seems to be
implemented wrongly, since you seemingly push two times the same value. It
isn't, since you change the stack with the push, so in the first push you push
the top of stack - 1 entry, and in the next push the former top of stack. A
comment should be added to make this clearer or change to the code to use
temporary variables.
In ExceptionMessageBuilder::do_instruction() the is_wide variable is never
used, so it should be removed.
In ExceptionMessageBuilder::do_instruction() when handling the invoke*
bytecodes, there is the following code:
if ((code != Bytecodes::_invokestatic) && (code != Bytecodes::_invokedynamic)) {
// Pop class.
stack->pop(1);
}
The // Pop class comment is misleading, because the receiver is popped in
reality.
In ExceptionMessageBuilder::get_NPE_null_slot() the opening brace for the
invoke* cases should be consistent with the rest of the code.
In ExceptionMessageBuilder::get_NPE_null_slot() maybe we should use defines for
-2 and -1 which convey the meaning of that return values.
In ExceptionMessageBuilder::print_NPE_failed_action() you assert that the
method holder is not the NativeConstructorAccessorImpl, noting that this should
already have been checked In get_NPE_null_slot(). But I don't see any code in
that message which would check this, it seemed to be check in
BytecodeUtils::get_NPE_message_at() instead.
In NullPointerExceptionTest.java:
It seems you don't have tests for invokeinterface or invokespecial calls to
cause an NPE (e.g. by calling a null interface variable or a private non-static
method of a null objects).
The rest looks good to me.
Best regards,
Ralf
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
- Mail original -
> De: "Goetz Lindenmaier"
> À: "Remi Forax"
> Cc: "hotspot-runtime-dev" ,
> "core-libs-dev"
> Envoyé: Mercredi 18 Septembre 2019 09:37:36
> Objet: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
> Hi Remi,
Hi Goetz,
>
>> in bytecodeUtils.cpp, in print_local_var(),
>> i believe that the code
>> if (!method->is_static() && (slot == 0)) {
>> os->print("this");
>> }
>> ...
>> is only true if the bytecode is generated by javac and ecj, tools like
>> proguard
>> that tries to obfuscate the code will reuse the slot 0 once "this" is not
>> needed
>> anymore.
>> This is obviously also true for any other parameters, so in my opinion, you
>> should not try to be too heroic here and always display "local%d".
> Yes, you are right. I assume the bytecode local slots are mapped 1:1 to
> the parameters and are not reused for other values.
>
>> The other solution is to propagate "this" and "parameter%d" during the static
>> analysis, so "this" will become "local0" once a store_0 is seen.
> It would be possible to spot the place where "this" is first overwritten.
> For other parameters, this is not feasible, they are not read-only, so
> stores don't indicate obfuscation.
Java is not the only language to run on the Java plateform so try to detect
"obfuscation" is again akind of shortcut.
>
> I could mark the whole method as 'obfuscated' once I see a store_0,
> and then print "local" instead of "parameter" in all places.
> This does not work for static methods, though. Nor for methods
> where "this" is live to the end, but the parameter slots are
> reused.
and you can have a path with a store_0 and one without (the code is not linear).
>
> For obfuscated methods I would claim that printing "this" etc
> is fine even if the slot is reused for another value. It just
> adds to the obfuscation! But there might be code
> that is just optimized and not meant to be obfuscated.
I've used obfuscation as an example, but there are also valid case to reuse
slots likeit will consume less stack memory if you are using a very small
device.
>
> Best regards,
> Goetz.
regards,
Rémi
>
>
>
>
>
>
>>
>> Rémi
>>
>> - Mail original -
>> > De: "Goetz Lindenmaier"
>> > À: "hotspot-runtime-dev" ,
>> "core-libs-dev"
>> > Envoyé: Mardi 17 Septembre 2019 16:18:03
>> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
>> NullPointerException describing what is null.
>>
>> > @core-libs experts, I would appreciate comments on the changes
>> > to NullPointerException.java, especially wrt. the Javadoc comment.
>> > The change there is S.
>> >
>> > Best regards,
>> > Goetz.
>> >
>> >> -Original Message-
>> >> From: Lindenmaier, Goetz
>> >> Sent: Dienstag, 10. September 2019 11:48
>> >> To: 'Hotspot dev runtime' ;
>> Java Core
>> >> Libs
>> >> Subject: RFR (L, final): 8218626: Add detailed message to
>> NullPointerException
>> >> describing what is null.
>> >>
>> >> Hi,
>> >>
>> >>
>> >>
>> >> this is the implementation of JEP 358: Helpful NullPointerExceptions.
>> >>
>> >>
>> >>
>> >> The JEP is in status "Candidate". Coleen (many, many thanks!) ran
>> >>
>> >> it through the Oracle-internal processes. Now I please need final reviews
>> >>
>> >> for this change so that I can propose it to target jdk 14.
>> >>
>> >>
>> >>
>> >> JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
>> >>
>> >> Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628
>> >>
>> >> webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
>> NPE/16/
>> >>
>> >>
>> >>
>> >> The change ran through a lot of testing, all jtreg and jck tests to name
>> >>
>> >> only some. The webrev points to patch
>> >>
>> >> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
>> >> NPE/16/enable_NPE_message.patch
>> >>
>> >> that enables the change by default, which was useful for testing to
>> >>
>> >> assure the code is used in the tests.
>> >>
>> >> I just pushed the change to jdk/submit once more.
>> >>
>> >>
>> >>
>> >> Please review.
>> >>
>> >>
>> >>
>> >> Thanks!
>> >>
> > > > Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Remi,
> in bytecodeUtils.cpp, in print_local_var(),
> i believe that the code
> if (!method->is_static() && (slot == 0)) {
> os->print("this");
> }
> ...
> is only true if the bytecode is generated by javac and ecj, tools like
> proguard
> that tries to obfuscate the code will reuse the slot 0 once "this" is not
> needed
> anymore.
> This is obviously also true for any other parameters, so in my opinion, you
> should not try to be too heroic here and always display "local%d".
Yes, you are right. I assume the bytecode local slots are mapped 1:1 to
the parameters and are not reused for other values.
> The other solution is to propagate "this" and "parameter%d" during the static
> analysis, so "this" will become "local0" once a store_0 is seen.
It would be possible to spot the place where "this" is first overwritten.
For other parameters, this is not feasible, they are not read-only, so
stores don't indicate obfuscation.
I could mark the whole method as 'obfuscated' once I see a store_0,
and then print "local" instead of "parameter" in all places.
This does not work for static methods, though. Nor for methods
where "this" is live to the end, but the parameter slots are
reused.
For obfuscated methods I would claim that printing "this" etc
is fine even if the slot is reused for another value. It just
adds to the obfuscation! But there might be code
that is just optimized and not meant to be obfuscated.
Best regards,
Goetz.
>
> Rémi
>
> ----- Mail original -
> > De: "Goetz Lindenmaier"
> > À: "hotspot-runtime-dev" ,
> "core-libs-dev"
> > Envoyé: Mardi 17 Septembre 2019 16:18:03
> > Objet: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
>
> > @core-libs experts, I would appreciate comments on the changes
> > to NullPointerException.java, especially wrt. the Javadoc comment.
> > The change there is S.
> >
> > Best regards,
> > Goetz.
> >
> >> -Original Message-
> >> From: Lindenmaier, Goetz
> >> Sent: Dienstag, 10. September 2019 11:48
> >> To: 'Hotspot dev runtime' ;
> Java Core
> >> Libs
> >> Subject: RFR (L, final): 8218626: Add detailed message to
> NullPointerException
> >> describing what is null.
> >>
> >> Hi,
> >>
> >>
> >>
> >> this is the implementation of JEP 358: Helpful NullPointerExceptions.
> >>
> >>
> >>
> >> The JEP is in status "Candidate". Coleen (many, many thanks!) ran
> >>
> >> it through the Oracle-internal processes. Now I please need final reviews
> >>
> >> for this change so that I can propose it to target jdk 14.
> >>
> >>
> >>
> >> JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
> >>
> >> Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628
> >>
> >> webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> NPE/16/
> >>
> >>
> >>
> >> The change ran through a lot of testing, all jtreg and jck tests to name
> >>
> >> only some. The webrev points to patch
> >>
> >> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
> >> NPE/16/enable_NPE_message.patch
> >>
> >> that enables the change by default, which was useful for testing to
> >>
> >> assure the code is used in the tests.
> >>
> >> I just pushed the change to jdk/submit once more.
> >>
> >>
> >>
> >> Please review.
> >>
> >>
> >>
> >> Thanks!
> >>
> > > Goetz.
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz,
in bytecodeUtils.cpp, in print_local_var(),
i believe that the code
if (!method->is_static() && (slot == 0)) {
os->print("this");
}
...
is only true if the bytecode is generated by javac and ecj, tools like proguard
that tries to obfuscate the code will reuse the slot 0 once "this" is not
needed anymore.
This is obviously also true for any other parameters, so in my opinion, you
should not try to be too heroic here and always display "local%d".
The other solution is to propagate "this" and "parameter%d" during the static
analysis, so "this" will become "local0" once a store_0 is seen.
Rémi
- Mail original -
> De: "Goetz Lindenmaier"
> À: "hotspot-runtime-dev" ,
> "core-libs-dev"
> Envoyé: Mardi 17 Septembre 2019 16:18:03
> Objet: RE: RFR (L, final): 8218626: Add detailed message to
> NullPointerException describing what is null.
> @core-libs experts, I would appreciate comments on the changes
> to NullPointerException.java, especially wrt. the Javadoc comment.
> The change there is S.
>
> Best regards,
> Goetz.
>
>> -Original Message-
>> From: Lindenmaier, Goetz
>> Sent: Dienstag, 10. September 2019 11:48
>> To: 'Hotspot dev runtime' ; Java Core
>> Libs
>> Subject: RFR (L, final): 8218626: Add detailed message to
>> NullPointerException
>> describing what is null.
>>
>> Hi,
>>
>>
>>
>> this is the implementation of JEP 358: Helpful NullPointerExceptions.
>>
>>
>>
>> The JEP is in status "Candidate". Coleen (many, many thanks!) ran
>>
>> it through the Oracle-internal processes. Now I please need final reviews
>>
>> for this change so that I can propose it to target jdk 14.
>>
>>
>>
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8220715
>>
>> Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628
>>
>> webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/
>>
>>
>>
>> The change ran through a lot of testing, all jtreg and jck tests to name
>>
>> only some. The webrev points to patch
>>
>> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-
>> NPE/16/enable_NPE_message.patch
>>
>> that enables the change by default, which was useful for testing to
>>
>> assure the code is used in the tests.
>>
>> I just pushed the change to jdk/submit once more.
>>
>>
>>
>> Please review.
>>
>>
>>
>> Thanks!
>>
> > Goetz.
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Makes sense Goetz. Cheers, Thomas On Tue, Sep 17, 2019 at 11:23 AM Lindenmaier, Goetz < goetz.lindenma...@sap.com> wrote: > Hi Thomas, > > thanks for pointing this out. I improved the placement > of the ResourceMarks. > Unfortunately, base() returns an immutable string, but > for trim_well_known_class_names this does not work. > So I'd propose this: > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/17-incremental/ > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/17/ > > Best regards, > Goetz. > > > -Original Message- > > From: Thomas Stüfe > > Sent: Dienstag, 17. September 2019 09:06 > > To: Lindenmaier, Goetz > > Cc: Hotspot dev runtime ; Java > Core > > Libs > > Subject: Re: RFR (L, final): 8218626: Add detailed message to > > NullPointerException describing what is null. > > > > Additionally, since 8224193, stringStream does not use RA anymore, so > you do > > not need ResourceMarks for the backing buffer. 8224193 has been > backported > > to 11, btw. > > > > On Mon, Sep 16, 2019 at 2:53 PM Thomas Stüfe > <mailto:thomas.stu...@gmail.com> > wrote: > > > > > > Hi Goetz, > > > > not a full review, just a small suggestion. In jvm.cpp you could > just > > access ss->base() instead of ss->as_string() since the internal buffer > is already > > NULL terminated and result_string does not outlive the stringStream > object. > > Also it misses including ostream.hpp. > > > > Cheers, Thomas > > > > > > On Tue, Sep 10, 2019 at 4:46 PM Lindenmaier, Goetz > > mailto:goetz.lindenma...@sap.com> > wrote: > > > > > > Hi, > > > > the subject should mention 8218628. (Not 8218626). > > Sorry for this! > > > > Best regards, > > Goetz. > > > > From: Lindenmaier, Goetz > > Sent: Dienstag, 10. September 2019 11:48 > > To: 'Hotspot dev runtime' > d...@openjdk.java.net <mailto:hotspot-runtime-...@openjdk.java.net> >; > Java > > Core Libs mailto:core-libs- > > d...@openjdk.java.net> > > > Subject: RFR (L, final): 8218626: Add detailed message to > > NullPointerException describing what is null. > > > > Hi, > > > > this is the implementation of JEP 358: Helpful > > NullPointerExceptions. > > > > The JEP is in status "Candidate". Coleen (many, many > thanks!) > > ran > > it through the Oracle-internal processes. Now I please > need > > final reviews > > for this change so that I can propose it to target jdk 14. > > > > JEP: https://bugs.openjdk.java.net/browse/JDK-8220715 > > Enhancement: https://bugs.openjdk.java.net/browse/JDK- > > 8218628 > > webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628- > > exMsg-NPE/16/ > > > > The change ran through a lot of testing, all jtreg and jck > tests to > > name > > only some. The webrev points to patch > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg- > > NPE/16/enable_NPE_message.patch > > that enables the change by default, which was useful for > > testing to > > assure the code is used in the tests. > > I just pushed the change to jdk/submit once more. > > > > Please review. > > > > Thanks! > > Goetz. > > > >
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
@core-libs experts, I would appreciate comments on the changes to NullPointerException.java, especially wrt. the Javadoc comment. The change there is S. Best regards, Goetz. > -Original Message- > From: Lindenmaier, Goetz > Sent: Dienstag, 10. September 2019 11:48 > To: 'Hotspot dev runtime' ; Java Core > Libs > Subject: RFR (L, final): 8218626: Add detailed message to NullPointerException > describing what is null. > > Hi, > > > > this is the implementation of JEP 358: Helpful NullPointerExceptions. > > > > The JEP is in status "Candidate". Coleen (many, many thanks!) ran > > it through the Oracle-internal processes. Now I please need final reviews > > for this change so that I can propose it to target jdk 14. > > > > JEP: https://bugs.openjdk.java.net/browse/JDK-8220715 > > Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628 > > webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/ > > > > The change ran through a lot of testing, all jtreg and jck tests to name > > only some. The webrev points to patch > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg- > NPE/16/enable_NPE_message.patch > > that enables the change by default, which was useful for testing to > > assure the code is used in the tests. > > I just pushed the change to jdk/submit once more. > > > > Please review. > > > > Thanks! > > Goetz.
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Thomas, thanks for pointing this out. I improved the placement of the ResourceMarks. Unfortunately, base() returns an immutable string, but for trim_well_known_class_names this does not work. So I'd propose this: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/17-incremental/ http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/17/ Best regards, Goetz. > -Original Message- > From: Thomas Stüfe > Sent: Dienstag, 17. September 2019 09:06 > To: Lindenmaier, Goetz > Cc: Hotspot dev runtime ; Java Core > Libs > Subject: Re: RFR (L, final): 8218626: Add detailed message to > NullPointerException describing what is null. > > Additionally, since 8224193, stringStream does not use RA anymore, so you do > not need ResourceMarks for the backing buffer. 8224193 has been backported > to 11, btw. > > On Mon, Sep 16, 2019 at 2:53 PM Thomas Stüfe <mailto:thomas.stu...@gmail.com> > wrote: > > > Hi Goetz, > > not a full review, just a small suggestion. In jvm.cpp you could just > access ss->base() instead of ss->as_string() since the internal buffer is > already > NULL terminated and result_string does not outlive the stringStream object. > Also it misses including ostream.hpp. > > Cheers, Thomas > > > On Tue, Sep 10, 2019 at 4:46 PM Lindenmaier, Goetz > mailto:goetz.lindenma...@sap.com> > wrote: > > > Hi, > > the subject should mention 8218628. (Not 8218626). > Sorry for this! > > Best regards, > Goetz. > > From: Lindenmaier, Goetz > Sent: Dienstag, 10. September 2019 11:48 > To: 'Hotspot dev runtime' d...@openjdk.java.net <mailto:hotspot-runtime-...@openjdk.java.net> >; Java > Core Libs mailto:core-libs- > d...@openjdk.java.net> > > Subject: RFR (L, final): 8218626: Add detailed message to > NullPointerException describing what is null. > > Hi, > > this is the implementation of JEP 358: Helpful > NullPointerExceptions. > > The JEP is in status "Candidate". Coleen (many, many thanks!) > ran > it through the Oracle-internal processes. Now I please need > final reviews > for this change so that I can propose it to target jdk 14. > > JEP: https://bugs.openjdk.java.net/browse/JDK-8220715 > Enhancement: https://bugs.openjdk.java.net/browse/JDK- > 8218628 > webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628- > exMsg-NPE/16/ > > The change ran through a lot of testing, all jtreg and jck > tests to > name > only some. The webrev points to patch > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg- > NPE/16/enable_NPE_message.patch > that enables the change by default, which was useful for > testing to > assure the code is used in the tests. > I just pushed the change to jdk/submit once more. > > Please review. > > Thanks! > Goetz. >
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Additionally, since 8224193, stringStream does not use RA anymore, so you do not need ResourceMarks for the backing buffer. 8224193 has been backported to 11, btw. On Mon, Sep 16, 2019 at 2:53 PM Thomas Stüfe wrote: > Hi Goetz, > > not a full review, just a small suggestion. In jvm.cpp you could just > access ss->base() instead of ss->as_string() since the internal buffer is > already NULL terminated and result_string does not outlive the stringStream > object. Also it misses including ostream.hpp. > > Cheers, Thomas > > On Tue, Sep 10, 2019 at 4:46 PM Lindenmaier, Goetz < > goetz.lindenma...@sap.com> wrote: > >> Hi, >> >> the subject should mention 8218628. (Not 8218626). >> Sorry for this! >> >> Best regards, >> Goetz. >> >> From: Lindenmaier, Goetz >> Sent: Dienstag, 10. September 2019 11:48 >> To: 'Hotspot dev runtime' ; Java >> Core Libs >> Subject: RFR (L, final): 8218626: Add detailed message to >> NullPointerException describing what is null. >> >> Hi, >> >> this is the implementation of JEP 358: Helpful NullPointerExceptions. >> >> The JEP is in status "Candidate". Coleen (many, many thanks!) ran >> it through the Oracle-internal processes. Now I please need final reviews >> for this change so that I can propose it to target jdk 14. >> >> JEP: https://bugs.openjdk.java.net/browse/JDK-8220715 >> Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628 >> webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/ >> >> The change ran through a lot of testing, all jtreg and jck tests to name >> only some. The webrev points to patch >> >> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/enable_NPE_message.patch >> that enables the change by default, which was useful for testing to >> assure the code is used in the tests. >> I just pushed the change to jdk/submit once more. >> >> Please review. >> >> Thanks! >> Goetz. >> >
Re: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi Goetz, not a full review, just a small suggestion. In jvm.cpp you could just access ss->base() instead of ss->as_string() since the internal buffer is already NULL terminated and result_string does not outlive the stringStream object. Also it misses including ostream.hpp. Cheers, Thomas On Tue, Sep 10, 2019 at 4:46 PM Lindenmaier, Goetz < goetz.lindenma...@sap.com> wrote: > Hi, > > the subject should mention 8218628. (Not 8218626). > Sorry for this! > > Best regards, > Goetz. > > From: Lindenmaier, Goetz > Sent: Dienstag, 10. September 2019 11:48 > To: 'Hotspot dev runtime' ; Java > Core Libs > Subject: RFR (L, final): 8218626: Add detailed message to > NullPointerException describing what is null. > > Hi, > > this is the implementation of JEP 358: Helpful NullPointerExceptions. > > The JEP is in status "Candidate". Coleen (many, many thanks!) ran > it through the Oracle-internal processes. Now I please need final reviews > for this change so that I can propose it to target jdk 14. > > JEP: https://bugs.openjdk.java.net/browse/JDK-8220715 > Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628 > webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/ > > The change ran through a lot of testing, all jtreg and jck tests to name > only some. The webrev points to patch > > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/enable_NPE_message.patch > that enables the change by default, which was useful for testing to > assure the code is used in the tests. > I just pushed the change to jdk/submit once more. > > Please review. > > Thanks! > Goetz. >
RE: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null.
Hi, the subject should mention 8218628. (Not 8218626). Sorry for this! Best regards, Goetz. From: Lindenmaier, Goetz Sent: Dienstag, 10. September 2019 11:48 To: 'Hotspot dev runtime' ; Java Core Libs Subject: RFR (L, final): 8218626: Add detailed message to NullPointerException describing what is null. Hi, this is the implementation of JEP 358: Helpful NullPointerExceptions. The JEP is in status "Candidate". Coleen (many, many thanks!) ran it through the Oracle-internal processes. Now I please need final reviews for this change so that I can propose it to target jdk 14. JEP: https://bugs.openjdk.java.net/browse/JDK-8220715 Enhancement: https://bugs.openjdk.java.net/browse/JDK-8218628 webrev: http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/ The change ran through a lot of testing, all jtreg and jck tests to name only some. The webrev points to patch http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/16/enable_NPE_message.patch that enables the change by default, which was useful for testing to assure the code is used in the tests. I just pushed the change to jdk/submit once more. Please review. Thanks! Goetz.
