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

[Coleen Phillimore]

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.

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.

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