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

[David Holmes]

On 3/10/2012 7:33 PM, Mark Wielaard wrote:
> 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.

If OPENJDK is set it should only ever be set to true. The 
jdk/make/common/Defs.gmk file (as used by the top-level build) will set 
it to true if the closed sources are not found:

# If CLOSE_SRC_INCLUDED isn't set to true, check if there's any
# closed directory.
ifneq ($(CLOSED_SRC_INCLUDED), true)
   CLOSED_SRC_INCLUDED := $(shell \
     if [ -d $(CLOSED_SRC) ] ; then \
       echo true; \
     else \
       echo false; \
     fi)
endif

# Set OPENJDK based on CLOSED_SRC_INCLUDED
ifeq ($(CLOSED_SRC_INCLUDED), false)
   OPENJDK = true
endif

Hotspot will default to a non-OPENJDK build unless OPENJDK has been set 
to true, but that only potentially changes the set of sources that will 
be built - if you don't have src/closed or make/closed repos then there 
is no difference between the two builds. Unfortunately Coleen's change 
in dtrace.make requires that OPENJDK be explicitly set to enable the SDT 
probes.

David
-----


>> 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