RFR 8219579: Remove redundant signature parsing from the verifier

[Harold Seigel]

Hi Lois,

Thanks for the review!

Please see comments inline.

On 2/28/2019 5:16 PM, Lois Foltan wrote:
>
> On 2/28/2019 3:57 PM, Harold Seigel wrote:
>> Hi,
>>
>> Please review this fix to remove redundant field and method signature 
>> parsing from the verifier.  The parsing is redundant because field 
>> and method signatures are already checked by ClassFileParser.
>>
>> Note that current behavior, in the odd case where a user enables 
>> local verification but disables remote verification, is that both 
>> local and remote classes get verified.  The change in arguments.cpp 
>> does not change that.
>>
>> Open Webrev: 
>> http://cr.openjdk.java.net/~hseigel/bug_8219579/webrev/index.html
>>
>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8219579
>>
>> The fix was regression tested by running Mach5 tiers 1 and 2 tests 
>> and builds on Linux-x64, Solaris, Windows, and Mac OS X, running 
>> tiers 3-5 tests on Linux-x64, and by running JCK-13 Lang and VM tests 
>> on Linux-x64.
>>
>> Thanks, Harold
>>
>
> Hi Harold,
>
> Looks good.  A couple of comments:
>
> - is there a way to remove the SignatureVerifier data structure since 
> it is now only used in an ASSERT situation by verifier.cpp? Could you 
> use ClassFileParser::verify_legal_method_signature and 
> ClassFileParser::verify_legal_field_signature within 
> classfile/verifier.cpp instead?
I plan to do this in a subsequent cleanup enhancement that would include 
the above and also reconciling ClassFileParser's _need_verify and 
_relax_verify flags, and adding some consistency to what is a trusted 
class loader.  For example, Verifier::should_verify_for() thinks it's 
the boot loader. ClassFileParser::relax_format_check_for() thinks it's 
the boot and platform loaders.

>
> - classfile/verifier.cpp
>   line #2114 - can you change the comment in the assert back to 
> something specific to condy like "Invalid type for dynamic constant"?
>   line #2241 & 2717 - nit the other asserts use "Invalid" instead of 
> "Bad".  My suggestion is to make them all consistently use "Invalid"

Done.

Thanks, Harold

>
> Thanks, I don't need to see another webrev.
> Lois