RFR: 8341094: Clean up relax_verify in ClassFileParser [v2]

[David Holmes]

On Thu, 7 Nov 2024 23:03:26 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Removed ClassFileParser::_relax_verify and cleaned up setting need_verify in the ClassFileStream code and ClassFileParser.  Wrote up how I found control to flow to this setting with -Xverify:all/none/remote settings, which become BytecodeVerificationRemote/Local in the CR comments.
>> 
>> Tested with tier1-4.  Valid class, field and method name testing is done through the JCK and there are CDS tests that test verification in runtime/cds/appcds/VerifierTest.java.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Rename ClassFileStream::_need_verify to _check_truncation and set based on one call in ClassFileParser.

I won't pretend that I could follow your analysis in the full. At the end of the day we can relax verification (of all kinds) for trusted classes in the released product, but we should have a way to enforce all verification checks on trusted classes during development in case we introduce a bug and start producing invalid class files. I'm not sure that this latter part is actually in place unless "verify all" is explicitly enabled. Of course we will discover bad trusted class files one way or another, but enabling verification would make it more obvious as to the problem.

Doing additional checks on trusted classes is okay provided it doesn't affect startup or general performance - if verification (and related checks) were free we'd always do them, so there has to be a tipping point.

Thanks

src/hotspot/share/classfile/classFileStream.hpp line 55:

> 53: 
> 54:  public:
> 55:   constexpr static bool verify = true;

Where, and how, is this used?

src/hotspot/share/classfile/verifier.hpp line 54:

> 52:   // Return false if the class is loaded by the bootstrap loader,
> 53:   // or if defineClass was called requesting skipping verification
> 54:   // -Xverify:all overrides this value

This comment doesn't describe the new operation of this method.

-------------

PR Review: https://git.openjdk.org/jdk/pull/21954#pullrequestreview-2422669145
PR Review Comment: https://git.openjdk.org/jdk/pull/21954#discussion_r1833688487
PR Review Comment: https://git.openjdk.org/jdk/pull/21954#discussion_r1833687873