RFR(xxs): 8249748: gtest silently ignores bad jvm arguments

Tue Jul 21 06:16:06 UTC 2020

Hi David,

On Tue, Jul 21, 2020 at 12:06 AM David Holmes <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>> 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
> >      >
> >
>