RFR(S): 8241101: [s390] jtreg test failure after JDK-8238696: not conformant features string
Schmidt, Lutz
lutz.schmidt at sap.com
Wed Apr 1 12:49:56 UTC 2020
Hi Martin,
thanks for admitting the fix to be pushed.
As I stated initially, I'm in favor of fixing root causes. In this particular case, however, I prefer to cure the symptoms only. I just don't have the time at the moment to dig deeper, understand all other tests calling CPUInfo.hasFeature(), and iterate until all tests complete OK. I will keep this in mind, though.
Thanks,
Lutz
On 01.04.20, 12:04, "Doerr, Martin" <martin.doerr at sap.com> wrote:
Hi Lutz,
I'm ok with making the features lower case (even though it is not my preferred solution).
But this test code deserves improvement! Maybe other people will run into the same issue and waste a lot of time figuring out why capital letters in feature names don't work. (Not necessarily part of your fix, but if you don't fix it who will?)
Best regards,
Martin
> -----Original Message-----
> From: Schmidt, Lutz <lutz.schmidt at sap.com>
> Sent: Mittwoch, 1. April 2020 11:22
> 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,
> 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