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

Adds static and dynamic verification for ConstantDynamic Entry #1858

Merged
merged 3 commits into from
Sep 11, 2018
Merged

Adds static and dynamic verification for ConstantDynamic Entry #1858

merged 3 commits into from
Sep 11, 2018

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented May 4, 2018 •
edited
Loading

  • Add validation to ldc ldc_w and ldc2_w for ConstantDynamic entry
  • Check that the Condy return type matches the bytecode
  • Refactor ldc with more informative error messages
  • Update description comments on pushLdcType(...)
  • Update pushLdcType to expect a constant dynamic to provide a field
  • Create new NLS entries for constantDynamic errors

Replaces: #1631
Close: #1278

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

@taliamccormick taliamccormick mentioned this pull request May 12, 2018
Closed
@Ailloviee Ailloviee mentioned this pull request Jul 19, 2018
Open
@fengxue-IS fengxue-IS force-pushed the condyVerifier branch 3 times, most recently from 6afd1d0 to c49779d Compare August 30, 2018 15:44
@fengxue-IS fengxue-IS changed the title WIP: Adds static and dynamic verification for ConstantDynamic Entry Adds static and dynamic verification for ConstantDynamic Entry Aug 30, 2018
@fengxue-IS
Copy link
Contributor Author

@ChengJin01 can you take a look please

