RFR(M): 8046471: Use OPENJDK_TARGET_CPU_ARCH instead of legacy value for hotspot ARCH
Volker Simonis
volker.simonis at gmail.com
Thu Jun 26 12:51:18 UTC 2014
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.
Regards,
Volker
On Mon, Jun 23, 2014 at 6:42 PM, Volker Simonis
<volker.simonis at 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.vidstedt at 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
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
More information about the build-dev
mailing list