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

[Schmidt, Lutz]

Hi Christoph, 

I'm sorry but I strongly disagree. This feature string is solely used as supplemental information in the hs_err* file. At least I couldn't find any other usage. And for that use case, it simply doesn't matter if the string is all lowercase. 

Best,
Lutz


On 31.03.20, 21:23, "Langer, Christoph" <christoph.langer at sap.com> wrote:

    Hi Lutz,
    
    I also agree with Martin's idea of fixing this. I think it's better than changing existing feature strings just for the sake of quiescing a test.
    
    I would even suggest to have a second Array of features in CPUInfo that contains all values in lowercase to be queried by CPUInfo::hasFeature. We should give it a try.
    
    Best regards
    Christoph
    
    > -----Original Message-----
    > From: hotspot-runtime-dev <hotspot-runtime-dev-
    > bounces at openjdk.java.net> On Behalf Of Schmidt, Lutz
    > Sent: Dienstag, 31. März 2020 15:59
    > To: Doerr, Martin <martin.doerr at sap.com>; Thomas Stüfe
    > <thomas.stuefe at gmail.com>
    > Cc: hotspot-runtime-dev at openjdk.java.net
    > Subject: [CAUTION] Re: RFR(S): 8241101: [s390] jtreg test failure after JDK-
    > 8238696: not conformant features string
    > 
    > 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
    >     > >
    >     > >
    >     > >
    >     > >
    >