Quantcast

RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Hi,

Please review this small change to throw a NoClassDefFoundError
exception, for class file versions >= 53, if a class's access_flags have
ACC_MODULE set.  This behavior will be required in the upcoming JVM-9 Spec.

Open Web: http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html

JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725


The fix was tested with JPRT, the hotspot, java/lang, java/util,
java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated NSK
tests.

Thanks, Harold

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

Lois Foltan
Looks good.
Lois

On 2/14/2017 9:52 AM, harold seigel wrote:

> Hi,
>
> Please review this small change to throw a NoClassDefFoundError
> exception, for class file versions >= 53, if a class's access_flags
> have ACC_MODULE set.  This behavior will be required in the upcoming
> JVM-9 Spec.
>
> Open Web:
> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>
>
> The fix was tested with JPRT, the hotspot, java/lang, java/util,
> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
> NSK tests.
>
> Thanks, Harold
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Thanks Lois!

Harold


On 2/14/2017 9:55 AM, Lois Foltan wrote:

> Looks good.
> Lois
>
> On 2/14/2017 9:52 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this small change to throw a NoClassDefFoundError
>> exception, for class file versions >= 53, if a class's access_flags
>> have ACC_MODULE set.  This behavior will be required in the upcoming
>> JVM-9 Spec.
>>
>> Open Web:
>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>
>>
>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>> NSK tests.
>>
>> Thanks, Harold
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

George Triantafillou
In reply to this post by harold seigel
Hi Harold,

Looks good.

-George

On 2/14/2017 9:52 AM, harold seigel wrote:

> Hi,
>
> Please review this small change to throw a NoClassDefFoundError
> exception, for class file versions >= 53, if a class's access_flags
> have ACC_MODULE set.  This behavior will be required in the upcoming
> JVM-9 Spec.
>
> Open Web:
> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>
>
> The fix was tested with JPRT, the hotspot, java/lang, java/util,
> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
> NSK tests.
>
> Thanks, Harold
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Thanks George!

Harold


On 2/14/2017 10:14 AM, George Triantafillou wrote:

> Hi Harold,
>
> Looks good.
>
> -George
>
> On 2/14/2017 9:52 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this small change to throw a NoClassDefFoundError
>> exception, for class file versions >= 53, if a class's access_flags
>> have ACC_MODULE set.  This behavior will be required in the upcoming
>> JVM-9 Spec.
>>
>> Open Web:
>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>
>>
>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>> NSK tests.
>>
>> Thanks, Harold
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

Alan Bateman
In reply to this post by harold seigel


On 14/02/2017 14:52, harold seigel wrote:

> Hi,
>
> Please review this small change to throw a NoClassDefFoundError
> exception, for class file versions >= 53, if a class's access_flags
> have ACC_MODULE set.  This behavior will be required in the upcoming
> JVM-9 Spec.
>
> Open Web:
> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>
>
> The fix was tested with JPRT, the hotspot, java/lang, java/util,
> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
> NSK tests.
This looks okay to me although I think I would name the test case
"BadAccModule" rather than "badAccModule" as it's a bit unusual to have
a class name starting with a lower case letter.

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Thanks Alan.

I'll rename the class.  I also need to add 'return' statements after the
fthrow() calls that I added.

Harold


On 2/14/2017 10:32 AM, Alan Bateman wrote:

>
>
> On 14/02/2017 14:52, harold seigel wrote:
>> Hi,
>>
>> Please review this small change to throw a NoClassDefFoundError
>> exception, for class file versions >= 53, if a class's access_flags
>> have ACC_MODULE set.  This behavior will be required in the upcoming
>> JVM-9 Spec.
>>
>> Open Web:
>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>
>>
>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>> NSK tests.
> This looks okay to me although I think I would name the test case
> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
> have a class name starting with a lower case letter.
>
> -Alan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
In reply to this post by Alan Bateman
Hi,

Please review this updated webrev:
http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html

This webrev has 'return' statements after the calls to fthrow() and a
new test case for a class file in an InnerClasses attribute that has
ACC_MODULE set.

Thanks, Harold


On 2/14/2017 10:32 AM, Alan Bateman wrote:

>
>
> On 14/02/2017 14:52, harold seigel wrote:
>> Hi,
>>
>> Please review this small change to throw a NoClassDefFoundError
>> exception, for class file versions >= 53, if a class's access_flags
>> have ACC_MODULE set.  This behavior will be required in the upcoming
>> JVM-9 Spec.
>>
>> Open Web:
>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>
>>
>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>> NSK tests.
> This looks okay to me although I think I would name the test case
> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
> have a class name starting with a lower case letter.
>
> -Alan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

David Holmes
Hi Harold,

On 15/02/2017 9:53 AM, harold seigel wrote:
> Hi,
>
> Please review this updated webrev:
> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html

