Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clarity: replace storeValidationRecordIfNecessary #4181

Merged
merged 1 commit into from
Jan 9, 2019
Merged

Clarity: replace storeValidationRecordIfNecessary #4181

merged 1 commit into from
Jan 9, 2019

Conversation

jdmpapin
Copy link
Contributor

@jdmpapin jdmpapin commented Jan 4, 2019

  • Avoid recursion when adding records
  • Remove addThisRecord flag
  • Avoid duplicated logic for array types in almost all class records
  • Avoid unnecessary logic for handling primitive types
  • Deduplicate common tests (sym != NULL, inHeuristicRegion())

@jdmpapin jdmpapin mentioned this pull request Jan 4, 2019
Closed
28 tasks
@dsouzai dsouzai self-assigned this Jan 4, 2019
@jdmpapin
Clarity: replace storeValidationRecordIfNecessary
25fdd25 
- Avoid recursion when adding records
- Remove addThisRecord flag
- Avoid duplicated logic for array types in almost all class records
- Avoid unnecessary logic for handling primitive types
- Deduplicate common tests (sym != NULL, inHeuristicRegion())

Signed-off-by: Devin Papineau <[email protected]>
dsouzai
dsouzai reviewed Jan 5, 2019
View reviewed changes
runtime/compiler/runtime/SymbolValidationManager.cpp

for (auto it = _symbolValidationRecords.begin(); it != _symbolValidationRecords.end(); it++)
SVM_ASSERT(!_fej9->isClassArray(component), "expected base component type");
Copy link
Contributor

@dsouzai dsouzai Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dsouzai
dsouzai reviewed Jan 5, 2019
View reviewed changes
runtime/compiler/runtime/SymbolValidationManager.cpp
return true;

int32_t arrayDims = 0;
superClass = getBaseComponentClass(superClass, arrayDims);
Copy link
Contributor

@dsouzai dsouzai Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own clarification, it is impossible for a superclass of some class to be an array class?

EDIT: nvm, I forgot that addClassRecord will generate successive ComponentClassFromArrayClassRecord records.

dsouzai
dsouzai approved these changes Jan 5, 2019
View reviewed changes
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took some time, but I'm convinced everything is correct. There were a few places I initially wasn't sure made sense, but then that's mainly because I needed to forget the old approach of finding the leaf component class, pretending the record was for that leaf class, and generating consecutive arrayOfs (and doing the inverse of that during the validation phase).

I think the semantics are far clearer now. Thanks for also updating the documentation.

@dsouzai
Copy link
Contributor

dsouzai commented Jan 5, 2019

Jenkins test sanity xlinux,win,plinux jdk8,jdk11

@dsouzai
Copy link
Contributor

dsouzai commented Jan 5, 2019

Jenkins test sanity xlinux jdk8

@dsouzai
Copy link
Contributor

dsouzai commented Jan 7, 2019

the xlinux jdk8 build failed because of

00:08:49 Exception in thread "main" java.lang.RuntimeException: Unexpected SRP reference type: -1 from J9SRP(J9ROMClassTOCEntry)
00:08:49 	at com.ibm.j9ddr.tools.PointerGenerator.writeSRPMethod(PointerGenerator.java:1215)
00:08:49 	at com.ibm.j9ddr.tools.PointerGenerator.generateImplementationMethods(PointerGenerator.java:567)
00:08:49 	at com.ibm.j9ddr.tools.PointerGenerator.generateClass(PointerGenerator.java:329)
00:08:49 	at com.ibm.j9ddr.tools.PointerGenerator.generateClasses(PointerGenerator.java:157)
00:08:49 	at com.ibm.j9ddr.tools.PointerGenerator.main(PointerGenerator.java:117)

xlinux jdk11 build failed because of

Compiling 3085 files for java.base
error: module not found: jdk.hotspot.agent
Creating support/symbols/ct.sym
ar: creating libcrypto.a
Staging OpenJ9 sourcetools in /home/jenkins/workspace/PullRequest-Sanity-JDK11-linux_x86-64_cmprssptrs-OpenJ9/build/linux-x86_64-normal-server-release/vm
Overriding jvm.cfg with J9VM version
/bin/ln: failed to create symbolic link '/home/jenkins/workspace/PullRequest-Sanity-JDK11-linux_x86-64_cmprssptrs-OpenJ9/build/linux-x86_64-normal-server-release/jdk/lib/jvm.cfg': File exists
ModuleWrapper.gmk:72: recipe for target '/home/jenkins/workspace/PullRequest-Sanity-JDK11-linux_x86-64_cmprssptrs-OpenJ9/build/linux-x86_64-normal-server-release/jdk/lib/jvm.cfg' failed
gmake[3]: *** [/home/jenkins/workspace/PullRequest-Sanity-JDK11-linux_x86-64_cmprssptrs-OpenJ9/build/linux-x86_64-normal-server-release/jdk/lib/jvm.cfg] Error 1
gmake[3]: *** Deleting file '/home/jenkins/workspace/PullRequest-Sanity-JDK11-linux_x86-64_cmprssptrs-OpenJ9/build/linux-x86_64-normal-server-release/jdk/lib/jvm.cfg'
gmake[2]: *** [java.base-copy] Error 1
gmake[2]: *** Waiting for unfinished jobs....
make/Main.gmk:162: recipe for target 'java.base-copy' failed

@dsouzai
Copy link
Contributor

dsouzai commented Jan 7, 2019

Jenkins test sanity xlinux jdk8,jdk11

@andrewcraik
Copy link
Contributor

@jdmpapin has this stalled in approval? that error looks like a problem that may have been fixed now in head? Just mindful of the list of changes in #3752 and trying to keep things moving.

@dsouzai
Copy link
Contributor

dsouzai commented Jan 9, 2019

that error looks like a problem that may have been fixed now in head

If that's true then @jdmpapin could you rebase and I'll launch the PR builds again.

@jdmpapin
Copy link
Contributor Author

jdmpapin commented Jan 9, 2019

The failures were two occurrences of this:

 [OUT] An exception has occurred in the compiler (11.0.1-internal). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program and the following diagnostic in your report. Thank you.
 [OUT] java.lang.NullPointerException
 [OUT] 	at java.base/jdk.internal.jrtfs.JrtPath.resolve(JrtPath.java:282)
 [OUT] 	at java.base/jdk.internal.jrtfs.JrtPath.resolve(JrtPath.java:343)
 [OUT] 	at java.base/jdk.internal.jrtfs.JrtPath.resolve(JrtPath.java:58)
 [OUT] 	at jdk.compiler/com.sun.tools.javac.file.RelativePath.resolveAgainst(RelativePath.java:60)
 ...

The same issue has appeared elsewhere. It might be #4166 (where I found that link), but regardless it's happening without the changes from this PR

@dsouzai
Copy link
Contributor

dsouzai commented Jan 9, 2019

I'm happy to merge now we know the error is not caused by this PR.

@dsouzai dsouzai merged commit 4132ae3 into eclipse-openj9:master Jan 9, 2019
@jdmpapin jdmpapin deleted the svm.clarity branch April 9, 2021 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsouzai dsouzai dsouzai approved these changes

Assignees

@dsouzai dsouzai

Labels
comp:jit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdmpapin @dsouzai @andrewcraik