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 13:17:28 UTC 2014
On Thu, Jun 26, 2014 at 2:51 PM, Volker Simonis
<volker.simonis at 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.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