8298065: Provide more information in message of NoSuchFieldError #11745

matias9927 · 2022-12-20T22:55:59Z

A java.lang.NoSuchFieldError is typically thrown when you remove a field but do not recompile the client code that calls the field. However, the message does not indicate in which class the field was not found.

Additionally, java.lang.NoSuchFieldError is thrown if the field is still present but the types are incompatible. For example, if a field is first defined as int, and later changed to long without recompiling the client. The error text has been expanded to include the class name and field type. Verified with tier 1-4 tests.

Old output:
Exception in thread "main" java.lang.NoSuchFieldError: x at NoSuchFieldMain.main(NoSuchFieldMain.java:3)
Example output:
Exception in thread "main" java.lang.NoSuchFieldError: Class Other does not have field 'int x' at NoSuchFieldMain.main(NoSuchFieldMain.java:3)

The added test considers different types of fields like primitives, arrays, and references.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8298065: Provide more information in message of NoSuchFieldError

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Ioi Lam (@iklam - Reviewer)
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11745/head:pull/11745
$ git checkout pull/11745

Update a local copy of the PR:
$ git checkout pull/11745
$ git pull https://git.openjdk.org/jdk pull/11745/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11745

View PR using the GUI difftool:
$ git pr show -t 11745

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11745.diff

bridgekeeper · 2022-12-20T22:57:42Z

👋 Welcome back matsaave! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-12-20T23:01:05Z

@matias9927 The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-12-20T23:04:30Z

Webrevs

dholmes-ora

Unclear on the way Object types are reported but the general idea is fine.

A few comments/suggestions around the test.

I would also search for any tests that catch NoSuchFieldError in case they look for specific messages (e.g. in reflection tests).

Thanks.

dholmes-ora · 2022-12-21T04:05:17Z

test/hotspot/jtreg/runtime/linkResolver/FieldName.java

+ * questions.
+ */
+
+/*  Let jtreg compile this one with NoSuchFieldTest.java */


Suggestion:

/* Base definition for compilation by javac. */

dholmes-ora · 2022-12-21T04:05:38Z

test/hotspot/jtreg/runtime/linkResolver/FieldName.java

+/*  Then have a @compile this version into FieldName1.jasm
+public class FieldName {
+    public static long x = 123;
+}
+*/
+
+/*  Then have a @compile this version into FieldName2.jasm
+public class FieldName {
+}
+*/


These comments belong in the respective jasm files

dholmes-ora · 2022-12-21T04:05:53Z

test/hotspot/jtreg/runtime/linkResolver/FieldName1.jasm

@@ -0,0 +1,22 @@
+


Missing copyright and licence header

dholmes-ora · 2022-12-21T04:06:01Z

test/hotspot/jtreg/runtime/linkResolver/FieldName2.jasm

@@ -0,0 +1,13 @@
+


Missing copyright and licence header

src/hotspot/share/interpreter/linkResolver.cpp

iklam · 2022-12-22T20:58:33Z

We shouldn't rebuild the test three times, replacing the binaries of a classfile each time. This makes it difficult to run the test stand-alone.

Instead, the test should be written with three separate .jasm files like this:

super public class NoSuchFieldA
	version 63:0
{
  // REMOVED static Field staticFieldA:I;

  public Method "<init>":"()V"
	stack 1 locals 1
  {
		aload_0;
		invokespecial	Method java/lang/Object."<init>":"()V";
                iconst_0;
                putstatic	Field staticField:"I"; // This will throw NoSuchFieldError
		return;
  }
}

and the main test java file can do this:

    try {
        Class.forName("NoSuchFieldA").newInstance();
    } catch (NoSuchFieldError e) {
        ....
    }

This way, you can just build the test once. After the test finishes, you can rerun the manually by using the .class files from jtreg's output directory.

coleenp · 2022-12-22T21:14:00Z

NoSuchFieldA won't work because it's a different class.

coleenp · 2022-12-22T21:18:18Z

test/hotspot/jtreg/runtime/linkResolver/FieldName1.jasm

+ * questions.
+ */
+
+super public class FieldName


You should put the Java code that produced this jasm here as a comment.

coleenp · 2022-12-22T21:25:19Z

test/hotspot/jtreg/runtime/linkResolver/NoSuchFieldOutputTest.java

