RFR(xxs): 8249748: gtest silently ignores bad jvm arguments
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Jul 21 14:44:40 UTC 2020
Thanks Igor! I'll push it then.
On Tue, Jul 21, 2020 at 4:41 PM Igor Ignatyev <igor.ignatyev at oracle.com>
wrote:
> Hi Thomas,
>
> the fix looks good to me. and your reasoning is completely correct, this
> is just an oversight in JEP 281 implementation, nor gtest integration, nor
> Oracle internal infra, nor any other code which depends on that bug. thanks
> for fixing!
>
> Cheers,
> -- Igor
>
>
> > On Jul 21, 2020, at 5:13 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> >
> > 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