RFR: 8352869: Verify.checkEQ: extension for NaN, VectorAPI and arbitrary Objects [v7]

[Christian Hagedorn]

On Thu, 3 Apr 2025 06:10:14 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 254:
>> 
>>> 252:     private void checkEQimpl(float a, float b, String field, Object aParent, Object bParent) {
>>> 253:         if (isFloatEQ(a, b)) {
>>> 254:             System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits);
>> 
>> Just noticed this now (there are other places as well): Since we now have `Verify.checkEQ()` and `Verify.checkEQWithRawBits()`, it would improve the readability if we reported which method was used. It could be done with something like that (pseudo code):
>> 
>> System.err.println("ERROR: Verify.checkEQ" + withRawBitsString() + " failed: value mismatch.
>> 
>> String withRawBitsString() {
>>     return isFloatCheckWithRawBits ? "WithRawBits" : "";
>> }
>
> Boah. That is really going to bloat the code, don't you think?
> The exception that is thrown will already give you the complete stack trace, including which methods were called. Is that not good enough?

Hm, it could indeed be a little bit more complicated when you are deep down in a recursion. My thought was that it could be misleading when a test is using a mix of `verifyEQ()` and `verifyEQWithRawBits()` and you only read `verifyEQ` failed. You could be start looking at the wrong check even though the stack trace would have guided you to the correct place. Maybe we can just update "Verify.checkEQ" into something more generic like "Equality matching failed" and we're good. What do you think?

>> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 256:
>> 
>>> 254:             System.err.println("ERROR: Verify.checkEQ failed: value mismatch. check raw: " + isFloatCheckWithRawBits);
>>> 255:             System.err.println("  Values: " + a + " vs " + b);
>>> 256:             System.err.println("  Raw:    " + Float.floatToRawIntBits(a) + " vs " + Float.floatToRawIntBits(b));
>> 
>> Do we always want to dump the raw bits even when `isFloatCheckWithRawBits` is false? I guess it does not hurt.
>
> Yes, I want that. It can help if there are different `NaN` encodings. Or if we somehow reinterpreted integer values as floats. It's been useful for me in the past :)

Sounds good, let's leave it in then!

>> test/hotspot/jtreg/compiler/lib/verify/Verify.java line 463:
>> 
>>> 461:     private void print(Object a, Object b, String field, Object aParent, Object bParent) {
>>> 462:         System.err.println("  aParent: " + aParent);
>>> 463:         System.err.println("  bParent: " + bParent);
>> 
>> Should we print `null` parents or just skip them?
>
> I think it does not hurt to print `null` here. It makes the code a little simpler.

Okay, maybe we can print `<none>` in case of a null for more clarity?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026382031
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026386633
PR Review Comment: https://git.openjdk.org/jdk/pull/24224#discussion_r2026402954