On Thu, Jun 26, 2014 at 2:51 PM, Volker Simonis <volker.simo...@gmail.com> wrote: > OK, I've just realized that my comments were a little too late and the > change was submitted shortly after I sent the mail. > > I'll try to put this tiny fix into the patch for 8048169 then which > already contains some other PPC64 related stuff anyway. >
I've decided to better create an own bug for this issue (https://bugs.openjdk.java.net/browse/JDK-8048232) because 8048169 needs to be down-ported to jdk8 while this change doesn't. Regards, Volker > Regards, > Volker > > > On Mon, Jun 23, 2014 at 6:42 PM, Volker Simonis > <volker.simo...@gmail.com> wrote: >> Hi Mikael, >> >> sorry for the delayed answer - the PPC/AIX team was on holiday:) >> >> I've tested your changes on AIX and Linux/PPC64. On AIX everything works >> fine. >> >> For Linux/PPC64 there's a single occurrence of a test against $ARCH >> which you've missed: >> >> diff -r 116e9b1bf477 make/linux/Makefile >> --- a/make/linux/Makefile Mon Jun 23 17:43:30 2014 +0200 >> +++ b/make/linux/Makefile Mon Jun 23 18:15:20 2014 +0200 >> @@ -67,8 +67,10 @@ >> endif >> endif >> # C1 is not ported on ppc64, so we cannot build a tiered VM: >> -ifeq ($(ARCH),ppc64) >> - FORCE_TIERED=0 >> +ifeq ($(ARCH),ppc) >> + ifeq ($(ARCH_DATA_MODEL), 64) >> + FORCE_TIERED=0 >> + endif >> endif >> >> ifdef LP64 >> >> With this change I could successfully build on AIX and Linux/PPC64 >> (I've only tested complete builds). >> >> Thank you and best regards, >> Volker >> >> >> On Tue, Jun 17, 2014 at 9:11 PM, Mikael Vidstedt >> <mikael.vidst...@oracle.com> wrote: >>> >>> New webrev here (only the hotspot part, the webrev for top hasn't changed): >>> >>> http://cr.openjdk.java.net/~mikael/webrevs/8046471/webrev.02/hotspot/webrev/ >>> >>> Comments inline. >>> >>> >>> On 2014-06-16 19:49, David Holmes wrote: >>>> >>>> Hi Mikael, >>>> >>>> Sorry for the delay ... >>>> >>>> make/aix/makefiles/defs.make: >>>> >>>> This change doesn't make sense to me: >>>> >>>> 48 ifneq (,$(findstring $(ARCH), ppc)) >>>> >>>> given that the logic immediately preceding this sets ARCH to either ppc or >>>> ppc64 based on ARCH_DATA_MODEL. You seem to be trying to allow for both >>>> 32-bit and 64-bit cross-builds but the earlier logic is really precluding >>>> this. So it seems to me that the changes in this file are completely >>>> unnecessary (or else the earlier logic also needs changing). >>> >>> >>> Indeed, that is correct - I missed the fact that ARCH is always overriden to >>> be either ppc or ppc64. The old logic is unnecessarily hard to follow, but I >>> guess that's in line with our hotspot Makefiles in general ;) >>> >>> I'll revert the changes to the file and add a mental note to self and others >>> that linux appears to be the only platform where the incoming value of ARCH >>> is actually honored - it's ignored/overridden on Solaris, BSD and AIX. >>> >>> >>>> make/linux/makefiles/defs.make >>>> >>>> This block: >>>> >>>> 86 # i686/i586 and amd64/x86_64 >>>> 87 ifneq (,$(findstring $(ARCH), amd64 x86_64 i686 i586)) >>>> 88 ifeq ($(ARCH_DATA_MODEL), 64) >>>> 89 ARCH_DATA_MODEL = 64 >>>> 90 MAKE_ARGS += LP64=1 >>>> 91 PLATFORM = linux-amd64 >>>> 92 VM_PLATFORM = linux_amd64 >>>> 93 HS_ARCH = x86 >>>> 94 else >>>> 95 ARCH_DATA_MODEL = 32 >>>> 96 PLATFORM = linux-i586 >>>> 97 VM_PLATFORM = linux_i486 >>>> 98 HS_ARCH = x86 >>>> 99 # We have to reset ARCH to i686 since SRCARCH relies on it >>>> 100 ARCH = i686 >>>> 101 endif >>>> 102 endif >>>> >>>> seems to allow the user to try and do a 64-bit build on a 32-bit build >>>> machine. Not sure if that would get caught in an earlier sanity check? >>>> (Same >>>> is true for the sparc block). >>> >>> >>> While the changes are primarily just intended to cut down on the duplication >>> I don't immediately see the reason why we wouldn't want to allow compiling a >>> 64-bit VM from a 32-bit machine? We're cross compiling all sorts of >>> platforms, so why not allow this if the compiler supports it? I'd prefer to >>> keep it this way. >>> >>> >>>> Also I don't think the following is actually true: >>>> >>>> # We have to reset ARCH to i686 since SRCARCH relies on it >>>> ARCH = i686 >>>> >>>> As long as ARCH is not amd64 and not x86_64 any 32-bit x86 architecture >>>> designator will simply map to a SRCARCH of x86, as per defs.make: >>>> >>>> SRCARCH = $(ARCH/$(filter sparc sparc64 ia64 amd64 x86_64 arm ppc >>>> zero,$(ARCH))) >>>> ARCH/ = x86 >>>> ARCH/sparc = sparc >>>> ARCH/sparc64= sparc >>>> ARCH/ia64 = ia64 >>>> ARCH/amd64 = x86 >>>> ARCH/x86_64 = x86 >>>> ARCH/ppc64 = ppc >>>> ARCH/ppc = ppc >>>> ARCH/arm = arm >>>> ARCH/zero = zero >>> >>> >>> Indeed, I fully agree - as long as it's not { sparc, sparc64, ia64, ppc64, >>> ppc, arm, zero } it will map to x86 anyway. I thought about cleaning that up >>> before I sent out the first webrev, but soon found myself doing way more >>> cleanup than was healthy for this specific change. However, since I had to >>> update this change anyway I removed the ARCH reset part. >>> >>> Cheers, >>> Mikael >>> >>> >>>> >>>> >>>> Cheers, >>>> David >>>> >>>> On 17/06/2014 6:17 AM, Mikael Vidstedt wrote: >>>>> >>>>> >>>>> Thanks Erik. Another review please? >>>>> >>>>> Cheers, >>>>> Mikael >>>>> >>>>> On 2014-06-12 23:56, Erik Joelsson wrote: >>>>>> >>>>>> Looks fine to me. >>>>>> >>>>>> /Erik >>>>>> >>>>>> On 2014-06-12 23:18, Mikael Vidstedt wrote: >>>>>>> >>>>>>> >>>>>>> I have now verified that the changes work just fine for the platforms >>>>>>> we build and test - both from the top level and when building hotspot >>>>>>> only. Taking suggestions on other tests to perform. And it would be >>>>>>> great if somebody could test the changes on on aix/ppc. >>>>>>> >>>>>>> So, kindly asking for "real"/final reviews of the changes: >>>>>>> >>>>>>> top: >>>>>>> >>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046471/webrev.01/top/webrev/ >>>>>>> hotspot: >>>>>>> >>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046471/webrev.01/hotspot/webrev/ >>>>>>> >>>>>>> >>>>>>> Cheers, >>>>>>> Mikael >>>>>>> >>>>>>> On 2014-06-10 22:53, Mikael Vidstedt wrote: >>>>>>>> >>>>>>>> >>>>>>>> David, >>>>>>>> >>>>>>>> Thanks for the feedback. Essentially the logic in the >>>>>>>> make/<os>/makefiles/defs.make files needs to recognize and deal with >>>>>>>> two different use cases: >>>>>>>> >>>>>>>> 1. ARCH being set by the JDK build system to the value of >>>>>>>> OPENJDK_TARGET_CPU_ARCH, or >>>>>>>> 2. no ARCH being set, in which case it needs to be populated - >>>>>>>> typically from uname >>>>>>>> >>>>>>>> Since Solaris and bsd both override ARCH they do not care about >>>>>>>> OPENJDK_TARGET_CPU_ARCH and effectively always go through case 2. >>>>>>>> >>>>>>>> Linux/x86 is where most of the difference (and confusion) is, but I >>>>>>>> think aix/ppc64 will also change slightly since the ARCH value will >>>>>>>> go from ppc64 to ppc. I've tried to make the relevant changes, but I >>>>>>>> cannot verify that they actually work. cc:ing the ppc-aix list in >>>>>>>> the hope that somebody can help out with that. >>>>>>>> >>>>>>>> >>>>>>>> Summing it up, I have the following two webrevs: >>>>>>>> >>>>>>>> top: >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046471/webrev.01/top/webrev/ >>>>>>>> >>>>>>>> hotspot: >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046471/webrev.01/hotspot/webrev/ >>>>>>>> >>>>>>>> >>>>>>>> With these changes I can build the normal platforms, but more >>>>>>>> verification is needed - esp. building hotspot only. Meanwhile >>>>>>>> feedback is much appreciated! >>>>>>>> >>>>>>>> Cheers, >>>>>>>> Mikael >>>>>>>> >>>>>>>> On 2014-06-10 19:45, David Holmes wrote: >>>>>>>>> >>>>>>>>> Hi Mikael, >>>>>>>>> >>>>>>>>> This seems a reasonable proposal to me. We have an over-abundance >>>>>>>>> of "arch" variables and values, so reducing that is a good aim. >>>>>>>>> >>>>>>>>> As you note the main flow-on effect here is that the hotspot >>>>>>>>> makefiles have to be updated for the cases where >>>>>>>>> OPENJDK_TARGET_CPU_ARCH and OPENJDK_TARGET_CPU_LEGACY differ so >>>>>>>>> that it still sets LIBARCH, BUILDARCH, SRCARCH correctly. I think >>>>>>>>> only x86 has that issue. >>>>>>>>> >>>>>>>>> Wouldn't it be nice if we could get rid of i386, i586, i686 etc >>>>>>>>> both internally and in the build artifacts and bundles! >>>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> David >>>>>>>>> >>>>>>>>> On 11/06/2014 10:11 AM, Mikael Vidstedt wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> All, >>>>>>>>>> >>>>>>>>>> I need some feedback and comments on the below fix: >>>>>>>>>> >>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8046471 >>>>>>>>>> Webrev: >>>>>>>>>> http://cr.openjdk.java.net/~mikael/webrevs/8046471/webrev.00/webrev/ >>>>>>>>>> >>>>>>>>>> Background: >>>>>>>>>> >>>>>>>>>> When configuring the hotspot build the build system sets up the ARCH >>>>>>>>>> variable to reflect the target cpu. Currently the value is >>>>>>>>>> initialized >>>>>>>>>> to OPENJDK_TARGET_CPU_LEGACY, which is the internal legacy cpu >>>>>>>>>> name. For >>>>>>>>>> example, on x86 64-bit this is amd64 on linux (but x86_64 on mac). >>>>>>>>>> The >>>>>>>>>> goal in the new (JDK) build system is to have the "legacy" value >>>>>>>>>> gradually removed in favor of the other variables. >>>>>>>>>> >>>>>>>>>> Discussion: >>>>>>>>>> >>>>>>>>>> The two candidate variables to base ARCH on are as far as I can tell >>>>>>>>>> OPENJDK_TARGET_CPU and OPENJDK_TARGET_CPU_ARCH. Of the two >>>>>>>>>> OPENJDK_TARGET_CPU_ARCH is the more "stable" one, with a single, >>>>>>>>>> well >>>>>>>>>> defined value per cpu family { arm, ppc, s390, sparc, x86 }. >>>>>>>>>> Together >>>>>>>>>> with ARCH_DATA_MODEL/LP64 that information should be enough for the >>>>>>>>>> Hotspot build system to do its thing. Note: ARCH is currently >>>>>>>>>> ignored on >>>>>>>>>> solaris and bsd - it's overridden at the top of the respective >>>>>>>>>> make/<os>/makefiles/defs.make files. >>>>>>>>>> >>>>>>>>>> Before I go too far with this though I'd like to get some feedback >>>>>>>>>> on >>>>>>>>>> whether or not this is the right approach and what the exact value >>>>>>>>>> should be. Depending on the outcome of that the Hotspot build >>>>>>>>>> system may >>>>>>>>>> have to be updated for some platforms to support the new value(s). >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Mikael >>>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>