I'm unclear why this logic is needed in two places instead of just
putting it inside ClassFileParser::verify_legal_class_modifiers ?

Also with regards to the error message ... I think it is expressed
inappropriately for the NCDFE case. If we were throwing ClassFormatError
then "Illegal ACC_MODULE class modifier ..." would be appropriate. But
for NCDFE we need to phrase it in terms of the inability to find the
class in the given class representation - I suggest:

"%s claims to be a module (ACC_MODULE is set)"

Thanks,
David
-----

> This webrev has 'return' statements after the calls to fthrow() and a
> new test case for a class file in an InnerClasses attribute that has
> ACC_MODULE set.
>
> Thanks, Harold
>
>
> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>
>>
>> On 14/02/2017 14:52, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this small change to throw a NoClassDefFoundError
>>> exception, for class file versions >= 53, if a class's access_flags
>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>> JVM-9 Spec.
>>>
>>> Open Web:
>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>
>>>
>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>> NSK tests.
>> This looks okay to me although I think I would name the test case
>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>> have a class name starting with a lower case letter.
>>
>> -Alan
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Hi David,

I did not put the check in verify_legal_class_modifiers() because it
only checks the modifiers if _need_verify is set.  However, we want to
always check for ACC_MODULE (in class files >= 53) regardless of
_need_verify's value.  So verify_legal_class_modifiers() would need a
special case to check for ACC_MODULE.

Also, since a classfile version check is already needed in
parse_stream(), when fetching the access_flags from the stream, it was
convenient to just do the check and possible throw right there.  
However, if you think it's important, I'll move the check to
verify_legal_class_modifiers().

Thanks for proposing a better error message.  Instead of saying 'claims
to be a module', how about:

    "% is not a class because access_flag ACC_MODULE is set"

Thanks, Harold

On 2/14/2017 8:00 PM, David Holmes wrote:

> Hi Harold,
>
> On 15/02/2017 9:53 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this updated webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>
> I'm unclear why this logic is needed in two places instead of just
> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>
> Also with regards to the error message ... I think it is expressed
> inappropriately for the NCDFE case. If we were throwing
> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
> appropriate. But for NCDFE we need to phrase it in terms of the
> inability to find the class in the given class representation - I
> suggest:
>
> "%s claims to be a module (ACC_MODULE is set)"
>
> Thanks,
> David
> -----
>
>> This webrev has 'return' statements after the calls to fthrow() and a
>> new test case for a class file in an InnerClasses attribute that has
>> ACC_MODULE set.
>>
>> Thanks, Harold
>>
>>
>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>
>>>
>>> On 14/02/2017 14:52, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this small change to throw a NoClassDefFoundError
>>>> exception, for class file versions >= 53, if a class's access_flags
>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>> JVM-9 Spec.
>>>>
>>>> Open Web:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>
>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>
>>>>
>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>> NSK tests.
>>> This looks okay to me although I think I would name the test case
>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>> have a class name starting with a lower case letter.
>>>
>>> -Alan
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

David Holmes
Hi Harold,

On 15/02/2017 11:30 PM, harold seigel wrote:
> Hi David,
>
> I did not put the check in verify_legal_class_modifiers() because it
> only checks the modifiers if _need_verify is set.  However, we want to
> always check for ACC_MODULE (in class files >= 53) regardless of
> _need_verify's value.  So verify_legal_class_modifiers() would need a
> special case to check for ACC_MODULE.

That raises the question as to what classfile "format" checks are
unconditional and which are only carried out when "verification" is
enabled ??

> Also, since a classfile version check is already needed in
> parse_stream(), when fetching the access_flags from the stream, it was
> convenient to just do the check and possible throw right there.
> However, if you think it's important, I'll move the check to
> verify_legal_class_modifiers().

It seems like a good place for it (not withstanding it currently returns
immediately) and avoid the need to duplicate the code. It is the
duplication I disliked - but "verify_legal_class_modifiers" certainly
sounds like the place that would check for ACC_MODULE.

> Thanks for proposing a better error message.  Instead of saying 'claims
> to be a module', how about:
>
>     "% is not a class because access_flag ACC_MODULE is set"

Okay I guess.

Thanks,
David

> Thanks, Harold
>
> On 2/14/2017 8:00 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this updated webrev:
>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>
>> I'm unclear why this logic is needed in two places instead of just
>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>
>> Also with regards to the error message ... I think it is expressed
>> inappropriately for the NCDFE case. If we were throwing
>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>> appropriate. But for NCDFE we need to phrase it in terms of the
>> inability to find the class in the given class representation - I
>> suggest:
>>
>> "%s claims to be a module (ACC_MODULE is set)"
>>
>> Thanks,
>> David
>> -----
>>
>>> This webrev has 'return' statements after the calls to fthrow() and a
>>> new test case for a class file in an InnerClasses attribute that has
>>> ACC_MODULE set.
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>
>>>>
>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>> JVM-9 Spec.
>>>>>
>>>>> Open Web:
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>
>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>
>>>>>
>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>> NSK tests.
>>>> This looks okay to me although I think I would name the test case
>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>> have a class name starting with a lower case letter.
>>>>
>>>> -Alan
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Please review this new version of the fix for JDK-8174725:

    http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html

