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

JEP 359 part1: attribute & isRecord support #8061

Merged
merged 1 commit into from
Feb 8, 2020
Merged

JEP 359 part1: attribute & isRecord support #8061

merged 1 commit into from
Feb 8, 2020

Conversation

theresa-m
Copy link
Contributor

Part of: #7945

  • vm support for the Record attribute (class verification and addition to ROM class)
  • Class.isRecord implementation
  • tests for Class.isRecord

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

@theresa-m theresa-m mentioned this pull request Dec 11, 2019
Closed
25 tasks
@theresa-m theresa-m changed the title JEP 359 attribute & isRecord support JEP 359 part 1: record attribute & isRecord support Dec 11, 2019
@theresa-m theresa-m changed the title JEP 359 part 1: record attribute & isRecord support JEP 359 part 1: attribute & isRecord support Dec 11, 2019
@theresa-m theresa-m changed the title JEP 359 part 1: attribute & isRecord support JEP 359 part1: attribute & isRecord support Dec 11, 2019
DanHeidinga
DanHeidinga reviewed Dec 17, 2019
View reviewed changes
runtime/bcutil/ROMClassBuilder.cpp Show resolved Hide resolved
runtime/bcutil/ROMClassWriter.cpp Show resolved Hide resolved
runtime/bcutil/ROMClassWriter.cpp Outdated Show resolved Hide resolved
runtime/bcutil/cfreader.c Outdated Show resolved Hide resolved
test/functional/Java14andUp/playlist.xml Show resolved Hide resolved
@theresa-m theresa-m force-pushed the records_isrecord branch 3 times, most recently from 525de5a to 7000a4c Compare January 2, 2020 20:00
@theresa-m
Copy link
Contributor Author

New year copyright update.

@theresa-m theresa-m mentioned this pull request Jan 20, 2020
Merged
@theresa-m theresa-m mentioned this pull request Jan 23, 2020
Merged
DanHeidinga
DanHeidinga reviewed Jan 29, 2020
View reviewed changes
runtime/bcutil/ClassFileOracle.cpp Show resolved Hide resolved
runtime/bcutil/cfreader.c Outdated Show resolved Hide resolved
runtime/bcutil/cfreader.c Outdated
@@ -765,6 +766,42 @@ readAttributes(J9CfrClassFile * classfile, J9CfrAttribute *** pAttributes, U_32
memcpy (stackMap->entries, index, stackMap->mapLength);
index += stackMap->mapLength;

break;
case CFR_ATTRIBUTE_Record:
/* There should be no more than one record attribute in a ClassFile. */
Copy link
Member

Choose a reason for hiding this comment

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

And one record attribute in a ClassFile. Is there a spec reference for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. 4.7.30
There may be at most one Record attribute in the attributes table of a ClassFile structure.
I will reference the section in my comment.

runtime/bcutil/cfreader.c Outdated Show resolved Hide resolved
runtime/bcutil/cfreader.c Outdated Show resolved Hide resolved
runtime/oti/cfr.h Show resolved Hide resolved
test/functional/Java14andUp/src/org/openj9/test/java/lang/Test_Class.java Outdated Show resolved Hide resolved
runtime/bcutil/ROMClassWriter.cpp Show resolved Hide resolved
runtime/bcutil/ClassFileOracle.hpp Outdated Show resolved Hide resolved
runtime/oti/cfr.h Outdated
@@ -902,6 +919,7 @@ typedef struct J9CfrClassFile {
#define CFR_DECODE_J9_MULTIANEWARRAY 23
#define CFR_DECODE_J9_METHODTYPEREF 24
#define CFR_J9FLAG_HAS_JSR 1
#define CFR_J9FLAG_IS_RECORD 2
Copy link
Member

Choose a reason for hiding this comment

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

Can you point at why cfdump needs this? It would be cleaner to add this change (and related code) to the PR that requires it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move this to the cfdumper pr.
I use this in cfdumper as a way to identify a record class when displaying a J9CfrClassFile and not a loaded rom class.

@theresa-m theresa-m force-pushed the records_isrecord branch 2 times, most recently from dcdb2ae to 6be96a7 Compare January 29, 2020 21:25
@theresa-m theresa-m force-pushed the records_isrecord branch 2 times, most recently from 06a9639 to 11874c3 Compare February 3, 2020 22:28
DanHeidinga
DanHeidinga reviewed Feb 5, 2020
View reviewed changes
runtime/bcutil/ClassFileOracle.cpp Show resolved Hide resolved
runtime/bcutil/ROMClassWriter.cpp Outdated Show resolved Hide resolved
runtime/bcutil/cfreader.c Outdated Show resolved Hide resolved
@DanHeidinga DanHeidinga added this to the Release 0.19 (Java 14) milestone Feb 6, 2020
@DanHeidinga
Copy link
Member

Jenkins test sanity plinux jdk14,jdk11

@DanHeidinga DanHeidinga self-assigned this Feb 6, 2020
@DanHeidinga
Copy link
Member

@theresa-m Please comment on the state of the builds before you address the missing newline. Assuming they pass now, I'd like to commit without running a new build for just the newline change but need to have the results recorded before any new commits are pushed.

@pshipton
Copy link
Member

pshipton commented Feb 6, 2020

The builds passed https://ci.eclipse.org/openj9/job/PullRequest-OpenJ9/2624/

@theresa-m
JEP 359 attribute & isRecord support
36e92bf 
Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>
@theresa-m
Copy link
Contributor Author

I've added the newline change to this commit now.

@DanHeidinga DanHeidinga merged commit 6a356f9 into eclipse-openj9:master Feb 8, 2020
This was referenced Feb 10, 2020
Merged
Closed
@theresa-m theresa-m deleted the records_isrecord branch February 24, 2020 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga left review comments

Assignees

@DanHeidinga DanHeidinga

Labels
comp:vm jdk14
Projects
None yet
Milestone
Release 0.19 (Java 14)
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@theresa-m @DanHeidinga @pshipton