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

[Coleen Phillimore]

I have modified this change in two ways.  Rather than adding setting 
OPENJDK to openjdk_distro, I am setting it after openjdk_distro is 
included if OPENJDK isn't set, which I was the change that David and I 
discussed offline.  It is redundant but not circular and is safe if 
OPENJDK is passed down.    I will file a bug against the JDK build to do 
this in the future, but many of us build only the hotspot repository.

The other change was that dtrace.hpp didn't build on windows.  The linux 
definitions expand to null for windows.

open webrev at http://cr.openjdk.java.net/~coleenp/7170638_5/
bug link at http://bugs.sun.com/view_bug.do?bug_id=7170638_5

A couple of comments below.

On 10/4/2012 10:00 AM, David Holmes wrote:
> For the record, the suggested fix to defs.make doesn't presently work 
> because defs.make isn't seen by all makefiles used by the various 
> sub-makes. But that will change with the changes coming as part of the 
> minimal VM work.

Even if this test is included in defs.make and OPENJDK is set, the 
dtrace.make makefile won't get this definition.  It only gets flags in 
flags.make created by buildtree.make.  So I didn't see any reason to 
move this to defs.make.   I had a look at the latest minimal vm changes 
and this doesn't affect that.
>
> Coleen's fix seemed to have a circular dependency, but thinking more 
> it is only partially circular - if that makes sense :) The default 
> path for altsrc depends on whether OPENJDK is true or not, but the 
> actual existence check will still work. So if OPENJDK is not set we 
> will look for src/closed, and if that exists then we are not an 
> OPENJDK build - else if it does not exist we can simply set 
> OPENJDK=true (no need to do it in the distro file - as Coleen has 
> pointed out offline). Had OPENJDK been set to true initially we would 
> have looked for a different altsrc but it still would not exist so 
> we'd execute the same logic that would (redundantly) set OPENJDK to 
> true again.
>
> We will need to fix the top-level build regardless, so that it passes 
> down OPENJDK=true.

Yes, I will file a bug (actually if someone else can do that before me, 
that would be great because I don't know what subcategory to file it 
in).   And provide Mark's suggested fix in the bug.

>
> David (signing off for the night)

Thanks David.  It's almost tomorrow for you now.

