RFR (S): 8006965: test_gamma should run with import JDK

David Holmes david.holmes at oracle.com
Wed Mar 20 17:15:52 PDT 2013 

On 21/03/2013 9:47 AM, Christian Thalinger wrote:
>
> On Mar 20, 2013, at 3:12 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> I tested it with my workflow and it produces a lot more output and I did not specify verbose output. It is caused by next change in make/solaris/makefiles/buildtree.make:
>>
>>   include $(GAMMADIR)/make/scm.make
>> + include $(GAMMADIR)/make/defs.make
>>   include $(GAMMADIR)/make/altsrc.make
>>
>> Why do you need it there?
>
> Because of JDK_IMPORT_PATH which is now used in buildtree.make.  I'm actually surprised that nothing else is required from defs.make.

We have a complex three tier make system in hotspot. defs.make is 
supposed to set everything for the top-level make, and update MAKE_ARGS 
for the submakes (ie buildtree.make invocation); buildtree then outputs 
hard-wired values into the generated makefiles.

In recent time defs.make got added to buildtree.make on linux/bsd. That 
was not seen to be harmful, but was probably not the right thing to do. 
The downside is that things in defs.make get executed multiple times - 
hence the noisy debuginfo stuff. It also means you need to be more 
careful about what does go into defs.make (values might change depending 
on which file has included it).

JDK_IMPORT_PATH might work okay if just added to MAKE_ARGS in defs.make 
rather than having to add the include of defs.make in buildtree.make.

David
-----

