8297271: AccessFlags should be specific to class file version #11399
Conversation
|
👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into |
openjdk
bot
added
csr
|
@RogerRiggs The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
security
Webrevs
|
Hmm. I don't agree with that characterization of the current situation. The AccessFlag class has a method that takes a ClassFileFormatVersion argument and there are regression tests of the AccessFlag feature that user downrev versions of class file to exercise all the functionality: It is more accurate to say that the class file version is an implicit argument to all the give-me-the-access-flags functionality.
The current implementation does rely on the JVM "doing the right thing"(TM) in terms of screening out access_flags bits that are not appropriate for the class file version. However, I don't think this needs to be underlined, necessarily. Perhaps a sealed interface, strawman name AccessFlags, stated such a requirement for the core reflection classes? |
|
Yes, the location information via |
|
/label remove security |
|
@wangweij |
I don't agree the assessment that the API implicitly assumes the class file version used to compute the access flags is the latest one supported by the JVM. In the context of the accessFlags methods defined in core reflection, the implementation in the Java libraries and the specification implicitly assume the JVM "does the right thing" with respect to access flags per JVMS 4.1. The ClassFile Structure: "All bits of the access_flags item not assigned in Table 4.1-B are reserved for future use. They should be set to zero in generated class files and should be ignored by Java Virtual Machine implementations." So the desired (and actual observed) behavior is that in effect a class file version specific mask is applied to screen out the access flag bits before they are handed back to the core reflection library code to build the Set for a class, method, field, etc. |
…file version number. Removed unnecessary tests of ACC_SYNTHETIC, the class file format version tests for strictfp cover them sufficiently.
| @@ -1340,6 +1342,7 @@ private Class<?> elementType() { | |||
| * <li> its {@code ABSTRACT} and {@code FINAL} flags are present | |||
| * <li> its {@code INTERFACE} flag is absent, even when the | |||
| * component type is an interface | |||
| * <li> its class file format version is that of the component class | |||
There was a problem hiding this comment.
Please remove this requirement.
First, I'm not sure if it is true of the implementation. Even if it were true today, I don't think it is necessary to guarantee this as the class file format version is not directly retrievable by end-users (nor do I think it should be).
There was a problem hiding this comment.
For arrays, the implementation does use the cffv of the element type and it is a natural extension of the description of Class.accessFlags() using some of the modifiers (public, protected, and private) of the component type (as written a couple of lines above). But it may be a bit of overreach to say that its appropriate for other modifiers of an array to have the same behavior.
There was a problem hiding this comment.
For the purposes at hand of specifying the access flags as dependent on the class file format version, if the access flags are already completely specified, there is no need to mention a possible dependence on the class file format version, especially give the spec from a few lines down:
- For {@code Class} objects representing void, primitive types, and
- arrays, access flags are absent other than as specified above.
| * @param location context to interpret mask value | ||
| * @param cffv the class file format version | ||
| */ | ||
| public static Set<AccessFlag> maskToAccessFlags(int mask, Location location, |
There was a problem hiding this comment.
The difference in exception handling behavior compared to the method w/o a ClassFileFormatVersion argument should at least be discussed.
There was a problem hiding this comment.
I would propose to say:
Mask bits that do not match an {@code AccessFlag} for the location and
class file format version are ignored.
The case arises when the mask argument contains mask bits that are not consistent with the locations defined for any AccessFlag.
The current method throws IllegalArgumentException, while the added method returns a set of AccessFlags appropriate to the location and cffv and ignores the undefined mask bits.
If the source of the error/inconsistency is an application developer calling maskToAccessFlags() then IAE makes sense.
However, it would puzzling if a call to Class.accessFlags() threw IllegalArgumentException; in that case the mask bits are those of a loaded class, presumed to be conforming to the spec, but none the less having unexpected mask bits.
Such an recurrence is likely very rare but might be the result of a class file created by some means other than the javac compiler. The declaration of each AccessFlag implements the location and cffv information and corresponding mask bits according the JVM spec. If the VM loads the class, then the question is whether the inconsistency should be reported and if so how.
There was a problem hiding this comment.
From an end-user perspective, "the system" should screen out or otherwise take care of undefined/inappropriate modifier bits and access flags in loaded class files.
In the JDK implementation, there is a separation between the HotSpot JVM and the class libraries, in this case core reflection. One could argue -- and as a maintainer of the core reflection libraries I would argue -- that HotSpot should screen out undefined/inappropriate access flag bits before presenting them to the user. If that is not done, with the non-public method to return the class file format version of the class file, the Java libraries side of core reflection has enough information to do such screening on its own. (IIRC, from some off-list discussions @RogerRiggs ran into cases where a hand-crafted class file could have the ACC_SYNTHETIC flag set on a class for a version of the class file format where ACC_SYNTHETIC wasn't defined. This flag was passed from HotSpot to the core libraries, running afoul of the stricter checking on the existing maskToAccessFlags method.)
There are class file / byte code processing APIs where showing all the access flag bits, even when not defined for a class file versions, is the right answer. However, I don't think core reflection is one of those APIs. In the spirit of JVMS "4.1. The ClassFile Structure":
"All bits of the access_flags item not assigned in Table 4.1-B are reserved for future use. They should be set to zero in generated class files and should be ignored by Java Virtual Machine implementations."
I think it would be reasonable to mask out undefined bits either in HotSpot or the core reflection libraries.
There was a problem hiding this comment.
Should the masking out unassigned bits that is done in this method be extended to the existing AccessFlag.maskToAccessFlags(mask, location); Instead of throwing IllegalArgumentException?
The two methods should be consistent in this regard and build their return values on the meta-data in each AccessFlag.
There was a problem hiding this comment.
Should the masking out unassigned bits that is done in this method be extended to the existing
AccessFlag.maskToAccessFlags(mask, location); Instead of throwingIllegalArgumentException? The two methods should be consistent in this regard and build their return values on the meta-data in each AccessFlag.
My prior comment
"The difference in exception handling behavior compared to the method w/o a ClassFileFormatVersion argument should at least be discussed."
was calling attention to the differing strict vs lax handling of unexpected bits in the two overloaded methods. If the policies are not the same, there should at least be prominent text nothing and explaining the difference.
There was a problem hiding this comment.
…nent type AccessFlag.maskToAccessFlags(mask, location, cffv): clarify the handling of undefined mask flags
|
@RogerRiggs This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
The accessFlags() methods added (in JDK 20, the current release) to java.lang.Class, java.lang.reflect.Executable, and java.lang.reflect.Field implicitly uses the access flags from the current/most recent class file format version. For current and past class file format versions there are few significant variations but future changes are anticipated that change the meaning of some access flag mask bits.
The accessFlags() methods are clarified to return the access flags that are applicable to the class file format version of the class. The existing AccessFlag.Locations API already contains information about the locations that are applicable to a class file format version. That information should be used to construct the set of AccessFlags returned. A method is added to AccessFlag that returns the applicable flags for a particular mask, Location, and class file format version:
Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11399/head:pull/11399$ git checkout pull/11399Update a local copy of the PR:
$ git checkout pull/11399$ git pull https://git.openjdk.org/jdk pull/11399/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11399View PR using the GUI difftool:
$ git pr show -t 11399Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11399.diff