This fix moves the check for ACC_Module to
verify_legal_class_modifiers() as required by David.

 >> That raises the question as to what classfile "format" checks are
unconditional and which are only carried out when "verification" is
enabled ??

That is outside of the scope of this bug.  Feel free to open a new bug
for this issue.  Until then, just use common sense or ask Alex Buckley
what to do when adding a format check.

Thanks, Harold


On 2/15/2017 5:38 PM, David Holmes wrote:

> Hi Harold,
>
> On 15/02/2017 11:30 PM, harold seigel wrote:
>> Hi David,
>>
>> I did not put the check in verify_legal_class_modifiers() because it
>> only checks the modifiers if _need_verify is set.  However, we want to
>> always check for ACC_MODULE (in class files >= 53) regardless of
>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>> special case to check for ACC_MODULE.
>
> That raises the question as to what classfile "format" checks are
> unconditional and which are only carried out when "verification" is
> enabled ??
>
>> Also, since a classfile version check is already needed in
>> parse_stream(), when fetching the access_flags from the stream, it was
>> convenient to just do the check and possible throw right there.
>> However, if you think it's important, I'll move the check to
>> verify_legal_class_modifiers().
>
> It seems like a good place for it (not withstanding it currently
> returns immediately) and avoid the need to duplicate the code. It is
> the duplication I disliked - but "verify_legal_class_modifiers"
> certainly sounds like the place that would check for ACC_MODULE.
>
>> Thanks for proposing a better error message.  Instead of saying 'claims
>> to be a module', how about:
>>
>>     "% is not a class because access_flag ACC_MODULE is set"
>
> Okay I guess.
>
> Thanks,
> David
>
>> Thanks, Harold
>>
>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this updated webrev:
>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>
>>> I'm unclear why this logic is needed in two places instead of just
>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>
>>> Also with regards to the error message ... I think it is expressed
>>> inappropriately for the NCDFE case. If we were throwing
>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>> inability to find the class in the given class representation - I
>>> suggest:
>>>
>>> "%s claims to be a module (ACC_MODULE is set)"
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>> new test case for a class file in an InnerClasses attribute that has
>>>> ACC_MODULE set.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>
>>>>>
>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>>> JVM-9 Spec.
>>>>>>
>>>>>> Open Web:
>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>
>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>
>>>>>>
>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>>> NSK tests.
>>>>> This looks okay to me although I think I would name the test case
>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>> have a class name starting with a lower case letter.
>>>>>
>>>>> -Alan
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
To clarify about format checks:

Conditional checks remain conditional because of backward compatibility
concerns.  New checks should be unconditional unless there is an
explicit reason for them to be conditional.  With ACC_Module, there is
no reason for that check to be unconditional.

Thanks, Harold


On 2/17/2017 7:58 AM, harold seigel wrote:

> Please review this new version of the fix for JDK-8174725:
>
> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>
> This fix moves the check for ACC_Module to
> verify_legal_class_modifiers() as required by David.
>
> >> That raises the question as to what classfile "format" checks are
> unconditional and which are only carried out when "verification" is
> enabled ??
>
> That is outside of the scope of this bug.  Feel free to open a new bug
> for this issue.  Until then, just use common sense or ask Alex Buckley
> what to do when adding a format check.
>
> Thanks, Harold
>
>
> On 2/15/2017 5:38 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>> Hi David,
>>>
>>> I did not put the check in verify_legal_class_modifiers() because it
>>> only checks the modifiers if _need_verify is set.  However, we want to
>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>> special case to check for ACC_MODULE.
>>
>> That raises the question as to what classfile "format" checks are
>> unconditional and which are only carried out when "verification" is
>> enabled ??
>>
>>> Also, since a classfile version check is already needed in
>>> parse_stream(), when fetching the access_flags from the stream, it was
>>> convenient to just do the check and possible throw right there.
>>> However, if you think it's important, I'll move the check to
>>> verify_legal_class_modifiers().
>>
>> It seems like a good place for it (not withstanding it currently
>> returns immediately) and avoid the need to duplicate the code. It is
>> the duplication I disliked - but "verify_legal_class_modifiers"
>> certainly sounds like the place that would check for ACC_MODULE.
>>
>>> Thanks for proposing a better error message.  Instead of saying 'claims
>>> to be a module', how about:
>>>
>>>     "% is not a class because access_flag ACC_MODULE is set"
>>
>> Okay I guess.
>>
>> Thanks,
>> David
>>
>>> Thanks, Harold
>>>
>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this updated webrev:
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>
>>>> I'm unclear why this logic is needed in two places instead of just
>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>
>>>> Also with regards to the error message ... I think it is expressed
>>>> inappropriately for the NCDFE case. If we were throwing
>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>> inability to find the class in the given class representation - I
>>>> suggest:
>>>>
>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>> ACC_MODULE set.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>>>
>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>
>>>>>>
>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>>> have ACC_MODULE set.  This behavior will be required in the
>>>>>>> upcoming
>>>>>>> JVM-9 Spec.
>>>>>>>
>>>>>>> Open Web:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>
>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>
>>>>>>>
>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and
>>>>>>> non-colocated
>>>>>>> NSK tests.
>>>>>> This looks okay to me although I think I would name the test case
>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>> have a class name starting with a lower case letter.
>>>>>>
>>>>>> -Alan
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