> -- Chris
>
>>
>> Vladimir
>>
>> On 3/20/13 12:14 PM, Christian Thalinger wrote:
>>>
>>> On Mar 19, 2013, at 7:16 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> Add new test_* targets to default targets to execute a test when you do 'make all_fastdebug' for example.
>>>
>>> Yesterday we thought that's a good idea but it's a problem for cross-compilations (e.g. JPRT uses all_* targets).  So I'd rather not add test_* targets to all_* targets.
>>>
>>>>
>>>> The next looks strange:
>>>>
>>>> + test_fastdebug: generic_test
>>>> + test_fastdebug: VM_FLAVOR=fastdebug
>>>>
>>>> May be it is time to let Queens go and use -help as you suggested
>>>
>>> Some data on using -help instead of Queens:
>>>
>>> - C2 compiles one method when running Queens and two methods for -help
>>> - C1 compiles around 44 for Queens and around 20 for -help
>>> - tiered compiles 73 for Queens and 33 for -help
>>>
>>> So it's about the same smoke test for C2 but half for C1.  Which is think is enough.
>>>
>>> None of both do any GC testing.
>>>
>>>>   or 'cd test; make VM_FLAVOR=$@ servertest' (as JPRT does).
>>>
>>> No, that doesn't work because you need to install the libjvm into a JDK for running that test.
>>>
>>> Here is a new webrev:
>>>
>>> http://cr.openjdk.java.net/~twisti/8006965/
>>>
>>> (PS: I don't know why udiffs don't work in this webrev.)
>>>
>>> -- Chris
>>>
>>>> Vladimir
>>>>
>>>> On 3/19/13 6:16 PM, Christian Thalinger wrote:
>>>>>
>>>>> On Mar 13, 2013, at 4:44 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>
>>>>>>
>>>>>> On Feb 26, 2013, at 6:09 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Feb 25, 2013, at 11:24 PM, David Holmes <David.Holmes at oracle.com> wrote:
>>>>>>>
>>>>>>>> On 26/02/2013 4:42 AM, Christian Thalinger wrote:
>>>>>>>>> On Feb 24, 2013, at 2:54 PM, David Holmes <David.Holmes at oracle.com> wrote:
>>>>>>>>>> On 23/02/2013 1:55 PM, Christian Thalinger wrote:
>>>>>>>>>>> I talked to a lot of people about this today.  What we really want is to not run tests when we build.  Mikael and I were looking into how we could do that without gamma and there is a way:
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~twisti/8006965/
>>>>>>>>>>>
>>>>>>>>>>> This would be the first of three fixes:
>>>>>>>>>>>
>>>>>>>>>>> Fix 1)  The patch above removes test_gamma and uses some weirdness in the VM (-Dsun.java.launcher=gamma) to run it with an existing JDK; add test_{product,fastdebug,debug} targets
>>>>>>>>>>
>>>>>>>>>> This logic is not suitable:
>>>>>>>>>>
>>>>>>>>>> 541 # Testing the built JVM
>>>>>>>>>> 542 ifeq ($(JAVA_HOME),)
>>>>>>>>>> 543   RUN_JVM=JAVA_HOME=$(JDK_IMPORT_PATH) $(JDK_IMPORT_PATH)/bin/java -d$(ARCH_DATA_MODEL) -server -XXaltjvm=$(MISC_DIR)/$(VM_FLAVOR) -Dsun.java.launcher=gamma
>>>>>>>>>> 544 else
>>>>>>>>>> 545   RUN_JVM=$(JAVA_HOME)/bin/java -d$(ARCH_DATA_MODEL) -server -XXaltjvm=$(MISC_DIR)/$(VM_FLAVOR) -Dsun.java.launcher=gamma
>>>>>>>>>> 546 endif
>>>>>>>>>>
>>>>>>>>>> I have JAVA_HOME set in my environment for use by other tools/scripts and it points at JDK7. The existing logic does not use my environments JAVA_HOME setting so neither should the revised logic!
>>>>>>>>>
>>>>>>>>> That's not entirely correct.  test_gamma uses your JAVA_HOME setting:
>>>>>>>>
>>>>>>>> This is so confusing. Our makefiles are an abomination!
>>>>>>>
>>>>>>> I couldn't agree more.
>>>>>>>
>>>>>>>>
>>>>>>>> So this all started because the makefile has:
>>>>>>>>
>>>>>>>> JAVA_HOME=$(ABS_BOOTDIR)
>>>>>>>>
>>>>>>>> which was flagged as wrong because gamma would run in the boot JDK. But now it seems the make variable JAVA_HOME is irrelevant anyway because the test_gamma script will use the JAVA_HOME environment variable.
>>>>>>>>
>>>>>>>> So how did the boot JDK come back into this???
>>>>>>>>
>>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ export JAVA_HOME=/java/re/jdk/8/latest/binaries/linux-x64
>>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ ./test_gamma
>>>>>>>>> Using java runtime at: /java/re/jdk/8/latest/binaries/linux-x64/jre
>>>>>>>>> <snip>
>>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ export JAVA_HOME=/foo
>>>>>>>>> cthaling at sc14a501:/export/twisti/build/graal/build/linux_amd64_compiler2/product$ ./test_gamma
>>>>>>>>> JAVA_HOME must point to a 64-bit OpenJDK.
>>>>>>>>>
>>>>>>>>> And here comes this little snippet into play:
>>>>>>>>>
>>>>>>>>> -MAKE_ARGS += JAVA_HOME=$(ABS_BOOTDIR)
>>>>>>>>> +MAKE_ARGS += BOOTDIR=$(ABS_BOOTDIR)
>>>>>>>>>
>>>>>>>>> Only setting JAVA_HOME to ABS_BOOTDIR make test_gamma work even if you have a JAVA_HOME set.
>>>>>>>>
>>>>>>>> I still don't get this. What has BOOTDIR got to do with JAVA_HOME? Where is this BOOTDIR value being used?
>>>>>>
>>>>>> Ha!  I can remember.  It's used by jmvti.make (implicitly).  The logic in rules.make is:
>>>>>>
>>>>>> ifdef ALT_BOOTDIR
>>>>>>
>>>>>> COMPILE.JAVAC = $(ALT_BOOTDIR)/bin/javac
>>>>>>
>>>>>> else
>>>>>>
>>>>>> ifdef BOOTDIR
>>>>>>
>>>>>> COMPILE.JAVAC = $(BOOTDIR)/bin/javac
>>>>>>
>>>>>> else
>>>>>>
>>>>>> ifdef JAVA_HOME
>>>>>> else
>>>>>>
>>>>>> endif
>>>>>> endif
>>>>>> endif
>>>>>>
>>>>>> BOOTDIR is not defined in rules.make and JAVA_HOME is picked up (which is set to ABS_BOOTDIR).
>>>>>
>>>>> Back to my favorite review these days :-)  I put the BOOTDIR setting back because we need it.
>>>>>
>>>>> Any reviewers who approve or veto pushing this change?
>>>>>
>>>>> -- Chris
>>>>>
>>>>>>
>>>>>> This all sucks and needs to be replaced by something completely different.  Soon.
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>>>> There is no use of it in http://cr.openjdk.java.net/~twisti/8006965/8006965.patch and I don't see it pre-existing ??
>>>>>>>
>>>>>>> I talked to John Coomes about that yesterday and we can remove that line.  ABS_BOOTDIR is only used by Windows.
>>>>>>>
>>>>>>> -- Chris
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> I have added this logic so that users can control what JDK is used when running the test.  In fact they should use ALT_JDK_IMPORT_PATH if they want to control that.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I also don't see why the above sets JAVA_HOME at #543 - what will read that environment variable?
>>>>>>>>>
>>>>>>>>> It's the odd logic in os::jvm_path guarded by Arguments::created_by_gamma_launcher().  A clean-up of that logic would be part of Fix 3.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I still have concerns over what JDK_IMPORT_PATH will point to for different JDK builders.
>>>>>>>>>
>>>>>>>>> It's either JDK_IMPORT_PATH or JDK_IMAGE_DIR.  Since most people don't want to export their libjvm into a JDK we have to use JDK_IMPORT_PATH.  We could add some logic that checks if JDK_IMAGE_DIR exists and use that one.
>>>>>>>>>
>>>>>>>>> -- Chris
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> And this addition still makes no sense to me:
>>>>>>>>>>
>>>>>>>>>> MAKE_ARGS += BOOTDIR=$(ABS_BOOTDIR)
>>>>>>>>>>
>>>>>>>>>> Why do you need to add BOOTDIR to the MAKE_ARGS?
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Fix 2)  Remove gamma and all the ugly code that comes with it (copies of the jdk launcher in hotspot and other pieces); make the hotspot script work like the test targets in Fix 1
>>>>>>>>>>>
>>>>>>>>>>> Fix 3)  Remove the -Dsun.java.launcher=gamma and possibly replace the existing -Dsun.boot.library.path weirdness by explicit command line options like -Xbootlibrarypath:{/p,/a}
>>>>>>>>>>>
>>>>>>>>>>> -- Chris
>>>>>>>>>>>
>>>>>>>>>>> On Feb 22, 2013, at 3:21 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Feb 22, 2013, at 12:58 AM, Staffan Larsen <staffan.larsen at oracle.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> I'm not sure what the correct solution is, but when you do find out, the jdkpath.sh target should also be updated.
>>>>>>>>>>>>
>>>>>>>>>>>> How many are actually using the hotspot script?  Would people be very sentimental if we would remove the gamma launcher altogether?
>>>>>>>>>>>>
>>>>>>>>>>>> Taking to people here it seems that most are copying their libjvm into a JDK and use java anyway.
>>>>>>>>>>>>
>>>>>>>>>>>> -- Chris
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> /Staffan
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 22 feb 2013, at 03:40, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~twisti/8006965
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 8006965: test_gamma should run with import JDK
>>>>>>>>>>>>>> Reviewed-by:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Right now test_gamma runs with the boot JDK which is JDK n-1 (where
>>>>>>>>>>>>>> JDK n is the version we are actually compiling for).  This setup is
>>>>>>>>>>>>>> unsupported and thus should not be done during HotSpot builds.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The fix is to always use JDK_IMPORT_PATH instead of JAVA_HOME when
>>>>>>>>>>>>>> running test_gamma.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> make/bsd/makefiles/buildtree.make
>>>>>>>>>>>>>> make/defs.make
>>>>>>>>>>>>>> make/linux/makefiles/buildtree.make
>>>>>>>>>>>>>> make/solaris/makefiles/buildtree.make
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>

More information about the hotspot-dev mailing list