JDK-8317635: Improve GetClassFields test to verify correctness of field order #16131
Conversation
|
👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into |
|
@alexmenkov The following label will be automatically applied to this pull request:
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. |
Webrevs
|
| * fields. | ||
| * The test checks if the function returns the expected list of fields: | ||
| * - the list contains only directly declared (not inherited) fields; | ||
| * - fields are returned in the order they occur in the class file. |
There was a problem hiding this comment.
Is the order of the fields in the class file guaranteed to be the same as the declaration order?
There was a problem hiding this comment.
Good question.
Javac works this way, but looks like this is not a requirement.
Looks like the test should use some class file parser to get actual field order
There was a problem hiding this comment.
Updated the test to get expected field order (as well as field name and type) from class file
|
@alexmenkov 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: 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 140 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
| } | ||
| return explorer.fieldNameAndSig; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: I'd suggest to make get() an instance method. It will be a little bit simpler and more natural.
It wiill look as below:
static void check(Class cls) throws Exception {
FieldExplorer explorer = new FieldExplorer(cls);
List<String> fields = explorer.get();
check(cls, fields.toArray(new String[0]));
}
. . .
List<String> get() throws Exception {
System.out.println("Class " + cls.getName());
try (InputStream classBytes = getClassBytes()) {
ClassReader classReader = new ClassReader(classBytes);
classReader.accept(this, 0);
}
return fieldNameAndSig;
}
| } | ||
|
|
||
| // helper class to get list of the class field | ||
| // in the order they appear in the class file |
There was a problem hiding this comment.
Nit: class field => class fields.
| jvmti->Deallocate((unsigned char *)sig); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: I'd suggest to refactor a little bit the content of loop at L111 as below:
for (j = 0; j < fcount; j++) {
if (fields[j] == NULL) {
printf("(%d) fieldID = null\n", j);
result = STATUS_FAILED;
continue; => /you can consider to return here
}
err = jvmti->GetFieldName(clazz, fields[j], &name, &sig, NULL);
if (err != JVMTI_ERROR_NONE) {
printf("(GetFieldName#%d) unexpected error: %s (%d)\n",
j, TranslateError(err), err);
result = STATUS_FAILED; => This result set is missed
continue; => you can consider to return here
}
printf(">>> [%d]: %s, sig = \"%s\"\n", j, name, sig);
if ((j < field_count) &&
(name == NULL || sig == NULL ||
!equals_str(env, name, fieldArr, j * 2) ||
!equals_str(env, sig, fieldArr, j * 2 + 1))) {
printf("(%d) wrong field: \"%s%s\"", j, name, sig);
result = STATUS_FAILED;
}
jvmti->Deallocate((unsigned char *)name);
jvmti->Deallocate((unsigned char *)sig);
}
sspitsyn
left a comment
sspitsyn
left a comment
There was a problem hiding this comment.
The fix looks good to me, better then it was.
It is nice you get rid of the verbose option and print info by default.
I've posted a couple of nits.
Error is not set properly in one place of native agent.
sspitsyn
left a comment
sspitsyn
left a comment
There was a problem hiding this comment.
Thank you for the update. Looks good.
Thanks,
Serguei
|
/integrate |
|
Going to push as commit 599560a.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@alexmenkov Pushed as commit 599560a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
All test cases in getclfld007 had 1 (or 0) field in test classes/interfaces.
The change adds several fields in one of the test classes to verify order of the returned fields (as described by GetClassFields spec: "in the order they occur in the class file").
Field order in the class file is not guaranteed to be the same as in the source, so information about expected fields and expected order is extracted by ASM (it parses class file sequentially).
This allows to drop hardcoded field name/type in native part.
Additionally did some test cleanup:
genericin native check() method, added deallocation ofnameandsigProgress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16131/head:pull/16131$ git checkout pull/16131Update a local copy of the PR:
$ git checkout pull/16131$ git pull https://git.openjdk.org/jdk.git pull/16131/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16131View PR using the GUI difftool:
$ git pr show -t 16131Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16131.diff
Webrev
Link to Webrev Comment