RFR(M): 8046471: Use OPENJDK_TARGET_CPU_ARCH instead of legacy value for hotspot ARCH
Mikael Vidstedt
mikael.vidstedt at oracle.com
Tue Jun 17 19:11:16 UTC 2014
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