RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not conformant features string

[Schmidt, Lutz]

Hi Martin,
your extended check would heal the issue we saw with the original feature string on s390. So far, so good. I can’t judge if other tests can live with that.
Regards,
Lutz

From: "Doerr, Martin (martin.doerr at sap.com)" <martin.doerr at sap.com>
Date: Tuesday, 31. March 2020 at 15:54
To: Thomas Stüfe <thomas.stuefe at gmail.com>, Lutz Schmidt <lutz.schmidt at sap.com>
Cc: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>, "hotspot-runtime-dev at openjdk.java.net" <hotspot-runtime-dev at openjdk.java.net>
Subject: RE: RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not conformant features string

But what about
features.contains(feature) || features.contains(feature.toLowerCase())
?

Is it still problematic if we just support both, lower case and exact match?

Best regards,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Dienstag, 31. März 2020 15:51
To: Schmidt, Lutz <lutz.schmidt at sap.com>
Cc: Doerr, Martin <martin.doerr at sap.com>; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not conformant features string

Okay. The change looks good to me.

Cheers, Thomas

On Tue, Mar 31, 2020 at 3:38 PM Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>> wrote:
Thomas, Martin,

thank you for looking into this.

Generally, I'm very much in favor of the "fix the root cause" approach. In this particular case, however, I'm afraid of side effects and excessive effort. CPUInfo.hasFeature() is called in quite a few places, see the attached text file for reference.

My proposed change, on the other hand, is very constrained in scope: just s390, just a few string edits. The risk to break anything is extremely low. Therefore, I would like to stick with the proposed change.

Thanks,
Lutz


On 31.03.20, 11:33, "Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:

    Hi Thomas,

    I agree with that.

    test/lib/sun/hotspot/cpuinfo/CPUInfo.java uses
    return features.contains(feature.toLowerCase());
    in "hasFeature" which is not nicely designed.

    If it is required to lower the case (for some reason), I think it should better use
    features.contains(feature) || features.contains(feature.toLowerCase())

    Or even better if we don't need toLowerCase at all and fix the strings.

    Anyway, the 2nd part of the change (SHA, AES, ...) looks good to me.

    Best regards,
    Martin


    > -----Original Message-----
    > From: hotspot-runtime-dev <hotspot-runtime-dev-
    > bounces at openjdk.java.net<mailto:bounces at openjdk.java.net>> On Behalf Of Thomas Stüfe
    > Sent: Dienstag, 31. März 2020 10:01
    > To: Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>>
    > Cc: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>
    > Subject: Re: RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not
    > conformant features string
    >
    > HI Lutz,
    >
    > Should we not better adjust the jtreg test?
    >
    > Cheers, Thomas
    >
    > On Tue, Mar 31, 2020 at 9:22 AM Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>>
    > wrote:
    >
    > > Dear All,
    > >
    > > may I please request reviews for this fix, adjusting the CPU features
    > > string such that jtreg tests, in particular a newly introduced one, pass OK.
    > >
    > > Bug:    https://bugs.openjdk.java.net/browse/JDK-8241101
    > > Webrev: https://cr.openjdk.java.net/~lucy/webrevs/8241101.00/
    > >
    > > Thank you!
    > > Lutz
    > >
    > >
    > >
    > >