[NEW RFC] Re: RFC: Test set for systemtap instrumentation

[jon.vanalten at redhat.com]

Hi Mark,

Thanks for the great comments.

----- "Mark Wielaard" <mjw at redhat.com> wrote:

> Hi Jon,
> 
> On Fri, 2010-03-05 at 14:29 -0500, jon.vanalten at redhat.com wrote: 
> > As promised, updated patch for review.
> 
> > +2010-03-05  Jon VanAlten  <jon.vanalten at redhat.com>
> > +       * Makefile.am:
> > +       Add target to run tapset tests.
> 
> It would be nice to have the log file in the same directory as the
> other
> check targets. test/check-stap.log
> 

Done.

> The current setup won't work for builddir != srcdir though (see
> below).
> 
> > +       * tapset/hotspot_jni.stp.in:
> > +       Add notes regarding required JVM option to
> Get<PrimitiveType>Field
> > +       family of probe aliases.
> 
> Thanks for these notes. I haven't seen that requirement documented
> anywhere else. We should discuss it also with the hotspot-devs to see
> if
> that is an oversight or intentional.

I should also add I slightly changed/fixed the variable description and probestr printout for variable "nvms" in the hotspot.jni.GetCreatedJavaVMs alias to reflect the fact that nvms is actually a pointer to where the JNI function will return the total number of VMs (not a number passed in to the function).  As in, upon return the caller will by dereferencing this pointer be able to know how much of its buffer of vm handles has been filled, or alternatively whether there are more vm's to which it does not have handles.

> 
> > +       * test/tapset/ClassUnloadedProbeTester.java:
> > +       Part of test coverage for hotspot.stp and hotspot_jni.stp
> tapsets.
> > +       * test/tapset/JNIStapTest.c:
> > +       Likewise.
> > +       * test/tapset/JNITestClass.c:
> > +       Likewise.
> > +       * test/tapset/JNITestClass.h:
> > +       Likewise.
> > +       * test/tapset/JNITestClass.java:
> > +       Likewise.
> > +       * test/tapset/RunWrapper.java:
> > +       Likewise.
> > +       * test/tapset/StapJNIClassLoader.java:
> > +       Likewise.
> > +       * test/tapset/StapURLClassLoader.java:
> > +       Likewise.
> > +       * test/tapset/SystemtapTester.java:
> > +       Likewise.
> > +       * test/tapset/TestingRunner.java:
> > +       Likewise.
> 
> Nice, impressively thorough test setup.
> 

Thanks!

> > +       * test/tapset/jstaptest.pl:
> > +       Wrapper script, compiles and runs tests for tapsets.
> 
> Nice script, but to make it work for all build setups builddir ==
> srcdir
> and/or builddir != srcdir it needs to no hard code the dependency on
> the
> current working directory. Ideally it would have a -S (sourcedir) and
> -O
> (output/builddir) argument (possibly named differently). So you could
> write the make target as:
> 
> tapsetcheck:
>         mkdir -p test/tapset
>         $(abs_top_srcdir)/test/tapset/jstaptest.pl \
>           -S $(abs_top_srcdir)/test/tapset \
>           -O test/tapset \
>           -J $(abs_top_builddir)/$(BUILD_OUTPUT_DIR) \
>           -A $(BUILD_ARCH_DIR) -o test/check-stap.log
> 
> That would run the compilers with arguments based on
> $(abs_top_srcdir)/test/tapset source files, writes out
> objects/classes
> to test/tapset and runs the java runtime either inside the output dir
> or
> with the classpath set to that directory.

Good catch.  I did basically what you suggest, but I left out the -O option.  Presumably, since the script cleans up all files it creates (except for the logfile), it can go ahead and create them in the current working directory.

In addition, this update includes meaningful format string arguments for the stap scripts and corresponding regex strings for each probe, so that variables that are predictable (mainly anything but pointers to memory) are now checked.

Any other concerns?

jon

> 
> Cheers,
> 
> Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: staptest.patch
Type: text/x-patch
Size: 175308 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20100324/a91f27d6/staptest.patch