+      Object z = FieldName.z;
+      s = s + "\nz = " + z;
+      throwTestException("Did not throw NoSuchFieldError", s);
+    } catch (NoSuchFieldError nsfe) {


You could put each field access in a try block and remove all the fields in the second jasm file, so you can test for all the messages specifically. Although the pattern compiler is cool.

Compiling each jasm file after the main file, IS the easiest way to run it outside jtreg.

Here we have very simple conditions to test, so there's no need for NoSuchFieldOutputTest.java to access FieldName.java at all. That's why I propose that all the error conditions should be implemented inside the jasm files alone. NoSuchFieldOutputTest.java can just refer to the error class by name.

Okay so rather than generate the NSFE by separate compilation, Ioi is suggesting the jasm code can reference a non-existent field directly and so throw NSFE. That works too and the test can be re-run stand-alone.

iklam · 2022-12-22T23:06:57Z

I think we should change the wording of the error message. There was a reason (or maybe it's by accident) that the original message was vague. It just mentioned the name of the field without the name of the class used by the FieldRef. Consider the following example:

class Base {
  int foo, bar;
}

class Top extends Base {
  public static void main(String args[]) {
    Top t = new Top();
    t.foo = 1;
    t.bar = 1;
  }
}

If it's rewritten to remove the bar field

super class Base
	version 63:0
{
  Field foo:I;
  //Field bar:I;

  Method "<init>":"()V"
	stack 1 locals 1
  {
		aload_0;
		invokespecial	Method java/lang/Object."<init>":"()V";
		return;
  }
}

super class Top extends Base
	version 63:0
{
  Method "<init>":"()V"
	stack 1 locals 1
  {
		aload_0;
		invokespecial	Method Base."<init>":"()V";
		return;
  }
  public static Method main:"([Ljava/lang/String;)V"
	stack 2 locals 2
  {
		new	class Top;
		dup;
		invokespecial	Method "<init>":"()V";
		astore_1;
		aload_1;
		iconst_1;
		putfield	Field Top.foo:"I";
		aload_1;
		iconst_1;
		putfield	Field Top.bar:"I";  // Javac outputs "Top" as the class, not "Base"
		return;
  }
}

The current output is:

$ java -cp . Top
Exception in thread "main" java.lang.NoSuchFieldError: bar
	at Top.main(top.jasm)

After this PR, if we output this:

$ java -cp . Top
Exception in thread "main" java.lang.NoSuchFieldError: Class Top does not have field 'int bar'
	at Top.main(top.jasm)

I think this would be misleading. Top.java was written with the knowledge that "bar" is a field inherited from the "Base" class (**). During execution, "Base" is replaced by an incompatible version that no longer has such a field. However, the error message blames the "Top" class, even when the problem is in the "Base" class.

It would be more correct to say this:

NoSuchFieldError: field 'int bar' cannot be be found in class Top or any of its super types

** Note: javac generates the FieldRef as "Top.bar" so that the code will work as long as "bar" is declared in "Top" or any of its super types. It doesn't mandate which class must declare this field. This allows future refactoring of the class hierarchy. For example, the "Base" class could be refactored to move all of its declared fields to a super type of "Base".

iklam · 2022-12-22T23:23:18Z

src/hotspot/share/runtime/signature.cpp

+    for (int j = 0; j < i; j++) {
+      strncat(buf, "[]", strlen(buf));
+      buf[strlen(buf)] = '\0';
+    }


Java array dimensions can be up to 255, so the buf is not long enough.

Anyway, I think we already have code that can translate a signature like [I to the source code form of int []. Try this:

super class Foofoo version 63:0 { public static Method main:"([Ljava/lang/String;)V" stack 30 locals 3 { iconst_0; iconst_0; aconst_null; invokestatic Method "xxx":"(II[I)V"; return; } } $ java -cp . Foofoo Exception in thread "main" java.lang.NoSuchMethodError: 'void Foofoo.xxx(int, int, int[])' at Foofoo.main(Foofoo.jasm)

Writing such conversion is very difficult. You should try to use the existing code (refactoring it as necessary).

After a while of searching I was unable to find anything that could easily provide this type of information. The closest example is Signature::print_as_signature_external_return_type, but this does not work with fields.

Ignore my above comment, I found a way to rework a method in symbol.cpp to accomplish the same task more safely. Thank you for this!

dholmes-ora · 2022-12-23T03:42:24Z

After this PR, if we output this:
$ java -cp . Top
Exception in thread "main" java.lang.NoSuchFieldError: Class Top does not have field 'int bar'
	at Top.main(top.jasm)
I think this would be misleading. Top.java was written with the knowledge that "bar" is a field inherited from the "Base" class (**). During execution, "Base" is replaced by an incompatible version that no longer has such a field. However, the error message blames the "Top" class, even when the problem is in the "Base" class.

I didn't read "have field bar" as meaning has the direct member field bar, but if you are concerned about that then a simple change is:

"Class Top does not have, or inherit, field 'int bar'"

or more succinctly:

"Class Top does not have a member field 'int bar'"

iklam · 2023-01-03T19:27:35Z

test/hotspot/jtreg/runtime/linkResolver/NoSuchFieldPrimitive.jasm

+
+  public static final InnerClass Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles;
+
+} // end Class NoSuchFieldPrimitive


These jasm test cases are too complicated and contain a lot of details unrelated to what you're testing. They should be simplified as:

/* public class NoSuchFieldPrimitive { // static int x; public NoSuchFieldPrimitive() { x = 123; } } */ super public class NoSuchFieldPrimitive version 65:0 { // REMOVED static Field x:I; public Method "<init>":"()V" stack 2 locals 2 { aload_0; invokespecial Method java/lang/Object."<init>":"()V"; bipush 123; putstatic Field x:"I"; return; }

iklam · 2023-01-03T19:48:49Z

src/hotspot/share/oops/symbol.cpp

+    }
+  }
+}
+


