Request for review 7170638: Use DTRACE_PROBE[N] in JNI Set and SetStatic Field.

[Mark Wielaard]

On Tue, 2012-10-02 at 21:32 -0400, Coleen Phillmore wrote:
> I have changed the dtrace.make makefile a bit.   The JVM group at Oracle 
> hasn't added support for SystemTap so I wanted to make sure that Oracle 
> doesn't support this in the binaries and mistakenly enable this if we 
> upgrade our build compilers or systems.    The dtrace probe support is 
> now also guarded by an ifdef OPENJDK.  Do you build with OPENJDK=true 
> when you build just hotspot binaries?  The JDK sets this variable in the 
> Makefiles.

No I don't use OPENJDK=true, just the normal build targets. As far as I
can see IcedTea also doesn't set it or uses any other target than the
default (or hotspot or jdk_only in case of alternative VMs).

Minor nit (please feel free to ignore). The reason text is a little
misleading, it should say something like:
REASON = "This JDK does not support SDT probes yet"
Whether you then use them with Systemtap (or GDB or some other tool that
can use them) seems irrelevant. (aka, it isn't hotspot supporting
Systemtap, it is Systemtap supporting DTrace compatible SDT probes).

> Please review and try the patch in this webrev.
> 
> http://cr.openjdk.java.net/~coleenp/7170638_3/ 
> <http://cr.openjdk.java.net/%7Ecoleenp/7170638_3/>

It doesn't seem to work for me. The conditional ifndef OPENJDK seems to
always evaluate to true in my setup.

Can you explain the OPENJDK=true setting a bit more? I don't see it
mentioned at:
http://hg.openjdk.java.net/jdk7/build/raw-file/tip/README-builds.html#variables

I do see a Build Directives: OPENJDK = true in my build logs, but that
is not passed down to any subdir makes. Maybe it should be part of
COMMON_BUILD_ARGUMENTS in make/Defs-internal.gmk? Or maybe the hotspot
subdir make should call the sanity target first like the jdk subdir
does?

It does work for me if I change the conditional to
ifeq ($(OPENJDK),false). But I don't know if that is what you want.

> Also is it possible for the test to do more than just test that the 
> probes are enabled?  Or does it require some external tools?

That would require external tools (in this case I even was careful to
not use any newer feature of readelf, since only newer versions know how
to parse the ELF note section to parse out the probes). Given what a
nightmare it has been to just get a simple build fix in, I rather not
also try to push any new runtime features or new external tool
requirements :)

Thanks,

Mark