ChengJin01
ChengJin01 suggested changes Sep 5, 2018
View reviewed changes
runtime/bcverify/staticverify.c Outdated
} else if (('D' == constantDynamicSignature->bytes[0])
|| ('J' == constantDynamicSignature->bytes[0])
) {
ldcErrorType = J9NLS_CFR_ERR_BC_LDC_CONSTANT_DYNAMIC_RETURNS_LONG_OR_DOUBLE;
Copy link

Choose a reason for hiding this comment

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

Should it be "J9NLS_CFR_ERR_BC_LDC_CONSTANT_DYNAMIC_RETURNS_LONG_OR_DOUBLE__ID" ?

runtime/bcverify/staticverify.c Outdated
if (CFR_CONSTANT_Dynamic == tag) {
J9CfrConstantPoolInfo *constantDynamicSignature = &classfile->constantPool[classfile->constantPool[info->slot2].slot2];
if ( ((flags & BCT_MajorClassFileVersionMask) < BCT_Java11MajorVersionShifted)
&& ((flags & BCT_MajorClassFileVersionMask) != 0)
Copy link

Choose a reason for hiding this comment

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

Please indent against the coding guide for if statement. I recall it should be:
if ((..)
&& ()
) {....

runtime/bcverify/staticverify.c Outdated
default:
ldcErrorType = J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT_OR_CONSTANT_DYNAMIC__ID;
}
if (0 != ldcErrorType) {
Copy link

Choose a reason for hiding this comment

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

There is no need to introduce a new local variable ldcErrorType as errorType has been initialized to 0 at the very beginning of the function.

@fengxue-IS fengxue-IS force-pushed the condyVerifier branch 4 times, most recently from 7aadb82 to 3809173 Compare September 6, 2018 15:21
@fengxue-IS
Copy link
Contributor Author

fixed code format, @ChengJin01 please take a look, thanks

ChengJin01
ChengJin01 suggested changes Sep 6, 2018
View reviewed changes
runtime/bcverify/staticverify.c Outdated
if (CFR_CONSTANT_Dynamic == tag) {
J9CfrConstantPoolInfo *constantDynamicSignature = &classfile->constantPool[classfile->constantPool[info->slot2].slot2];
if (((flags & BCT_MajorClassFileVersionMask) < BCT_Java11MajorVersionShifted)
&& (0 != (flags & BCT_MajorClassFileVersionMask))
Copy link

@ChengJin01 ChengJin01 Sep 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Better use J9_ARE_ANY_BITS_SET() for (0 != (flags & BCT_MajorClassFileVersionMask) and other similar cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@fengxue-IS fengxue-IS force-pushed the condyVerifier branch 2 times, most recently from f89c939 to 11ca45e Compare September 6, 2018 19:09
@fengxue-IS
Copy link
Contributor Author

per discussion with @DanHeidinga, removed the zero major version check on constant dynamic as cfdumper no longer sets default classfile version to zero.

@ChengJin01
Copy link

lgtm

@fengxue-IS
Copy link
Contributor Author

fengxue-IS commented Sep 6, 2018 •
edited
Loading

Senior reviewer: @DanHeidinga please take a look

DanHeidinga
DanHeidinga reviewed Sep 7, 2018
View reviewed changes
runtime/bcverify/staticverify.c Outdated
break;
case CFR_CONSTANT_Class:
if (((flags & BCT_MajorClassFileVersionMask) < BCT_Java5MajorVersionShifted)
&& (J9_ARE_ANY_BITS_SET(flags, BCT_MajorClassFileVersionMask))
Copy link
Member

Choose a reason for hiding this comment

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

given the way this has been rewritten, it makes sense to just remove the && (J9_ARE_ANY_BITS_SET(flags, BCT_MajorClassFileVersionMask)) as I don't believe it's required anymore.

Previously, cfdump used it.

runtime/nls/cfre/cfrerr.nls Outdated
# END NON-TRANSLATABLE

#ldc and ldc_w are not translatable
J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT_OR_CONSTANT_DYNAMIC=The ldc and ldc_2 bytecodes must reference a constant or a constant dynamic
Copy link
Member

Choose a reason for hiding this comment

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

constant dynamic -> constant_dynamic

runtime/nls/cfre/cfrerr.nls Outdated
#ldc and ldc_w are not translatable
J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT_OR_CONSTANT_DYNAMIC=The ldc and ldc_2 bytecodes must reference a constant or a constant dynamic
# START NON-TRANSLATABLE
J9NLS_CFR_ERR_BC_LDC_NOT_CONSTANT_OR_CONSTANT_DYNAMIC.explanation=An 'ldc' or 'ldc_w' bytecode can only reference constant values or a constant dynamic entry.
Copy link
Member

Choose a reason for hiding this comment

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

same here

runtime/nls/cfre/cfrerr.nls Outdated
# END NON-TRANSLATABLE

#ldc2_w is not translatable
J9NLS_CFR_ERR_BC_LDC2W_NOT_CONSTANT_OR_CONSTANT_DYNAMIC=The ldc2_w bytecode must reference a constant or a constant dynamic
Copy link
Member

Choose a reason for hiding this comment

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

same

runtime/nls/cfre/cfrerr.nls Outdated
# END NON-TRANSLATABLE

#ldc and ldc_w are not translatable
J9NLS_CFR_ERR_BC_LDC_CONSTANT_DYNAMIC_RETURNS_LONG_OR_DOUBLE=Constant dynamic entries referenced by the ldc and ldc_w bytecodes must not return long or double
Copy link
Member

Choose a reason for hiding this comment

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

same. Might be better to use Constant_Dynamic

runtime/nls/cfre/cfrerr.nls Outdated
# END NON-TRANSLATABLE

#ldc2_w is not translatable
J9NLS_CFR_ERR_BC_LDC2W_CONSTANT_DYNAMIC_NOT_LONG_OR_DOUBLE=Constant dynamic entries referenced by the ldc2_w bytecode must return long or double
Copy link
Member

Choose a reason for hiding this comment

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

same

Talia McCormick and others added 3 commits September 7, 2018 19:27
@fengxue-IS
Adds static and dynamic verification for ConDy
2caef91 
- Add validation to ldc ldc_w and ldc2_w for ConstantDynamic entry
- Check that the Condy return type matches the bytecode
- Refactor ldc with more informative error messages
- Update description comments on pushLdcType(...)
- Update pushLdcType to expect a constant dynamic to provide a field
- Refactor ldc error codes

Code by Talia McCormick from eclipse-openj9#1631

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Fix verifier errors in ConstantDynamic code
343ebcf 
- fixed constant pool item access macro names
- corrected Java majorVersion checks in static verifier
- update NLS messages

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Refactor Static verification of Constant_Dynamic
b9ad8a5 
- Removed legacy check condition that no longer apply
- Updated wording of the error message to be more clear

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

removed the legacy check condition, changed nls to use Constant_Dynamic, also modified some error message wording to make the text more clear to users

DanHeidinga
DanHeidinga approved these changes Sep 10, 2018
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

lgtm. @ChengJin01 Any other comments on this?

@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux jdk8,jdk11

@DanHeidinga DanHeidinga self-assigned this Sep 10, 2018
ChengJin01
ChengJin01 approved these changes Sep 10, 2018
View reviewed changes
Copy link

@ChengJin01 ChengJin01 left a comment

Choose a reason for hiding this comment

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

I am OK with the latest update.

@AdamBrousseau
Copy link
Contributor

Jenkins line endings check

@DanHeidinga
Copy link
Member

The extended test failures are the jdk11 issues we're still working through. Not related to this PR

@DanHeidinga DanHeidinga merged commit 0096ddb into eclipse-openj9:master Sep 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga approved these changes

@ChengJin01 ChengJin01 ChengJin01 approved these changes

Assignees

@DanHeidinga DanHeidinga

Labels
comp:vm jdk11
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fengxue-IS @ChengJin01 @DanHeidinga @AdamBrousseau @pshipton