New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8303489: Add a test to verify classes in vmStruct have unique vtables #12820
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
|
} | ||
} | ||
|
||
if (vtable == null && t.getSuperclass() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only log if there is no superclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vmStruct contains a lot of entries which should have vtable == null
I added logging of some stats to the test, on win-x64-fastdebug it reports:
total: 861, no vtable: 503, no_vtable_with_super: 24
if test would log everything with no vtable, test log is truncated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case is it worth listing the "no vtable" classes that have a superclass? Are they somehow more interesting than those without a superclass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If superclass is set then it's the type which describes VM class (but some JM classes have super == null)
Maybe the test should report all Types with CInt: false, CStr: false, JPrimitive: false, Oop: false, Ptr: false
Looks like these entries correspond declare_toplevel_type/declare_type in vmStruct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated criteria to log classes without vtable
+ ", CInt: " + t.isCIntegerType() | ||
+ ", CStr: " + t.isCStringType() | ||
+ ", JPrimitive: " + t.isJavaPrimitiveType() | ||
+ ", Oop: " + t.isOopType() | ||
+ ", Ptr: " + t.isPointerType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that these always print "false". Are they worth having?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It reports all available info about the Type. As these are "suspicious" types I think it make sense to provide more info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this property as by the updated condition logged classes have all "false"
/issue JDK-8303489 |
@alexmenkov This issue is referenced in the PR title - it will now be updated. |
} | ||
} | ||
|
||
if (vtable == null && t.getSuperclass() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case is it worth listing the "no vtable" classes that have a superclass? Are they somehow more interesting than those without a superclass?
Iterator<Type> it = agent.getTypeDataBase().getTypes(); | ||
int dupsFound = 0; | ||
// agent.getTypeDataBase() returns HotSpotTypeDataBase (extends BasicTypeDataBase) | ||
BasicTypeDataBase typeDB = (BasicTypeDataBase)(agent.getTypeDataBase()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the cast is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agent.getTypeDataBase() returns TypeDataBase, we need BasicTypeDataBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comment seems to indicate otherwise, although I think what you meant is that the declared return type for agent.getTypeDataBase() is TypeDataBase, but it actually returns an object of type HotSpotTypeDataBase, which is a subclass of BasicTypeDataBase. You should make that more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment. Hope it's clearer now
if (vtable != null) { | ||
List<Type> typeList = types.get(vtable); | ||
no_vtable++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no_vtable
is actually tracking the number of Types with a vtable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed reported stats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for adding this.
@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 186 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Thanks,
Serguei
/integrate |
Going to push as commit f9aadb9.
Your commit was automatically rebased without conflicts. |
@alexmenkov Pushed as commit f9aadb9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Unique vtables for classes in vmStruct data is a requirement for SA to correctly detect hotspot classes.
The fix adds test to verify this requirement.
The test fails as expected on Windows if VM is built without RTTI (see JDK-8302817)
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12820/head:pull/12820
$ git checkout pull/12820
Update a local copy of the PR:
$ git checkout pull/12820
$ git pull https://git.openjdk.org/jdk pull/12820/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12820
View PR using the GUI difftool:
$ git pr show -t 12820
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12820.diff