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

[David Holmes]

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.

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.

David (signing off for the night)
-----

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