Coleen
> -----
>
> On 4/10/2012 11:29 PM, David Holmes wrote:
>> On 4/10/2012 9:43 PM, Coleen Phillimore wrote:
>>> On 10/3/2012 7:01 PM, David Holmes wrote:
>>>> Coleen,
>>>>
>>>> On 4/10/2012 7:52 AM, Coleen Phillimore wrote:
>>>>>
>>>>> The logic for setting up OPENJDK is different but there was some
>>>>> code to
>>>>> set up HOTSPOT_VM_DISTRO based on the presence or absence of certain
>>>>> closed files. Anyway, I think this code is correct now. It sets 
>>>>> OPENJDK
>>>>> if HOTSPOT_VM_DISTRO is OpenJDK. I can build Hotspot on Linux both 
>>>>> ways
>>>>> now.
>>>>
>>>> Sorry but that is the wrong fix. If anything OPENJDK should be
>>>> controlling which distro file gets included. The logic for that in
>>>> buildtree.make is:
>>>>
>>>> ifndef HOTSPOT_VM_DISTRO
>>>> ifeq ($(call if-has-altsrc,$(HS_COMMON_SRC)/,true,false),true)
>>>> include $(GAMMADIR)/make/hotspot_distro
>>>> else
>>>> include $(GAMMADIR)/make/openjdk_distro
>>>> endif
>>>> endif
>>>>
>>>> So simply overriding HOTSPOT_VM_DISTRO would lose this OPENJDK=true
>>>> setting - and that file is the wrong place to set that variable.
>>>
>>> Actually, if you set HOTSPOT_VM_DISTRO, that implies that you want some
>>> other distro regardless of whether the Oracle closed repositories 
>>> exist.
>>> So OPENJDK should be false in this case.
>>
>> HOTSPOT_VM_DISTRO just gives a name to the distro. It can still be
>> OpenJDK, or more to the point it will fail to build if not treated as
>> OPENJDK when built externally.
>>
>> Further the logic that chooses to include hotspot_distro or
>> openjdk_distro uses the altsrc macros, but they are in turn defined
>> depending on whether OPENJDK is true or not. So there seems to be a
>> circular dependency there.
>>
>>> i still think this change does exactly what we want. I tried the
>>> suggested change you had in defs.make and it was pretty horrible. This
>>> checks this same thing more cleanly.
>>
>> I've sent a more detailed suggestion offline. This whole situation is a
>> complete mess (and nothing directly to do with this specific bug fix).
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks,
>>> Coleen
>>>>
>>>> To remedy this in hotspot, for a hotspot-only build, we need to check
>>>> for something that indicates this is not an OPENJDK build. The
>>>> presence of the closed repos is generally what is used for that. But
>>>> placing such a check correctly is extremely awkward (something
>>>> compounded by the fact that present hotspot make/defs.make has a
>>>> couple of bugs in ordering that the minimal VM changes will fix). So
>>>> right now I can't tell you exactly how to fix this in a more suitable
>>>> way.
>>>>
>>>> David
>>>> -----
>>>>
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/7170638_4/
>>>>> bug link at http://bugs.sun.com/view_bug.do?bug_id=7170638
>>>>>
>>>>> I also filed a bug 8000408 for Oracle JVM to support this in the
>>>>> future.
>>>>>
>>>>> Please review, again, and thank you for your patience!
>>>>>
>>>>> Coleen
>>>>>
>>>>> On 10/3/2012 10:38 AM, Mark Wielaard wrote:
>>>>>> On Wed, 2012-10-03 at 23:00 +1000, David Holmes wrote:
>>>>>>> To be clear, when hotspot is built as part of a full build then it
>>>>>>> should be being passed the OPENJDK variable. But I don't think 
>>>>>>> it is,
>>>>>>> which is a bug in the top-level make logic. It has been a harmless
>>>>>>> bug
>>>>>>> to-date
>>>>>> Yeah, it looks like it might have been the intention to pass it down
>>>>>> through COMMON_BUILD_ARGUMENTS?
>>>>>>
>>>>>>> To allow for the old build system, and for standalone builds within
>>>>>>> that
>>>>>>> system, I think the hotspot/make/defs.make would have to duplicate
>>>>>>> the
>>>>>>> logic that is in jdk/make/common/Defs.gmk.
>>>>>> I looked at that, but the build makefile setups are pretty 
>>>>>> different.
>>>>>> The simplest seems to be to at least add it to the
>>>>>> COMMON_BUILD_ARGUMENTS at the top-level like so:
>>>>>>
>>>>>> --- openjdk/make/Defs-internal.gmk.orig 2012-10-03 
>>>>>> 16:08:00.239767294
>>>>>> +0200
>>>>>> +++ openjdk/make/Defs-internal.gmk 2012-10-03 16:07:50.508629201 
>>>>>> +0200
>>>>>> @@ -325,6 +325,10 @@
>>>>>> PREVIOUS_MICRO_VERSION=$(PREVIOUS_MICRO_VERSION) \
>>>>>> STATIC_CXX=$(STATIC_CXX)
>>>>>>
>>>>>> +ifdef OPENJDK
>>>>>> + COMMON_BUILD_ARGUMENTS += OPENJDK=$(OPENJDK)
>>>>>> +endif
>>>>>> +
>>>>>> ifdef ARCH_DATA_MODEL
>>>>>> COMMON_BUILD_ARGUMENTS += ARCH_DATA_MODEL=$(ARCH_DATA_MODEL)
>>>>>> endif
>>>>>>
>>>>>> That works for my setup and should cover most default builds of
>>>>>> openjdk
>>>>>> starting from the top-level. Does that look OK to you?
>>>>>>
>>>>>> Next we have to figure out how to get the default for OPENJDK set to
>>>>>> true in a plain hotspot dir. But the logic seems to be somewhat
>>>>>> different from what the jdk dir does. We might be able to use the
>>>>>> openjdk/hostpot/make/openjdk_distro Makefile snippet. Since that is
>>>>>> executed when the the hotspot build detects that it doesn't have
>>>>>> "closed" components. But I am not entirely sure how to satisfy the
>>>>>> restriction listed there:
>>>>>> # This file format must remain compatible with both
>>>>>> # GNU Makefile and Microsoft nmake formats.
>>>>>> Since I don't know anything about nmake.
>>>>>>
>>>>>> Cheers,
>>>>>>
>>>>>> Mark
>>>