[code-reflection] RFR: Model verifier

[Adam Sotona]

On Thu, 10 Oct 2024 18:52:53 GMT, Paul Sandoz <psandoz at openjdk.org> wrote:

>> This patch starts work on model `Verifier` and implements following verifications:
>>  - operands declaration dominance
>>  - `BranchOp` reference arguments matching target block parameters (simple matching by `TypeKind` with erased `int` sub-types)
>>  - `ArithmeticOperation`, `TestOperation` and `ConvOp` verified presence of relevant method handler in `InvokableLeafOps`
>>  
>> `TestSmallCorpus` is improved to verify code model.
>> 
>> Fixes of bugs newly discovered by the `TestSmallCorpus`:
>>  - missing methods in `InvokableLeafOps`
>>  - `Interpreter` use of provided lookup for `resolveToMethodType`
>>  - `Interpreter` erase  sub-`int` types for `InvokeOp` execution + added `TestLiftCustomBytecode::testEraseInts`
>> -  Removed complex sub-`int` types calculation from `BytecodeLift` and `LocalsToVarMapper` 
>> - `BytecodeLift` fixed to avoid production of some obsolete block parameters
>
> src/java.base/share/classes/java/lang/reflect/code/interpreter/Interpreter.java line 424:
> 
>> 422:             MethodHandle mh = resolveToMethodHandle(il, co.invokeDescriptor(), co.invokeKind());
>> 423:             Object[] values = o.operands().stream().map(oc::getValue).toArray();
>> 424:             target = eraseInts(mh.type(), target, values);
> 
> Can you provide an example of why this is required? I think i can guess based on changes to the lifter. Can this impact other areas too like field and array access?
> 
> I think in general we have to be careful here. The properties of lifted models are having a broader impact. Arguably, this could be a code model transformation instead.
> 
> Terminology-wise "erase" is reserved for references. IIUC correctly we are operating on values of basic primitive types and i believe converting to them non-basic types based on corresponding method parameter types.

Example is provided by the test: https://github.com/openjdk/babylon/blob/104ef7dda29d8f580b6f3f330436365764d0b3c7/test/jdk/java/lang/reflect/code/bytecode/TestLiftCustomBytecode.java#L74

Yes, it can impact also other areas and more similar Interpreter corrections might be necessary.

I'm aware of the impact of "erasing" int sub-types. We have to make a decision if all int sub-types are freely assignable (according to JVMS) or their assignability is strictly specified (as in JLS). This PR is a step toward the first option, where Interpreter should not fail on an attempt to call a method with short parameter when passing a boolean as an argument.

A transformation inserting explicit type conversions is the option if we decide to go the assignability-strict way. 

Exactly the same issue is with objects assignability. Should we allow to pass an argument declared as of  j.l.Object type to a method with other specific type parameter or should we inject explicit casts?

> src/java.base/share/classes/java/lang/reflect/code/interpreter/Verifier.java line 41:
> 
>> 39: 
>> 40:     @SuppressWarnings("serial")
>> 41:     public final class VerifyError extends Error {
> 
> I would avoid extending `Error` as it signals some serious abnormal condition when thrown, and these are not intended to be thrown.

I'll fix it, thanks.

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

PR Review Comment: https://git.openjdk.org/babylon/pull/247#discussion_r1796519661
PR Review Comment: https://git.openjdk.org/babylon/pull/247#discussion_r1796522931