RFR: Value types consistency checks

[John Rose]

Reviewed.  Looks good; ship it!

The class loader constraint checks and value type consistency checks
should be factored together more closely.  I noticed this when I had to
manually check each of the six places where CLC checks are initiated in
linkResolver.cpp and klassVTable.cpp.  Each of those places need to
initiate value type consistency checks also—and they do.  But the code
would hang together better if these two tasks were more aligned, so that
both kinds of consistency checks were handled by one subroutine
call.

I would also put the field and method versions of signature checking
together; they don't differ much and it's trivial to decide what to do
given the signature symbol based on a boolean.  This is what the
common checking logic routine does (SD:: check_signature_loaders),
so pull that pattern up the call chain.

I suggest renaming check_method_loader_constraints to something
like check_linkage_constraints, and then inlining and removing the
new function check_method_value_types_consistency.  I also suggest
refactoring check_field_loader_constraints so that it calls the same
function check_linkage_constraints, with a boolean is_method of
false (true for the other calls) just like SD::csl.  This will pay off
some technical debt relating to loader constraints, as well as folding
in VT checks almost for free.

What I'm asking for is some code cleanup, not a change to this patch.
I'm glad to see this moving forward as-is.

One other topic prompted by this review, and by thinking about
factoring:

The question about whether to constrain arrays is an interesting one.
There are two answers:  Require agreement about value-ness of the
array element, or don't require.  The answers can be given independently
in two places:  When stuffing vtable/itable slots (klassVT), and when
linking method/field calls (linkRes).  In the end I think simpler is better,
so unless we have a strong reason, we should not require value-ness
consistency of array elements.  I seem to remember that there *might*
have been a pretty strong reason suggested at one point, but I don't
remember it (and it's late, in a holiday week).  If in the end we can't
state a very sharp reason why array elements need to be constrained
across classfiles, then we should drop value-type constraints.

In the end, if two classes are sharing access to a single array (via a
non-null reference), that array will be the "source of truth" about value-ness,
and there's probably not much point in having the classes confer ahead
of time about the array.  For non-array values, the early conference is
necessary in order to fix call-by-value calling sequences.  But arrays
are passed by reference, regardless of element type: reference,
primitive, or value.  Thus, agreement about the status of their elements
is less valuable than agreement of the basic shape of each argument
and return value.

If we take the simple answer, after all, or if we decide to check array
elements in *both* places, then we should consider refactoring the
signature scanning and error reporting code in linkRes and klassVT
so that it is in common.  Perhaps it can factor all the way up to the
function InstanceKlass::check_value_type_consistency (which I am
imagining would handle both field and method types).  Or maybe
it can be pushed to SystemDictionary and live with the SD::csl.

— John

On Jul 6, 2018, at 1:26 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
> 
> Karen,
> 
> Thank you for the review.
> 
> Here’s a updated version of the patch:
> http://cr.openjdk.java.net/~fparain/VTAttributeChecks/webrev.02/index.html
> 
> I’ve added support for declarer/overrider checks, fixed a few bugs and
> added asserts to check “L” in array signatures.
> 
> I’ve tried to generate ValueTypes attribute meta-data only when
> EnableValhalla was set to true, unfortunately, it causes some
> failures with verifier tests. I’ll look at his in more details after
> my vacations.
> 
> Testing is tricky, I’m currently using some Java source files
> and a script to test mismatch scenarios, but in their current
> form, they cannot be integrated with jtreg. I’ve attached the
> test files to this mail if you want to use them.
> 
> Regards,
> 
> Fred
> 
> 
> 
> 
>> On Jun 29, 2018, at 16:51, Karen Kinnear <KAREN.KINNEAR at ORACLE.COM> wrote:
>> 
>> Frederic,
>> 
>> Many thanks for doing this and doing this so cleanly.
>> 
>> Couple of questions:
>> 1. instanceKlass.cpp: 
>> if (has_value_types_attribute) line 632 — does this want to also be if EnableValhalla?
>> my understanding is that extra attributes are ignored so classFileParser should just allow them
>> 
>> 2. instanceKlass.cpp check_symbol/signature_for_value_types_consistency
>> In the array part, did you want to check for “L” or is that just me being over cautious?
>> 
>> 3. klassVtable.cpp
>> In update_inherited_vtable when you override a method (this is the vtable part),
>> there is a if (check constraints & !targer_method()->is_overpass() - which needs
>> a declarer/overridder match check. Also in initialize_itable_for_interface under if (check constraints)
>> 
>> 4. Thank you for adding the ValueTypes attributes in the class files for the tests.
>> Did you add any tests that fail consistency?
>> 
>> many thanks,
>> Karen
>> 
>>> On Jun 28, 2018, at 4:38 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
>>> 
>>> Please review this changeset implementing consistency checks based on the
>>> ValueTypes attribute. These checks ensure that the assumptions a class has
>>> about value types, as encoded in its ValueTypes attribute, match the
>>> reality, or the assumptions of another class it links to.
>>> 
>>> The details of the consistency checks have been summarized by Karen in
>>> this document:
>>> http://cr.openjdk.java.net/~acorn/value-types-consistency-checking-details.pdf
>>> 
>>> If the implementation doesn’t match the document, this is likely to be bug,
>>> so please, report it.
>>> 
>>> Some tests using the Bytecodes API have been updated to include a
>>> ValueTypes attribute in the class files they generate (thanks to Srikanth
>>> for adding this feature to the Bytecodes API).
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~fparain/VTAttributeChecks/webrev.01/
>>> 
>>> Thank you,
>>> 
>>> Fred
>>> 
>> 
>