RFR(M): 8046471: Use OPENJDK_TARGET_CPU_ARCH instead of legacy value for hotspot ARCH
Mikael Vidstedt
mikael.vidstedt at oracle.com
Wed Jun 18 19:32:54 UTC 2014
Erik/David - thanks for the reviews!
Cheers,
Mikael
On 2014-06-18 01:14, Erik Joelsson wrote:
> Looks ok to me.
>
> /Erik
>
> On 2014-06-17 21:11, Mikael Vidstedt 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