RFR(xxs): 8249748: gtest silently ignores bad jvm arguments
David Holmes
david.holmes at oracle.com
Tue Jul 21 07:27:46 UTC 2020
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.
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.
Sorry to belabour this.
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>> 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>>> 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