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 <[email protected]> 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 >>>>>>>> >>>>>> >>>>> >>>> >>> >