coleen.phillimore

I agree.  We need a bug to figure out why there's this _need_verify flag
in classFileParser.cpp.   I think it was an optimization for classes on
the bootclasspath (formerly rt.jar) because they should never have any
errors.  But classFileParser does format checks which I can't see why
they'd be costly for performance (should be measured as part of the
upcoming bug).   The main effect of this _needs_verify flag here is to
confuse people looking at the code.

I've reviewed this and it looks like a good fix.  To me it makes no
difference if checking for module in verify_class_modifiers() is in
_needs_verify or not.

Thank you for adding comments in the .cod files in the tests.

Thanks,
Coleen

On 2/17/17 9:20 AM, harold seigel wrote:

> To clarify about format checks:
>
> Conditional checks remain conditional because of backward
> compatibility concerns.  New checks should be unconditional unless
> there is an explicit reason for them to be conditional.  With
> ACC_Module, there is no reason for that check to be unconditional.
>
> Thanks, Harold
>
>
> On 2/17/2017 7:58 AM, harold seigel wrote:
>> Please review this new version of the fix for JDK-8174725:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>
>> This fix moves the check for ACC_Module to
>> verify_legal_class_modifiers() as required by David.
>>
>> >> That raises the question as to what classfile "format" checks are
>> unconditional and which are only carried out when "verification" is
>> enabled ??
>>
>> That is outside of the scope of this bug.  Feel free to open a new
>> bug for this issue.  Until then, just use common sense or ask Alex
>> Buckley what to do when adding a format check.
>>
>> Thanks, Harold
>>
>>
>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>> Hi David,
>>>>
>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>> only checks the modifiers if _need_verify is set.  However, we want to
>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>> special case to check for ACC_MODULE.
>>>
>>> That raises the question as to what classfile "format" checks are
>>> unconditional and which are only carried out when "verification" is
>>> enabled ??
>>>
>>>> Also, since a classfile version check is already needed in
>>>> parse_stream(), when fetching the access_flags from the stream, it was
>>>> convenient to just do the check and possible throw right there.
>>>> However, if you think it's important, I'll move the check to
>>>> verify_legal_class_modifiers().
>>>
>>> It seems like a good place for it (not withstanding it currently
>>> returns immediately) and avoid the need to duplicate the code. It is
>>> the duplication I disliked - but "verify_legal_class_modifiers"
>>> certainly sounds like the place that would check for ACC_MODULE.
>>>
>>>> Thanks for proposing a better error message.  Instead of saying
>>>> 'claims
>>>> to be a module', how about:
>>>>
>>>>     "% is not a class because access_flag ACC_MODULE is set"
>>>
>>> Okay I guess.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks, Harold
>>>>
>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this updated webrev:
>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>
>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>
>>>>> Also with regards to the error message ... I think it is expressed
>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..."
>>>>> would be
>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>> inability to find the class in the given class representation - I
>>>>> suggest:
>>>>>
>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> This webrev has 'return' statements after the calls to fthrow()
>>>>>> and a
>>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>>> ACC_MODULE set.
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>>
>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>> exception, for class file versions >= 53, if a class's
>>>>>>>> access_flags
>>>>>>>> have ACC_MODULE set.  This behavior will be required in the
>>>>>>>> upcoming
>>>>>>>> JVM-9 Spec.
>>>>>>>>
>>>>>>>> Open Web:
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>
>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>
>>>>>>>>
>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests,
>>>>>>>> RBT
>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and
>>>>>>>> non-colocated
>>>>>>>> NSK tests.
>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>> have a class name starting with a lower case letter.
>>>>>>>
>>>>>>> -Alan
>>>>>>
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

Karen Kinnear
Code looks good.

In AccModuleTest, could you fix comment line 32: ClassFormatError -> NoClassDefFoundError
Thank you for testing inner classes as well.

