RFR(xxs): 8249748: gtest silently ignores bad jvm arguments
David Holmes
david.holmes at oracle.com
Tue Jul 21 12:09:37 UTC 2020
On 21/07/2020 6:35 pm, Thomas Stüfe wrote:
>
>
> On Tue, Jul 21, 2020 at 9:32 AM David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> Hi Thomas,
>
> I'm running this through our tier 1-3 testing to see if it exposes any
> issue. If not then lets proceed unless someone else chimes in. I'll
> also
> flag this RFR internally.
>
>
> Thank you David!
Tests all clear.
Cheers,
David
>
> To be clear my concern was that under the current uninitialized code it
> would more often act as
>
> args.ignoreUnrecognized = JNI_TRUE;
>
> than
>
> args.ignoreUnrecognized = JNI_FALSE;
>
> and so your change may perturb existing testing regimes.
>
>
> I understand.
>
> Sorry to belabour this.
>
>
> No problem at all, that is why we do Reviews.
> Thanks Thomas
>
> Thanks,
> David
> -----
>
> On 21/07/2020 4:16 pm, Thomas Stüfe wrote:
> > Hi David,
> >
> > On Tue, Jul 21, 2020 at 12:06 AM David Holmes
> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>> wrote:
> >
> > Hi Thomas,
> >
> > On 21/07/2020 12:17 am, Thomas Stüfe wrote:
> > > Hi David,
> > >
> > >
> > > On Mon, Jul 20, 2020 at 2:54 PM David Holmes
> > <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
> <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
> > > <mailto:david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>
> > <mailto:david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>>> wrote:
> > >
> > > Hi Thomas,
> > >
> > > On 20/07/2020 8:59 pm, Thomas Stüfe wrote:
> > > > Hi,
> > > >
> > > > could I please have reviews for this very trivial
> patch?
> > > >
> > > > I found that gtest ignores invalid jvm options randomly
> > since it
> > > relies on
> > > > an uninitialized variable.
> > >
> > > Is it really random? I would have expected it to be
> basically
> > always
> > > non-zero and hence always ignore unknown options.
> > >
> > >
> > > I remember seeing cmdline option errors from gtest, and
> since this
> > > coding is very old, this must be either my failing memory
> or the
> > random
> > > content of the underlying uninitialized stack memory.
> >
> > Yes but there is only one value of that uninitialized memory
> (zero)
> > that
> > would cause unrecognised options to not be ignored.
> >
> >
> > So you are saying that it was in the past more probable that options
> > were ignored than that they were heeded.
> >
> > I am saying that they were occasionally not ignored. If that were
> bad we
> > would have seen sporadic errors in the past. Especially as this
> is a day
> > zero bug, there since integration of gtests in 2016.
> >
> > > I'm not at all clear
> > > if running of the gtests might rely on always ignoring
> > > unexpected/unknown flags -
> > >
> > >
> > > It does not. googletest fishes its own arguments from the
> initial
> > > argument vector and passes the rest off to the JVM. So the JVM
> > ignoring
> > > or not ignoring arguments will not change anything.
> > >
> > > does it have the capability to distinguish
> > > product and non-product test runs?
> > >
> > >
> > > Gtests are hotspot coding and there are sections
> running with #ifdef
> > > ASSERT, though I am not sure why that would matter?
> >
> > For the same reason we sometimes need to @require that a VM
> is a debug
> > VM with a jtreg test - because the flag passed in is a
> non-product
> > flag.
> > Also when we run test suites we often pass
> > -XX:+IgnoreUnrecognisedVMOptions, again so that non-product
> flags don't
> > cause a failure with release bits.
> >
> >
> > Okay, I understand.
> >
> > >
> > > I think this needs wider review from people familiar
> with how
> > our gtest
> > > tests are run. (I have no idea - I never use it.)
> > >
> > >
> > > I am quite familiar with it since I use it almost daily. I
> really
> > depend
> > > on it being able to interpret JVM options.
> > >
> > > In fact I am a bit dismayed by this bug since I write tons of
> > tests for
> > > JEP387 and was feeling very smug about the tests running
> through
> > in all
> > > my test scenarios, only to find that since days I keep running
> > the same
> > > - default - scenario over and over again :( No, that should be
> > really fixed.
> >
> > I don't understand what you mean. This setting should only
> affect what
> > happens with unrecognised "bad" arguments (as per your subject).
> >
> >
> > It hides errors. Let's say I want to run metaspace gtests with a
> policy
> > different from the standard, I'd specify
> >
> > gtestLauncher -jdk:<jdk> -XX:MetaspaceReclaimStrategy=none
> >
> > but if I mistype the option - which happened to me - the VM
> ignores it
> > silently and the tests run in default settings, all being green.
> Which
> > gave me a false feeling of security until much later I started found
> > during debugging the setting had been ignored.
> > The correct behaviour would have been for gtestLauncher to give
> me an
> > "Unrecognized VM option" error right away.
> >
> > > Gtestlauncher is called as part of the jtreg tests by the
> > GtestWrapper,
> > > but that does not pass any options to it.
> >
> > Pardon my ignorance but how does one specify options for a
> gtest then?
> >
> >
> > You just pass them on the command line. One can intermix them freely
> > with the googletest options and those of the gtestlauncher itself.
> > For example:
> >
> > ./hotspot/variant-server/libjvm/gtest/gtestLauncher -Xlog:metaspace*
> > -jdk:./images/jdk/ --gtest_filter=metaspace.*
> > -XX:MetaspaceReclaimStrategy=none
> >
> > -jdk gets consumed by our gtestLauncher itself
> > -any one of the gtest_... options get eaten by the googletest
> framework
> > -Xlog and the -XX options are the remaining VM options and get
> handed to
> > the VM
> >
> > > But seriously, if there are tests which pass options to
> it, they
> > > probably want those options to do something in the jvm, so
> > ignoring them
> > > silently is not good.
> >
> > Again only "bad" options are ignored - which should only
> impact use of
> > non-product flags with product bits.
> >
> >
> > Same reasoning as above.
> >
> > David
> > -----
> >
> >
> > My reasoning is this:
> >
> > launcher should not ignore unrecognized VM options because whoever
> > specifies them expects them to arrive in the VM and do something. In
> > case this patch shakes loose a scenario where someone specified the
> > wrong option he/she should look at that case. Either it was wrong to
> > specify the option, or more probably it was a mistype like it
> happened
> > to me.
> >
> > As for how could that happen. ATM I see in our sources three
> invocations
> > of gtestLauncher.
> >
> > - from make files when one runs tests via make, make/RunTests.gmk.
> > There, we can specify VM options via command line similar to what
> I do
> > above. Though I am not sure this even works: the associated make
> > variable GTEST_JAVA_OPTIONS is not set anywhere AFAICS. Anyway, this
> > case is to my knowledge only executed manually and so the same
> reasoning
> > applies: whoever does this probably does not want a mistyped VM
> option
> > silently ignored.
> > - from the one jtreg test which runs the gtests. It does not
> specify any
> > VM options.
> > - from a vscode IDE integration script which does not either
> >
> > Of course I do not know what Oracle does internally and whether they
> > rely on bad options being ignored in some scripts somewhere.
> >
> > --
> >
> > So this is a bit of an impasse. Who do you suggest should look at
> this?
> > I am not sure how to proceed.
> >
> > Cheers, Thomas
> >
> > > Thanks, Thomas
> > >
> > >
> > > Thanks,
> > > David
> > >
> > > > issue: https://bugs.openjdk.java.net/browse/JDK-8249748
> > > > webrev:
> > > >
> > >
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8249748-gtest-silently-ignores-bad-jvm-arguments/webrev.00/webrev/
> > > >
> > > > Thanks, Thomas
> > > >
> > >
> >
>
More information about the hotspot-runtime-dev
mailing list