RFR(xxs): 8249748: gtest silently ignores bad jvm arguments
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Jul 21 12:13:41 UTC 2020
thanks!
I'll wait till tomorrow, if no one has opinions about this I push.
..Thomas
On Tue, Jul 21, 2020 at 2:09 PM David Holmes <david.holmes at oracle.com>
wrote:
> 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