Agree with Harold et al that this particular check should be unconditional. The meaning of this flag today could have been
expressed as a different magic number, so if this is a module-info.class file, we don’t want to look any further.

Part of the history of the _need_verify was for javaws which cached pre-verified files. It also is used for trusted class loaders.
Agreed that it is confusing, and if there is no performance benefit, totally worth removing the format checks on future
class file versions (so we don’t break code already in the field).

thanks,
Karen



> On Feb 17, 2017, at 10:37 AM, [hidden email] wrote:
>
>
> I agree.  We need a bug to figure out why there's this _need_verify flag in classFileParser.cpp.   I think it was an optimization for classes on the bootclasspath (formerly rt.jar) because they should never have any errors.  But classFileParser does format checks which I can't see why they'd be costly for performance (should be measured as part of the upcoming bug).   The main effect of this _needs_verify flag here is to confuse people looking at the code.
>
> I've reviewed this and it looks like a good fix.  To me it makes no difference if checking for module in verify_class_modifiers() is in _needs_verify or not.
>
> Thank you for adding comments in the .cod files in the tests.
>
> Thanks,
> Coleen
>
> On 2/17/17 9:20 AM, harold seigel wrote:
>> To clarify about format checks:
>>
>> Conditional checks remain conditional because of backward compatibility concerns.  New checks should be unconditional unless there is an explicit reason for them to be conditional.  With ACC_Module, there is no reason for that check to be unconditional.
>>
>> Thanks, Harold
>>
>>
>> On 2/17/2017 7:58 AM, harold seigel wrote:
>>> Please review this new version of the fix for JDK-8174725:
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>>
>>> This fix moves the check for ACC_Module to verify_legal_class_modifiers() as required by David.
>>>
>>> >> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>
>>> That is outside of the scope of this bug.  Feel free to open a new bug for this issue.  Until then, just use common sense or ask Alex Buckley what to do when adding a format check.
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>>> Hi David,
>>>>>
>>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>>> only checks the modifiers if _need_verify is set.  However, we want to
>>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>>> special case to check for ACC_MODULE.
>>>>
>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>>
>>>>> Also, since a classfile version check is already needed in
>>>>> parse_stream(), when fetching the access_flags from the stream, it was
>>>>> convenient to just do the check and possible throw right there.
>>>>> However, if you think it's important, I'll move the check to
>>>>> verify_legal_class_modifiers().
>>>>
>>>> It seems like a good place for it (not withstanding it currently returns immediately) and avoid the need to duplicate the code. It is the duplication I disliked - but "verify_legal_class_modifiers" certainly sounds like the place that would check for ACC_MODULE.
>>>>
>>>>> Thanks for proposing a better error message.  Instead of saying 'claims
>>>>> to be a module', how about:
>>>>>
>>>>>    "% is not a class because access_flag ACC_MODULE is set"
>>>>
>>>> Okay I guess.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks, Harold
>>>>>
>>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>>> Hi Harold,
>>>>>>
>>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this updated webrev:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>>
>>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>>
>>>>>> Also with regards to the error message ... I think it is expressed
>>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>>> inability to find the class in the given class representation - I
>>>>>> suggest:
>>>>>>
>>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>>>> ACC_MODULE set.
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>>
>>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>>>>>> JVM-9 Spec.
>>>>>>>>>
>>>>>>>>> Open Web:
>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>>
>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>>>>>> NSK tests.
>>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>>> have a class name starting with a lower case letter.
>>>>>>>>
>>>>>>>> -Alan
>>>>>>>
>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
Hi Karen,

Thanks for the review!  I'll fix the comment in AccModuleTest before
pushing the change.

Harold


On 2/17/2017 11:11 AM, Karen Kinnear wrote:

