RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not conformant features string
Schmidt, Lutz
lutz.schmidt at sap.com
Wed Apr 1 09:21:42 UTC 2020
Hi Christoph,
thank you for the review!
@Martin Do you still have concerns/objections?
Thanks,
Lutz
On 01.04.20, 08:18, "Langer, Christoph" <christoph.langer at sap.com> wrote:
Hi Lutz,
ok, after having another, closer, look to the features string and its usage in hotspot, I can confirm that it is only used in hs_err files and by the whitebox API. Furthermore, I see that features happen to be lowercase on all platforms. So, 390 was a bit out of order and it probably makes sense to follow the other platforms. That means your fix is good ��
Actually, I started to think that one should create a jtreg test to check whether all features are lowercase, given that they are queried with features.contains(feature.toLowerCase()). But I figured that that's what CPUInfoTest implicitly does ��
Best regards
Christoph
> -----Original Message-----
> From: Schmidt, Lutz <lutz.schmidt at sap.com>
> Sent: Dienstag, 31. März 2020 22:14
> To: Langer, Christoph <christoph.langer at sap.com>; Doerr, Martin
> <martin.doerr at sap.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not
> conformant features string
>
> 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
> > > >
> > > >
> > > >
> > > >
> >
>
>
More information about the hotspot-runtime-dev
mailing list