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