> Code looks good.
>
> In AccModuleTest, could you fix comment line 32: ClassFormatError -> NoClassDefFoundError
> Thank you for testing inner classes as well.
>
> Agree with Harold et al that this particular check should be unconditional. The meaning of this flag today could have been
> expressed as a different magic number, so if this is a module-info.class file, we don’t want to look any further.
>
> Part of the history of the _need_verify was for javaws which cached pre-verified files. It also is used for trusted class loaders.
> Agreed that it is confusing, and if there is no performance benefit, totally worth removing the format checks on future
> class file versions (so we don’t break code already in the field).
>
> thanks,
> Karen
>
>
>
>> On Feb 17, 2017, at 10:37 AM, [hidden email] wrote:
>>
>>
>> I agree.  We need a bug to figure out why there's this _need_verify flag in classFileParser.cpp.   I think it was an optimization for classes on the bootclasspath (formerly rt.jar) because they should never have any errors.  But classFileParser does format checks which I can't see why they'd be costly for performance (should be measured as part of the upcoming bug).   The main effect of this _needs_verify flag here is to confuse people looking at the code.
>>
>> I've reviewed this and it looks like a good fix.  To me it makes no difference if checking for module in verify_class_modifiers() is in _needs_verify or not.
>>
>> Thank you for adding comments in the .cod files in the tests.
>>
>> Thanks,
>> Coleen
>>
>> On 2/17/17 9:20 AM, harold seigel wrote:
>>> To clarify about format checks:
>>>
>>> Conditional checks remain conditional because of backward compatibility concerns.  New checks should be unconditional unless there is an explicit reason for them to be conditional.  With ACC_Module, there is no reason for that check to be unconditional.
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 2/17/2017 7:58 AM, harold seigel wrote:
>>>> Please review this new version of the fix for JDK-8174725:
>>>>
>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>>>
>>>> This fix moves the check for ACC_Module to verify_legal_class_modifiers() as required by David.
>>>>
>>>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>> That is outside of the scope of this bug.  Feel free to open a new bug for this issue.  Until then, just use common sense or ask Alex Buckley what to do when adding a format check.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>>>> only checks the modifiers if _need_verify is set.  However, we want to
>>>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>>>> special case to check for ACC_MODULE.
>>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>>>
>>>>>> Also, since a classfile version check is already needed in
>>>>>> parse_stream(), when fetching the access_flags from the stream, it was
>>>>>> convenient to just do the check and possible throw right there.
>>>>>> However, if you think it's important, I'll move the check to
>>>>>> verify_legal_class_modifiers().
>>>>> It seems like a good place for it (not withstanding it currently returns immediately) and avoid the need to duplicate the code. It is the duplication I disliked - but "verify_legal_class_modifiers" certainly sounds like the place that would check for ACC_MODULE.
>>>>>
>>>>>> Thanks for proposing a better error message.  Instead of saying 'claims
>>>>>> to be a module', how about:
>>>>>>
>>>>>>     "% is not a class because access_flag ACC_MODULE is set"
>>>>> Okay I guess.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>>>> Hi Harold,
>>>>>>>
>>>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>>>
>>>>>>> Also with regards to the error message ... I think it is expressed
>>>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>>>> inability to find the class in the given class representation - I
>>>>>>> suggest:
>>>>>>>
>>>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>>>>> ACC_MODULE set.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>>>
>>>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>>>>>>> JVM-9 Spec.
>>>>>>>>>>
>>>>>>>>>> Open Web:
>>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>>>
>>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>>>>>>> NSK tests.
>>>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>>>> have a class name starting with a lower case letter.
>>>>>>>>>
>>>>>>>>> -Alan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

harold seigel
In reply to this post by coleen.phillimore
Thanks Coleen!

Harold


On 2/17/2017 10:37 AM, [hidden email] wrote:

>
> I agree.  We need a bug to figure out why there's this _need_verify
> flag in classFileParser.cpp.   I think it was an optimization for
> classes on the bootclasspath (formerly rt.jar) because they should
> never have any errors.  But classFileParser does format checks which I
> can't see why they'd be costly for performance (should be measured as
> part of the upcoming bug). The main effect of this _needs_verify flag
> here is to confuse people looking at the code.
>
> I've reviewed this and it looks like a good fix.  To me it makes no
> difference if checking for module in verify_class_modifiers() is in
> _needs_verify or not.
>
> Thank you for adding comments in the .cod files in the tests.
>
> Thanks,
> Coleen
>
> On 2/17/17 9:20 AM, harold seigel wrote:
>> To clarify about format checks:
>>
>> Conditional checks remain conditional because of backward
>> compatibility concerns.  New checks should be unconditional unless
>> there is an explicit reason for them to be conditional. With
>> ACC_Module, there is no reason for that check to be unconditional.
>>
>> Thanks, Harold
>>
>>
>> On 2/17/2017 7:58 AM, harold seigel wrote:
>>> Please review this new version of the fix for JDK-8174725:
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>>
>>> This fix moves the check for ACC_Module to
>>> verify_legal_class_modifiers() as required by David.
>>>
>>> >> That raises the question as to what classfile "format" checks are
>>> unconditional and which are only carried out when "verification" is
>>> enabled ??
>>>
>>> That is outside of the scope of this bug.  Feel free to open a new
>>> bug for this issue.  Until then, just use common sense or ask Alex
>>> Buckley what to do when adding a format check.
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>>> Hi David,
>>>>>
>>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>>> only checks the modifiers if _need_verify is set. However, we want to
>>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>>> special case to check for ACC_MODULE.
>>>>
>>>> That raises the question as to what classfile "format" checks are
>>>> unconditional and which are only carried out when "verification" is
>>>> enabled ??
>>>>
>>>>> Also, since a classfile version check is already needed in
>>>>> parse_stream(), when fetching the access_flags from the stream, it
>>>>> was
>>>>> convenient to just do the check and possible throw right there.
>>>>> However, if you think it's important, I'll move the check to
>>>>> verify_legal_class_modifiers().
>>>>
>>>> It seems like a good place for it (not withstanding it currently
>>>> returns immediately) and avoid the need to duplicate the code. It
>>>> is the duplication I disliked - but "verify_legal_class_modifiers"
>>>> certainly sounds like the place that would check for ACC_MODULE.
>>>>
>>>>> Thanks for proposing a better error message.  Instead of saying
>>>>> 'claims
>>>>> to be a module', how about:
>>>>>
>>>>>     "% is not a class because access_flag ACC_MODULE is set"
>>>>
>>>> Okay I guess.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks, Harold
>>>>>
>>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>>> Hi Harold,
>>>>>>
>>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this updated webrev:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>>
>>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>>
>>>>>> Also with regards to the error message ... I think it is expressed
>>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..."
>>>>>> would be
>>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>>> inability to find the class in the given class representation - I
>>>>>> suggest:
>>>>>>
>>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> This webrev has 'return' statements after the calls to fthrow()
>>>>>>> and a
>>>>>>> new test case for a class file in an InnerClasses attribute that
>>>>>>> has
>>>>>>> ACC_MODULE set.
>>>>>>>
>>>>>>> Thanks, Harold
>>>>>>>
>>>>>>>
>>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>>> exception, for class file versions >= 53, if a class's
>>>>>>>>> access_flags
>>>>>>>>> have ACC_MODULE set.  This behavior will be required in the
>>>>>>>>> upcoming
>>>>>>>>> JVM-9 Spec.
>>>>>>>>>
>>>>>>>>> Open Web:
>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>>
>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM
>>>>>>>>> tests, RBT
>>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and
>>>>>>>>> non-colocated
>>>>>>>>> NSK tests.
>>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>>> have a class name starting with a lower case letter.
>>>>>>>>
>>>>>>>> -Alan
>>>>>>>
>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

