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

[David Holmes]

Thanks for your patience and perseverence on this Coleen.

On 5/10/2012 7:30 AM, Coleen Phillimore wrote:
> 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

Looks okay to me.

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

Yeah there's still a piece missing. :(

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

I will do that and push through the build repos.

Thanks,
David
-----

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