The SignatureStream for field refs should have exactly one element, and must not be a return type, so this function should be tightened (so it will assert on inputs such as (I)V or ()L/java/lang.Object;,

assert(!Signature::is_method(this), "must be a field signature"); SignatureStream ss(this, false); assert(!ss.is_done(), "must have at least one element in field ref"); assert(!at_return_type(), "field ref can not be a return type"); if (ss.is_array()) { print_array(os, ss); } else if (ss.is_reference()) { print_class(os, ss); } else { os->print("%s", type2name(ss.type())); } #ifdef ASSERT ss.next(); assert(ss.is_done(), "must have at most one element in field ref"); #endif

turbanoff · 2023-01-09T09:41:01Z

test/hotspot/jtreg/runtime/linkResolver/NoSuchFieldOutputTest.java

+    String output = nsfe.getMessage();
+    Matcher noSuchFieldMatcher = noSuchFieldPattern.matcher(output);
+    if (noSuchFieldMatcher.matches()) {
+      switch(testType) {


Suggested change

switch(testType) {

switch (testType) {

dholmes-ora · 2023-01-09T07:05:54Z

src/hotspot/share/oops/symbol.hpp

  void print_as_signature_external_parameters(outputStream *os);
+  void print_signature_as_external_field_type(outputStream *os);


Should this be print_as_signature_external_field_type?

I think Matias's version is more grammatically correct. Maybe we should rename the two existing functions

symbol.hpp: void print_as_signature_external_return_type(outputStream *os); symbol.hpp: void print_as_signature_external_parameters(outputStream *os);

to print_signature_as_xxx.

These are methods on VMSymbol that print the VMSymbol as a signature, so the naming is correct.

Sorry I am a bit obsessed with names.

The two existing methods are used only for method signatures. They each print a portion of the signature. For example, when given a signature like "(ZI)J":

print_as_signature_external_parameters() prints boolean, int. Note that the parentheses aren't printed

print_as_signature_external_return_type() prints long

Matias's new function is used only for field signatures. David's suggestion of "print_as_signature_external_field_type" is not consistent with the existing names (which do not say "method").

While "print_as_signature_external_parameters()" can be read as "print the parameters of a (method) signature in external format", I think it's clumsy and incomplete. How about:

print_method_signature_parameters()

print_method_signature_return_type()

print_field_signature_type()

I think the word "external" is confusing and can be skipped. For method signatures, we don't need methods that just print the parameters in their internal format (i.e., ZI or L), so there's no need to distinguish these functions from "internal" ones.

Also, the first two methods should assert Signature::is_method(this) and the last one should assert !Signature::is_method(this) .

I also get somewhat obsessed with names :)

We are dealing with a VMSymbol which is really just a character string that could represent many things. One of those things is a method signature. So as_signature is telling us that we are interpreting/expecting the string to be a method signature. We then want either the parameters or return type of the method signature, and those could be in internal or external format. So IMO all the existing naming is perfectly good and desirable.

The issue with the new method is that fields don't have signatures** in the general sense that methods do. A field just has a name and a type. So to me the correct naming here would be as_field_external_type.

** In the VM the classfile Signature attribute does exist for a FieldInfo but it is only used for fields declared with type variables or parameterized types. Within the VM all fields have their erased type.

as_field_external_type sounds good to me.

I still think the asserts for Signature::is_method should be added to these 3 functions.

dholmes-ora

Nothing further from me. Looks good. Thanks.

openjdk · 2023-01-10T21:10:30Z

@matias9927 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8298065: Provide more information in message of NoSuchFieldError

Reviewed-by: dholmes, iklam, coleenp

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 186 new commits pushed to the master branch:

a17b563: 8299918: Update Xcode11.3.1-MacOSX10.15 devkit at Oracle
9c1e98d: 8298321: 2 File Leak defect groups in 2 files
d663b5d: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs
437d69a: 8299836: Make user.timezone system property searchable
c7716a0: 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure
4cd87f1: 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes
7d3400b: 8299864: ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
d15285f: 8299956: [BACKOUT] 8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack
5a9490a: 8299853: Serial: Use more concrete type for TenuredGeneration::_the_space
f857f8a: 8299327: Allow super late barrier expansion of store barriers in C2
... and 176 more: https://git.openjdk.org/jdk/compare/386db07143883f85307138eca2f0305d997a2171...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @iklam, @coleenp) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

coleenp

I think it looks great!

matias9927 · 2023-01-11T19:02:21Z

Thank you for the comments and corrections @iklam, @dholmes-ora, @coleenp, and @turbanoff !
/integrate

openjdk · 2023-01-11T19:05:10Z

@matias9927
Your change (at version 3c07b81) is now ready to be sponsored by a Committer.

coleenp · 2023-01-11T19:10:35Z

/sponsor

openjdk · 2023-01-11T19:13:37Z

Going to push as commit 43847c4.
Since your change was applied there have been 186 commits pushed to the master branch:

a17b563: 8299918: Update Xcode11.3.1-MacOSX10.15 devkit at Oracle
9c1e98d: 8298321: 2 File Leak defect groups in 2 files
d663b5d: 8299498: Usage of constructors of primitive wrapper classes should be avoided in java.lang API docs
437d69a: 8299836: Make user.timezone system property searchable
c7716a0: 8299571: ZoneRulesProvider.registerProvider() can leave inconsistent state on failure
4cd87f1: 8299835: (jrtfs) Unnecessary null check in JrtPath.getAttributes
7d3400b: 8299864: ZipFileStore#supportsFileAttributeView(String) doesn't throw NPE
d15285f: 8299956: [BACKOUT] 8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack
5a9490a: 8299853: Serial: Use more concrete type for TenuredGeneration::_the_space
f857f8a: 8299327: Allow super late barrier expansion of store barriers in C2
... and 176 more: https://git.openjdk.org/jdk/compare/386db07143883f85307138eca2f0305d997a2171...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-11T19:14:02Z

@coleenp @matias9927 Pushed as commit 43847c4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

matias9927 added 2 commits December 20, 2022 22:50

8298065: Provide more information in message of NoSuchFieldError

12f46c4

Merge branch 'master' into noSuchFieldError_8298065

5020595

openjdk bot added the rfr Pull request is ready for review label Dec 20, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 20, 2022

dholmes-ora suggested changes Dec 21, 2022

View reviewed changes

matias9927 added 4 commits December 21, 2022 21:04

Improved printing of references

f493579

Updated test with more signature checks

0edddcf

Added copyright headers

6e037c8

Improved style and cleanup

3e8e923

coleenp reviewed Dec 22, 2022

View reviewed changes

iklam reviewed Dec 22, 2022

View reviewed changes

Ioi and David comments

b42ab12

iklam suggested changes Jan 3, 2023

View reviewed changes

Tightened method for printing external field type

7b7d6e6

matias9927 requested a review from dholmes-ora January 5, 2023 02:18

Simplified jasm test cases

59fad0b

turbanoff reviewed Jan 9, 2023

View reviewed changes

matias9927 added 2 commits January 9, 2023 15:58

Now compiles TestClass

7cdbd18

Spacing

aa49b34

dholmes-ora reviewed Jan 9, 2023

View reviewed changes

matias9927 added 2 commits January 10, 2023 20:30

Renamed method

b89909c

Added assert

3c07b81

dholmes-ora approved these changes Jan 10, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 10, 2023

coleenp approved these changes Jan 10, 2023

View reviewed changes

iklam approved these changes Jan 10, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 11, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 11, 2023

openjdk bot closed this Jan 11, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 11, 2023

matias9927 deleted the noSuchFieldError_8298065 branch January 11, 2023 19:18

quadhier mentioned this pull request Jun 23, 2023

8307512: Provide more information in message of NoSuchFieldException thrown by Class #14624

Closed

3 tasks


		public static final InnerClass Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles;

		} // end Class NoSuchFieldPrimitive

		void print_as_signature_external_parameters(outputStream *os);
		void print_signature_as_external_field_type(outputStream *os);

8298065: Provide more information in message of NoSuchFieldError #11745

8298065: Provide more information in message of NoSuchFieldError #11745

Conversation

matias9927 commented Dec 20, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 20, 2022

openjdk bot commented Dec 20, 2022

mlbridge bot commented Dec 20, 2022 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Dec 22, 2022

coleenp commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora commented Dec 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 10, 2023 • edited

coleenp left a comment

Choose a reason for hiding this comment

matias9927 commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

coleenp commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

matias9927 commented Dec 20, 2022 •

edited by openjdk bot

mlbridge bot commented Dec 20, 2022 •

edited

dholmes-ora commented Dec 23, 2022 •

edited

openjdk bot commented Jan 10, 2023 •

edited