David Holmes
In reply to this post by Karen Kinnear
Hi Karen,

On 18/02/2017 2:11 AM, Karen Kinnear wrote:
> Code looks good.
>
> In AccModuleTest, could you fix comment line 32: ClassFormatError -> NoClassDefFoundError
> Thank you for testing inner classes as well.
>
> Agree with Harold et al that this particular check should be unconditional. The meaning of this flag today could have been
> expressed as a different magic number, so if this is a module-info.class file, we don’t want to look any further.
>
> Part of the history of the _need_verify was for javaws which cached pre-verified files. It also is used for trusted class loaders.

The non-verification of classes from trusted loaders is the only use I
was aware of.

> Agreed that it is confusing, and if there is no performance benefit, totally worth removing the format checks on future
> class file versions (so we don’t break code already in the field).

I would expect there would be a significant startup benefit to not
verifying core classes!

But it remains non-obvious to me what kinds of checks are unconditional
and which can be disabled for trusted classes.

Thanks,
David

> thanks,
> Karen
>
>
>
>> On Feb 17, 2017, at 10:37 AM, [hidden email] wrote:
>>
>>
>> I agree.  We need a bug to figure out why there's this _need_verify flag in classFileParser.cpp.   I think it was an optimization for classes on the bootclasspath (formerly rt.jar) because they should never have any errors.  But classFileParser does format checks which I can't see why they'd be costly for performance (should be measured as part of the upcoming bug).   The main effect of this _needs_verify flag here is to confuse people looking at the code.
>>
>> I've reviewed this and it looks like a good fix.  To me it makes no difference if checking for module in verify_class_modifiers() is in _needs_verify or not.
>>
>> Thank you for adding comments in the .cod files in the tests.
>>
>> Thanks,
>> Coleen
>>
>> On 2/17/17 9:20 AM, harold seigel wrote:
>>> To clarify about format checks:
>>>
>>> Conditional checks remain conditional because of backward compatibility concerns.  New checks should be unconditional unless there is an explicit reason for them to be conditional.  With ACC_Module, there is no reason for that check to be unconditional.
>>>
>>> Thanks, Harold
>>>
>>>
>>> On 2/17/2017 7:58 AM, harold seigel wrote:
>>>> Please review this new version of the fix for JDK-8174725:
>>>>
>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>>>
>>>> This fix moves the check for ACC_Module to verify_legal_class_modifiers() as required by David.
>>>>
>>>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>>
>>>> That is outside of the scope of this bug.  Feel free to open a new bug for this issue.  Until then, just use common sense or ask Alex Buckley what to do when adding a format check.
>>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>>>> only checks the modifiers if _need_verify is set.  However, we want to
>>>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>>>> special case to check for ACC_MODULE.
>>>>>
>>>>> That raises the question as to what classfile "format" checks are unconditional and which are only carried out when "verification" is enabled ??
>>>>>
>>>>>> Also, since a classfile version check is already needed in
>>>>>> parse_stream(), when fetching the access_flags from the stream, it was
>>>>>> convenient to just do the check and possible throw right there.
>>>>>> However, if you think it's important, I'll move the check to
>>>>>> verify_legal_class_modifiers().
>>>>>
>>>>> It seems like a good place for it (not withstanding it currently returns immediately) and avoid the need to duplicate the code. It is the duplication I disliked - but "verify_legal_class_modifiers" certainly sounds like the place that would check for ACC_MODULE.
>>>>>
>>>>>> Thanks for proposing a better error message.  Instead of saying 'claims
>>>>>> to be a module', how about:
>>>>>>
>>>>>>    "% is not a class because access_flag ACC_MODULE is set"
>>>>>
>>>>> Okay I guess.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>>>> Hi Harold,
>>>>>>>
>>>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this updated webrev:
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>>>
>>>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>>>
>>>>>>> Also with regards to the error message ... I think it is expressed
>>>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>>>> inability to find the class in the given class representation - I
>>>>>>> suggest:
>>>>>>>
>>>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>>>>> ACC_MODULE set.
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>>>>>>> JVM-9 Spec.
>>>>>>>>>>
>>>>>>>>>> Open Web:
>>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>>>
>>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>>>>>>> NSK tests.
>>>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>>>> have a class name starting with a lower case letter.
>>>>>>>>>
>>>>>>>>> -Alan
>>>>>>>>
>>>>>>
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

David Holmes
In reply to this post by harold seigel
On 17/02/2017 10:58 PM, harold seigel wrote:
> Please review this new version of the fix for JDK-8174725:
>
>     http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>
> This fix moves the check for ACC_Module to
> verify_legal_class_modifiers() as required by David.

Thanks Harold, changes look good to me.

David
-----

>>> That raises the question as to what classfile "format" checks are
> unconditional and which are only carried out when "verification" is
> enabled ??
>
> That is outside of the scope of this bug.  Feel free to open a new bug
> for this issue.  Until then, just use common sense or ask Alex Buckley
> what to do when adding a format check.
>
> Thanks, Harold
>
>
> On 2/15/2017 5:38 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>> Hi David,
>>>
>>> I did not put the check in verify_legal_class_modifiers() because it
>>> only checks the modifiers if _need_verify is set.  However, we want to
>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>> special case to check for ACC_MODULE.
>>
>> That raises the question as to what classfile "format" checks are
>> unconditional and which are only carried out when "verification" is
>> enabled ??
>>
>>> Also, since a classfile version check is already needed in
>>> parse_stream(), when fetching the access_flags from the stream, it was
>>> convenient to just do the check and possible throw right there.
>>> However, if you think it's important, I'll move the check to
>>> verify_legal_class_modifiers().
>>
>> It seems like a good place for it (not withstanding it currently
>> returns immediately) and avoid the need to duplicate the code. It is
>> the duplication I disliked - but "verify_legal_class_modifiers"
>> certainly sounds like the place that would check for ACC_MODULE.
>>
>>> Thanks for proposing a better error message.  Instead of saying 'claims
>>> to be a module', how about:
>>>
>>>     "% is not a class because access_flag ACC_MODULE is set"
>>
>> Okay I guess.
>>
>> Thanks,
>> David
>>
>>> Thanks, Harold
>>>
>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this updated webrev:
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>
>>>> I'm unclear why this logic is needed in two places instead of just
>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>
>>>> Also with regards to the error message ... I think it is expressed
>>>> inappropriately for the NCDFE case. If we were throwing
>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." would be
>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>> inability to find the class in the given class representation - I
>>>> suggest:
>>>>
>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> This webrev has 'return' statements after the calls to fthrow() and a
>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>> ACC_MODULE set.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>>>
>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>
>>>>>>
>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>> exception, for class file versions >= 53, if a class's access_flags
>>>>>>> have ACC_MODULE set.  This behavior will be required in the upcoming
>>>>>>> JVM-9 Spec.
>>>>>>>
>>>>>>> Open Web:
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>
>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>
>>>>>>>
>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT
>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated
>>>>>>> NSK tests.
>>>>>> This looks okay to me although I think I would name the test case
>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>> have a class name starting with a lower case letter.
>>>>>>
>>>>>> -Alan
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

Claes Redestad
In reply to this post by David Holmes
Hi,

On 2017-02-18 07:02, David Holmes wrote:
>
> I would expect there would be a significant startup benefit to not
> verifying core classes!

Is this the exemption for core/trusted classes that one can override
with -Xverify:all?  If so, then yes, it has significant startup benefit
that is easy to measure (30-40ms on a measly Hello World on my 64-bit
Linux machine)

Thanks!

/Claes

>
> But it remains non-obvious to me what kinds of checks are unconditional
> and which can be disabled for trusted classes.
>
> Thanks,
> David
12
Loading...